diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 207fc9c7..411aefea 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -16,6 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - 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 index 7153c320..993480eb 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -11,12 +11,12 @@ * } * } */ -export function* activeElements (activeElement: Element | null = document.activeElement): Generator { - if (activeElement == null) return +export function* activeElements(activeElement: Element | null = document.activeElement): Generator { + if (activeElement === null || activeElement === undefined) return; - yield activeElement + yield activeElement; - if ("shadowRoot" in activeElement && activeElement.shadowRoot && activeElement.shadowRoot.mode !== "closed") { - yield* activeElements(activeElement.shadowRoot.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 ec3f6946..adbb1a8e 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -1,5 +1,5 @@ +import { activeElements } from './active-elements.js'; import { getTabbableElements } from './tabbable.js'; -import { activeElements } from './active-elements.js' let activeModals: HTMLElement[] = []; @@ -60,15 +60,15 @@ export default class Modal { * Checks if the `startElement` is already focused. This is important if the modal already * has an existing focused prior to the first tab key. */ - startElementAlreadyFocused (startElement: HTMLElement) { + startElementAlreadyFocused(startElement: HTMLElement) { for (const activeElement of activeElements()) { if (startElement === activeElement) { - // - return true + // + return true; } } - return false + return false; } handleKeyDown = (event: KeyboardEvent) => { diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 4d9b202a..4367d0f6 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -1,46 +1,51 @@ -import { elementUpdated, expect, fixture, aTimeout } from '@open-wc/testing'; +import { elementUpdated, expect, fixture } from '@open-wc/testing'; -import "../../dist/shoelace.js" +import '../../dist/shoelace.js'; import { html } from 'lit'; -import type { SlDrawer } from '../../dist/shoelace.js'; import { sendKeys } from '@web/test-runner-commands'; +import type { SlDrawer } from '../../dist/shoelace.js'; -function getActiveElements (el: null | Element = document.activeElement) { - const elements: Element[] = [] +function getActiveElements(el: null | Element = document.activeElement) { + const elements: Element[] = []; - function walk (el: null | Element) { - if (el == null) { - return + function walk(element: null | Element) { + if (element === null || element === undefined) { + return; } - elements.push(el) + elements.push(element); - if ("shadowRoot" in el && el.shadowRoot && el.shadowRoot.mode !== "closed") { - walk(el.shadowRoot.activeElement) + if ('shadowRoot' in element && element.shadowRoot && element.shadowRoot.mode !== 'closed') { + walk(element.shadowRoot.activeElement); } } - walk(el) + walk(el); - return elements + return elements; } -function getDeepestActiveElement (el: null | Element = document.activeElement) { - const activeElements = getActiveElements(el) +function getDeepestActiveElement(el: null | Element = document.activeElement) { + const activeElements = getActiveElements(el); - return activeElements[activeElements.length - 1] + return activeElements[activeElements.length - 1]; } -async function holdShiftKey (callback: () => Promise) { - await sendKeys({ down: "Shift" }) - await callback() - await sendKeys({ up: "Shift" }) +async function holdShiftKey(callback: () => Promise) { + await sendKeys({ down: 'Shift' }); + await callback(); + await sendKeys({ up: 'Shift' }); } -window.customElements.define("tab-test-1", class extends HTMLElement { - connectedCallback () { - this.attachShadow({ mode: "open" }) - this.shadowRoot!.innerHTML = ` +window.customElements.define( + 'tab-test-1', + class extends HTMLElement { + constructor() { + super(); + this.attachShadow({ mode: 'open' }); + } + connectedCallback() { + this.shadowRoot!.innerHTML = ` @@ -48,12 +53,12 @@ window.customElements.define("tab-test-1", class extends HTMLElement { - ` + `; + } } -}) +); - -it("Should allow tabbing to slotted elements", async () => { +it('Should allow tabbing to slotted elements', async () => { const el = await fixture(html`
@@ -72,77 +77,77 @@ it("Should allow tabbing to slotted elements", async () => {
- `) + `); - const drawer = el.shadowRoot!.querySelector("sl-drawer") as unknown as SlDrawer + const drawer = el.shadowRoot!.querySelector('sl-drawer') as unknown as SlDrawer; - await drawer.show() + await drawer.show(); - await elementUpdated(drawer) + await elementUpdated(drawer); - const focusZero = drawer.shadowRoot!.querySelector("[role='dialog']") - const focusOne = el.querySelector("#focus-1") - const focusTwo = drawer.shadowRoot!.querySelector("[part~='close-button']") - const focusThree = el.querySelector("#focus-3") - const focusFour = el.querySelector("#focus-4") - const focusFive = el.querySelector("#focus-5") - const focusSix = el.querySelector("#focus-6") + const focusZero = drawer.shadowRoot!.querySelector("[role='dialog']"); + const focusOne = el.querySelector('#focus-1'); + const focusTwo = drawer.shadowRoot!.querySelector("[part~='close-button']"); + 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) + expect(getDeepestActiveElement()).to.equal(focusZero); - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusOne) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusOne); // When we hit the key we should go to the "close button" on the drawer - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusTwo) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusTwo); - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusThree) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusThree); - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusFour) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusFour); - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusFive) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusFive); - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusSix) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusSix); // Now we should loop back to #panel - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusZero) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusZero); // Now we should loop back to #panel - await sendKeys({ press: "Tab" }) - expect(getActiveElements()).to.include(focusOne) + await sendKeys({ press: 'Tab' }); + expect(getActiveElements()).to.include(focusOne); // Let's reset and try from starting point 0 and go backwards. - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusZero) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusZero); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusSix) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusSix); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusFive) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusFive); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusFour) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusFour); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusThree) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusThree); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusTwo) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusTwo); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusOne) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusOne); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusZero) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusZero); - await holdShiftKey(async () => await sendKeys({ press: "Tab" })) - expect(getActiveElements()).to.include(focusSix) -}) + await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + expect(getActiveElements()).to.include(focusSix); +}); diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts index 1c426294..4baa4c83 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -75,8 +75,8 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { function walk(el: HTMLElement | ShadowRoot) { if (el instanceof Element) { // if the element has "inert" we can just no-op it. - if (el.hasAttribute("inert")) { - return + if (el.hasAttribute('inert')) { + return; } if (!allElements.includes(el)) { @@ -84,7 +84,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { } if (!tabbableElements.includes(el) && isTabbable(el)) { - tabbableElements.push(el) + tabbableElements.push(el); } /** @@ -92,12 +92,13 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { * However, there is an edge case if the `root` is wrapped by another shadowDOM, it won't grab the children. * This fixes that fun edge case. */ - const slotElementsOutsideRootElement = (el: HTMLSlotElement) => (el.getRootNode({ composed: true }) as ShadowRoot | null)?.host !== root + const slotChildrenOutsideRootElement = (slotElement: HTMLSlotElement) => + (slotElement.getRootNode({ composed: true }) as ShadowRoot | null)?.host !== root; - if (el instanceof HTMLSlotElement && slotElementsOutsideRootElement(el)) { - el.assignedElements({ flatten: true }).forEach((el: HTMLElement) => { - walk(el) - }) + if (el instanceof HTMLSlotElement && slotChildrenOutsideRootElement(el)) { + el.assignedElements({ flatten: true }).forEach((assignedEl: HTMLElement) => { + walk(assignedEl); + }); } if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') { @@ -111,7 +112,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { // Collect all elements including the root walk(root); - return tabbableElements + 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?