diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 19ac462cb7..e1f0c530b2 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -17,6 +17,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Added `--isolatedModules` and `--verbatimModuleSyntax` to `tsconfig.json`. For anyone directly importing event types, they no longer provide a default export due to these options being enabled. For people using the `events/event.js` file directly, there is no change. - Added support for submenus in `` [#1410] - Added the `--submenu-offset` custom property to `` [#1410] +- Fixed an issue with focus trapping elements like `` when wrapped by other elements not checking the assigned elements of ``s. [#1537] - Fixed type issues with the `ref` attribute in React Wrappers. [#1526] - Fixed a regression that caused `` to render incorrectly with gaps [#1523] - Improved expand/collapse behavior of `` to work more like users expect [#1521] diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts new file mode 100644 index 0000000000..e108ca47c1 --- /dev/null +++ b/src/internal/active-elements.ts @@ -0,0 +1,22 @@ +/** + * Use a generator so we can iterate and possibly break early. + * @example + * // to operate like a regular array. This kinda nullifies generator benefits, but worth knowing if you need the whole array. + * const allActiveElements = [...activeElements()] + * + * // Early return + * for (const activeElement of activeElements()) { + * if () { + * break; // Break the loop, dont need to iterate over the whole array or store an array in memory! + * } + * } + */ +export function* activeElements(activeElement: Element | null = document.activeElement): Generator { + if (activeElement === null || activeElement === undefined) return; + + yield activeElement; + + if ('shadowRoot' in activeElement && activeElement.shadowRoot && activeElement.shadowRoot.mode !== 'closed') { + yield* activeElements(activeElement.shadowRoot.activeElement); + } +} diff --git a/src/internal/modal.ts b/src/internal/modal.ts index 86bc866c03..680a0e97d8 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -1,3 +1,4 @@ +import { activeElements } from './active-elements.js'; import { getTabbableElements } from './tabbable.js'; let activeModals: HTMLElement[] = []; @@ -55,6 +56,20 @@ export default class Modal { return getTabbableElements(this.element).findIndex(el => el === this.currentFocus); } + /** + * Checks if the `startElement` is already focused. This is important if the modal already + * has an existing focus prior to the first tab key. + */ + startElementAlreadyFocused(startElement: HTMLElement) { + for (const activeElement of activeElements()) { + if (startElement === activeElement) { + return true; + } + } + + return false; + } + handleKeyDown = (event: KeyboardEvent) => { if (event.key !== 'Tab') return; @@ -68,7 +83,10 @@ export default class Modal { const tabbableElements = getTabbableElements(this.element); const start = tabbableElements[0]; - let focusIndex = this.currentFocusIndex; + + // Sometimes we programmatically focus the first element in a modal. + // Lets make sure the start element isn't already focused. + let focusIndex = this.startElementAlreadyFocused(start) ? 0 : this.currentFocusIndex; if (focusIndex === -1) { this.currentFocus = start; diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts new file mode 100644 index 0000000000..7807a39eeb --- /dev/null +++ b/src/internal/tabbable.test.ts @@ -0,0 +1,147 @@ +import { elementUpdated, expect, fixture } from '@open-wc/testing'; + +import '../../dist/shoelace.js'; +import { activeElements } from './active-elements.js'; +import { html } from 'lit'; +import { sendKeys } from '@web/test-runner-commands'; + +async function holdShiftKey(callback: () => Promise) { + await sendKeys({ down: 'Shift' }); + await callback(); + await sendKeys({ up: 'Shift' }); +} + +const tabKey = + navigator.userAgent.includes('Safari') && !navigator.userAgent.includes('HeadlessChrome') ? 'Alt+Tab' : 'Tab'; + +// Simple helper to turn the activeElements generator into an array +function activeElementsArray() { + return [...activeElements()]; +} + +function getDeepestActiveElement() { + return activeElementsArray().pop(); +} + +window.customElements.define( + 'tab-test-1', + class extends HTMLElement { + constructor() { + super(); + this.attachShadow({ mode: 'open' }); + } + connectedCallback() { + this.shadowRoot!.innerHTML = ` + + + + + + + + `; + } + } +); + +it('Should allow tabbing to slotted elements', async () => { + const el = await fixture(html` + +
+ Focus 1 +
+ +
+ + Focus 3 +
+ +
+
Focus 6
+ +
+
+ `); + + const drawer = el.shadowRoot?.querySelector('sl-drawer'); + + if (drawer === null || drawer === undefined) throw Error('Could not find drawer inside of the test element'); + + await drawer.show(); + + await elementUpdated(drawer); + + const focusZero = drawer.shadowRoot?.querySelector("[role='dialog']"); + + if (focusZero === null || focusZero === undefined) throw Error('Could not find dialog panel inside '); + + const focusOne = el.querySelector('#focus-1'); + const focusTwo = drawer.shadowRoot?.querySelector("[part~='close-button']"); + + if (focusTwo === null || focusTwo === undefined) throw Error('Could not find close button inside '); + + const focusThree = el.querySelector('#focus-3'); + const focusFour = el.querySelector('#focus-4'); + const focusFive = el.querySelector('#focus-5'); + const focusSix = el.querySelector('#focus-6'); + + // When we open drawer, we should be focused on the panel to start. + expect(getDeepestActiveElement()).to.equal(focusZero); + + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusOne); + + // When we hit the key we should go to the "close button" on the drawer + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusTwo); + + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusThree); + + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusFour); + + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusFive); + + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusSix); + + // Now we should loop back to #panel + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusZero); + + // Now we should loop back to #panel + await sendKeys({ press: tabKey }); + expect(activeElementsArray()).to.include(focusOne); + + // Let's reset and try from starting point 0 and go backwards. + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusZero); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusSix); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusFive); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusFour); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusThree); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusTwo); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusOne); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusZero); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(focusSix); +}); diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts index 4925e2ab7f..74ea767430 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -70,10 +70,36 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) { export function getTabbableElements(root: HTMLElement | ShadowRoot) { const allElements: HTMLElement[] = []; + const tabbableElements: HTMLElement[] = []; function walk(el: HTMLElement | ShadowRoot) { if (el instanceof Element) { - allElements.push(el); + // if the element has "inert" we can just no-op it. + if (el.hasAttribute('inert')) { + return; + } + + if (!allElements.includes(el)) { + allElements.push(el); + } + + if (!tabbableElements.includes(el) && isTabbable(el)) { + tabbableElements.push(el); + } + + /** + * This looks funky. Basically a slot's children will always be picked up *if* they're within the `root` element. + * However, there is an edge case when, if the `root` is wrapped by another shadow DOM, it won't grab the children. + * This fixes that fun edge case. + */ + const slotChildrenOutsideRootElement = (slotElement: HTMLSlotElement) => + (slotElement.getRootNode({ composed: true }) as ShadowRoot | null)?.host !== root; + + if (el instanceof HTMLSlotElement && slotChildrenOutsideRootElement(el)) { + el.assignedElements({ flatten: true }).forEach((assignedEl: HTMLElement) => { + walk(assignedEl); + }); + } if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') { walk(el.shadowRoot); @@ -86,10 +112,14 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { // Collect all elements including the root walk(root); - return allElements.filter(isTabbable).sort((a, b) => { - // Make sure we sort by tabindex. - const aTabindex = Number(a.getAttribute('tabindex')) || 0; - const bTabindex = Number(b.getAttribute('tabindex')) || 0; - return bTabindex - aTabindex; - }); + return tabbableElements; + + // Is this worth having? Most sorts will always add increased overhead. And positive tabindexes shouldn't really be used. + // So is it worth being right? Or fast? + // return allElements.filter(isTabbable).sort((a, b) => { + // // Make sure we sort by tabindex. + // const aTabindex = Number(a.getAttribute('tabindex')) || 0; + // const bTabindex = Number(b.getAttribute('tabindex')) || 0; + // return bTabindex - aTabindex; + // }); }