Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle shadow children when trapping focus #397

Merged
merged 7 commits into from
Jan 10, 2023
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 82 additions & 9 deletions src/a11y-dialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,17 +212,82 @@ function moveFocusToDialog(node: HTMLElement) {
focused.focus()
}

// Elements with these ARIA roles make their children
// `presentational`, which nullifies their semantics.
// @see: https://www.w3.org/TR/wai-aria/
Comment on lines +221 to +223
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that relevant to finding focusable children though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, yes. It’s a common enough pattern for anchors and buttons to have child elements, and

  1. we may as well bail out instead of walking into their subtrees, plus,
  2. if the author has for some reason done something like a > a, the browser won’t count the descendant a as tabbable and nor should we

const PRESENTATIONAL_CHILDREN_SELECTOR = `
mxmason marked this conversation as resolved.
Show resolved Hide resolved
a[href],
button,
img,
summary,
[role="button"],
[role="image"],
[role="link"],
[role="math"],
[role="progressbar"],
[role="scrollbar"],
[role="slider"]
`
mxmason marked this conversation as resolved.
Show resolved Hide resolved

/**
* Get the focusable children of the given element.
* Get focusable children by recursively traversing the subtree of `node`.
* This traversal allows us to account for Shadow DOM subtrees.
*/
function getFocusableChildren(node: HTMLElement): HTMLElement[] {
return $$(focusableSelectors.join(','), node).filter(
child =>
!!(
child.offsetWidth ||
child.offsetHeight ||
child.getClientRects().length
)
function getFocusableChildren(node: ParentNode): HTMLElement[] {
// Check for the bases case of our recursion:
if (node instanceof HTMLElement) {
// If this node is marked as inert, neither it nor any member of
// its subtree will be focusable.
if (node.inert === true) return []
mxmason marked this conversation as resolved.
Show resolved Hide resolved

// If this node has no children, we can stop traversing.
// If this node has children, but nullifies its children's
// semantics, we can stop traversing.
if (
!node.firstElementChild ||
node.matches(PRESENTATIONAL_CHILDREN_SELECTOR)
) {
// Check if the node is focusable, and then return early.
return isFocusable(node) ? [node] : []
}
}

let focusableEls: HTMLElement[] = []

// Walk all the immediate children of this node
// (with some type casting because node.children is an HTMLCollection)
for (const curr of node.children as unknown as HTMLElement[]) {
// If this element has a Shadow DOM attached,
// check the shadow subtree for focusable children.
if (!!curr.shadowRoot) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this should be enough:

Suggested change
if (!!curr.shadowRoot) {
if (curr.shadowRoot) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I had a reason for wanting to write it this way, but the naïve tests I just did didn't yield any weird behavior.

focusableEls = [...focusableEls, ...getFocusableChildren(curr.shadowRoot)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also write it like this, although it’s worth noting it would fail if there are over 65535 nodes (the typical maximum amount of arguments allowed by a function call):

Suggested change
focusableEls = [...focusableEls, ...getFocusableChildren(curr.shadowRoot)]
focusableEls.push(...getFocusableChildren(curr.shadowRoot))

If we do use .push(), we should also use it below. :)

Copy link
Contributor Author

@mxmason mxmason Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I did a benchmark for this. On my machine, it looks like:

  • In Chrome/Edge, [...oldNodes, ...newNodes] is fastest
  • In Firefox and Safari, oldNodes.push.apply(oldNodes, newNodes) is fastest
  • In all browsers, oldNodes.push(...newNodes) is slowest.

In Chromium, push.apply is second-fastest so I think it's the optimal choice between all 3 major browser engines. Interesting, because I would have thought it might be less expensive to make a new array than to modify one in place :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amending my earlier commentary: I don't think this implementation detail will have that much impact on our users. The time it takes to do this operation is probably negligible in the real world, regardless of implementation. I'm happy to male the change we discussed, if you feel strongly about it!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely marginal if anything. :)


// If this is a slot, look for any elements assigned to it
// then check each of those for focusable children.
} else if (curr.localName === 'slot') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a slot? 😅

Copy link
Contributor Author

@mxmason mxmason Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slots allow authors to define a place (a slot) for developer-supplied Light DOM content to be rendered inside a Web Component. Let's take this template for the <fancy-card> Web Component:

<template id="fancy-card-template">
  <slot name="title">
    <!-- Developer-supplied Light DOM node goes here -->
  </slot>
  <slot name="content">
    <!-- Developer-supplied Light DOM node goes here -->
  </slot>
</template>

This template has two slots:

  1. title, for the title of the card
  2. content, for long-form text content

The developer might use it like this.

<fancy-card>
  <h2 slot="title">Why my cat is a weird little guy</h2>
  <p slot="content">He sits like a man.</p>
</fancy-card>

The <h2> will go inside the title slot, and the <p> will go inside the content slot. These slotted children will be in the Light DOM, which is why we have to specifically look for them by calling assignedElements() on the slot.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Thank you for the explanation. :)

let assignedElements = (curr as HTMLSlotElement).assignedElements()
mxmason marked this conversation as resolved.
Show resolved Hide resolved
for (const assignedElement of assignedElements) {
focusableEls = [
...focusableEls,
...getFocusableChildren(assignedElement),
]
}

// Or else check this node's subtree for focusable children
} else {
focusableEls = [...focusableEls, ...getFocusableChildren(curr)]
}
}
return focusableEls
}

/**
* Determine if an element is focusable and has user-visible painted dimensions
*/
function isFocusable(el: HTMLElement) {
return (
el.matches(focusableSelectors.join(',')) &&
!!(el.offsetWidth || el.offsetHeight || el.getClientRects().length)
)
}

Expand Down Expand Up @@ -265,3 +330,11 @@ if (typeof document !== 'undefined') {
instantiateDialogs()
}
}

// TypeScript doesn't know about `inert` yet; this declaration extends
// the HTMLElement interface to include it.
declare global {
interface HTMLElement {
inert: boolean
}
}