From 04721fad71ae3ac2ef7c549b830627f177175b49 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:13:27 -0400 Subject: [PATCH 01/12] fix: internal logic for tabbable checks slotted elements --- src/internal/active-elements.ts | 22 +++++ src/internal/modal.ts | 21 ++++- src/internal/tabbable.test.ts | 148 ++++++++++++++++++++++++++++++++ src/internal/tabbable.ts | 43 ++++++++-- 4 files changed, 226 insertions(+), 8 deletions(-) create mode 100644 src/internal/active-elements.ts create mode 100644 src/internal/tabbable.test.ts diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts new file mode 100644 index 0000000000..7153c3203c --- /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 gets kinda nullify generator benefits. + * const allActiveElements = [...activeElements()] + * + * // Earlier 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) 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..ec3f69463c 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -1,4 +1,5 @@ import { getTabbableElements } from './tabbable.js'; +import { activeElements } from './active-elements.js' let activeModals: HTMLElement[] = []; @@ -55,6 +56,21 @@ 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 focused 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 +84,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..4d9b202a7c --- /dev/null +++ b/src/internal/tabbable.test.ts @@ -0,0 +1,148 @@ +import { elementUpdated, expect, fixture, aTimeout } from '@open-wc/testing'; + +import "../../dist/shoelace.js" +import { html } from 'lit'; +import type { SlDrawer } from '../../dist/shoelace.js'; +import { sendKeys } from '@web/test-runner-commands'; + +function getActiveElements (el: null | Element = document.activeElement) { + const elements: Element[] = [] + + function walk (el: null | Element) { + if (el == null) { + return + } + + elements.push(el) + + if ("shadowRoot" in el && el.shadowRoot && el.shadowRoot.mode !== "closed") { + walk(el.shadowRoot.activeElement) + } + } + + walk(el) + + return elements +} + +function getDeepestActiveElement (el: null | Element = document.activeElement) { + const activeElements = getActiveElements(el) + + return activeElements[activeElements.length - 1] +} + +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 = ` + + + + + + + + ` + } +}) + + +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") as unknown as SlDrawer + + await drawer.show() + + 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") + + // When we open drawer, we should be focused on the panel to start. + expect(getDeepestActiveElement()).to.equal(focusZero) + + 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(focusThree) + + 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(focusSix) + + // Now we should loop back to #panel + 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) + + // 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(focusSix) + + 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(focusThree) + + 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(focusZero) + + 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 4925e2ab7f..1c42629456 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -70,10 +70,35 @@ 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 slots children will always be picked up *if* they're within the `root` element. + * 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 + + if (el instanceof HTMLSlotElement && slotElementsOutsideRootElement(el)) { + el.assignedElements({ flatten: true }).forEach((el: HTMLElement) => { + walk(el) + }) + } if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') { walk(el.shadowRoot); @@ -86,10 +111,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; + // }); } From 90ed5cd8e7f1693a7ed36e3018d15dd3c7df4ef4 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:20:44 -0400 Subject: [PATCH 02/12] prettier --- docs/pages/resources/changelog.md | 1 + src/internal/active-elements.ts | 10 +- src/internal/modal.ts | 10 +- src/internal/tabbable.test.ts | 155 +++++++++++++++--------------- src/internal/tabbable.ts | 19 ++-- 5 files changed, 101 insertions(+), 94 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 207fc9c7d2..411aefeaf3 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 7153c3203c..993480eb9b 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 ec3f69463c..adbb1a8ee0 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 4d9b202a7c..4367d0f699 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 1c42629456..4baa4c83da 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? From 0fdd25485df2b71863e6d3b9c95a2e9dbfe67127 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:22:18 -0400 Subject: [PATCH 03/12] add better note for generators --- src/internal/active-elements.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index 993480eb9b..e108ca47c1 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -1,10 +1,10 @@ /** * Use a generator so we can iterate and possibly break early. * @example - * // to operate like a regular array. This gets kinda nullify generator benefits. + * // to operate like a regular array. This kinda nullifies generator benefits, but worth knowing if you need the whole array. * const allActiveElements = [...activeElements()] * - * // Earlier return + * // 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! From e9a008ae75d4afc3390559484131d69f23fa8e4f Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:23:01 -0400 Subject: [PATCH 04/12] prettier --- src/internal/modal.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/internal/modal.ts b/src/internal/modal.ts index adbb1a8ee0..dfb54f8502 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -63,7 +63,6 @@ export default class Modal { startElementAlreadyFocused(startElement: HTMLElement) { for (const activeElement of activeElements()) { if (startElement === activeElement) { - // return true; } } From ea75462c3de0a17e67ffe4decf93dce1c448b053 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:40:24 -0400 Subject: [PATCH 05/12] fix tests --- src/internal/tabbable.test.ts | 70 ++++++++++++++--------------------- 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 4367d0f699..0f98444363 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -4,32 +4,7 @@ import '../../dist/shoelace.js'; import { html } from 'lit'; 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 walk(element: null | Element) { - if (element === null || element === undefined) { - return; - } - - elements.push(element); - - if ('shadowRoot' in element && element.shadowRoot && element.shadowRoot.mode !== 'closed') { - walk(element.shadowRoot.activeElement); - } - } - - walk(el); - - return elements; -} - -function getDeepestActiveElement(el: null | Element = document.activeElement) { - const activeElements = getActiveElements(el); - - return activeElements[activeElements.length - 1]; -} +import { activeElements } from './active-elements.js'; async function holdShiftKey(callback: () => Promise) { await sendKeys({ down: 'Shift' }); @@ -37,6 +12,15 @@ async function holdShiftKey(callback: () => Promise) { await sendKeys({ up: 'Shift' }); } +// 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 { @@ -97,57 +81,57 @@ it('Should allow tabbing to slotted elements', async () => { expect(getDeepestActiveElement()).to.equal(focusZero); await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusOne); + expect(activeElementsArray()).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); + expect(activeElementsArray()).to.include(focusTwo); await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusThree); + expect(activeElementsArray()).to.include(focusThree); await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusFour); + expect(activeElementsArray()).to.include(focusFour); await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusFive); + expect(activeElementsArray()).to.include(focusFive); await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusSix); + expect(activeElementsArray()).to.include(focusSix); // Now we should loop back to #panel await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusZero); + expect(activeElementsArray()).to.include(focusZero); // Now we should loop back to #panel await sendKeys({ press: 'Tab' }); - expect(getActiveElements()).to.include(focusOne); + expect(activeElementsArray()).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); + expect(activeElementsArray()).to.include(focusZero); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusSix); + expect(activeElementsArray()).to.include(focusSix); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusFive); + expect(activeElementsArray()).to.include(focusFive); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusFour); + expect(activeElementsArray()).to.include(focusFour); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusThree); + expect(activeElementsArray()).to.include(focusThree); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusTwo); + expect(activeElementsArray()).to.include(focusTwo); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusOne); + expect(activeElementsArray()).to.include(focusOne); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusZero); + expect(activeElementsArray()).to.include(focusZero); await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); - expect(getActiveElements()).to.include(focusSix); + expect(activeElementsArray()).to.include(focusSix); }); From d2b7c9f64903a0002f0530d684f311b1274d7e2c Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:42:29 -0400 Subject: [PATCH 06/12] prettier --- src/internal/tabbable.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 0f98444363..824f9d1ef5 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -1,10 +1,10 @@ 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'; import type { SlDrawer } from '../../dist/shoelace.js'; -import { activeElements } from './active-elements.js'; async function holdShiftKey(callback: () => Promise) { await sendKeys({ down: 'Shift' }); @@ -13,12 +13,12 @@ async function holdShiftKey(callback: () => Promise) { } // Simple helper to turn the activeElements generator into an array -function activeElementsArray () { - return [...activeElements()] +function activeElementsArray() { + return [...activeElements()]; } -function getDeepestActiveElement () { - return activeElementsArray().pop() +function getDeepestActiveElement() { + return activeElementsArray().pop(); } window.customElements.define( From 1d2540a4bd00e2d5d59fecf1104a42443f434bd4 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Tue, 22 Aug 2023 17:52:39 -0400 Subject: [PATCH 07/12] prettier --- src/internal/tabbable.test.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 824f9d1ef5..c86de3e69b 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -4,7 +4,6 @@ import '../../dist/shoelace.js'; import { activeElements } from './active-elements.js'; import { html } from 'lit'; import { sendKeys } from '@web/test-runner-commands'; -import type { SlDrawer } from '../../dist/shoelace.js'; async function holdShiftKey(callback: () => Promise) { await sendKeys({ down: 'Shift' }); @@ -63,15 +62,23 @@ 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'); + + 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']"); + 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']"); + 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'); From 5827e0b8c624f509e57e654f6fe1f9fd969a1b03 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 23 Aug 2023 10:59:25 -0400 Subject: [PATCH 08/12] fix tabbable test for safari --- src/internal/tabbable.test.ts | 38 +++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index c86de3e69b..3701eb9425 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -11,6 +11,10 @@ async function holdShiftKey(callback: () => Promise) { 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()]; @@ -87,58 +91,58 @@ it('Should allow tabbing to slotted elements', async () => { // When we open drawer, we should be focused on the panel to start. expect(getDeepestActiveElement()).to.equal(focusZero); - await sendKeys({ press: 'Tab' }); + 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: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusTwo); - await sendKeys({ press: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusThree); - await sendKeys({ press: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusFour); - await sendKeys({ press: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusFive); - await sendKeys({ press: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusSix); // Now we should loop back to #panel - await sendKeys({ press: 'Tab' }); + await sendKeys({ press: tabKey }); expect(activeElementsArray()).to.include(focusZero); // Now we should loop back to #panel - await sendKeys({ press: 'Tab' }); + 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: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusZero); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusSix); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusFive); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusFour); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusThree); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusTwo); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusOne); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusZero); - await holdShiftKey(async () => await sendKeys({ press: 'Tab' })); + await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusSix); }); From 5e5800b5cfbc2c22d4cd4a24ac693a000b3590b1 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 23 Aug 2023 11:01:33 -0400 Subject: [PATCH 09/12] prettier --- src/internal/tabbable.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 3701eb9425..7807a39eeb 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -11,9 +11,8 @@ async function holdShiftKey(callback: () => Promise) { await sendKeys({ up: 'Shift' }); } -const tabKey = navigator.userAgent.includes('Safari') && !navigator.userAgent.includes('HeadlessChrome') - ? 'Alt+Tab' - : 'Tab'; +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() { From 28facf9551ca1c3a4c793499860ec0d9cdc4e5cd Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Wed, 23 Aug 2023 11:19:14 -0400 Subject: [PATCH 10/12] Update src/internal/tabbable.ts Co-authored-by: Cory LaViska --- src/internal/tabbable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts index 4baa4c83da..c4aa8c753e 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -89,7 +89,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { /** * This looks funky. Basically a slots children will always be picked up *if* they're within the `root` element. - * However, there is an edge case if the `root` is wrapped by another shadowDOM, it won't grab the children. + * 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) => From c8f9482b9c67816253eff95864bf53cdfe791a2b Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Wed, 23 Aug 2023 11:19:20 -0400 Subject: [PATCH 11/12] Update src/internal/modal.ts Co-authored-by: Cory LaViska --- src/internal/modal.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/modal.ts b/src/internal/modal.ts index dfb54f8502..680a0e97d8 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -58,7 +58,7 @@ 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. + * has an existing focus prior to the first tab key. */ startElementAlreadyFocused(startElement: HTMLElement) { for (const activeElement of activeElements()) { From 764d22e7d8d794988ccb6cc105e69f178ec97a37 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Wed, 23 Aug 2023 11:19:27 -0400 Subject: [PATCH 12/12] Update src/internal/tabbable.ts Co-authored-by: Cory LaViska --- src/internal/tabbable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts index c4aa8c753e..74ea767430 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -88,7 +88,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { } /** - * This looks funky. Basically a slots children will always be picked up *if* they're within the `root` element. + * 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. */