From 4f3ffea204735f079ae2aa63e5835b8f91e317ab Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 27 Sep 2023 13:21:18 -0400 Subject: [PATCH 1/7] fix focus trapping to respect the currently focused element --- src/internal/active-elements.ts | 11 +++++++++ src/internal/modal.ts | 43 +++++++++++---------------------- src/internal/tabbable.test.ts | 42 ++++++++++++++++++++++++++++---- src/internal/tabbable.ts | 19 +++++---------- 4 files changed, 68 insertions(+), 47 deletions(-) diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index e108ca47c1..cba176ab9e 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -20,3 +20,14 @@ export function* activeElements(activeElement: Element | null = document.activeE yield* activeElements(activeElement.shadowRoot.activeElement); } } + +export function getDeepestActiveElement() { + let activeElement + + for (const element of activeElements()) { + activeElement = element + } + + return activeElement +} + diff --git a/src/internal/modal.ts b/src/internal/modal.ts index 40128393e0..acd8905911 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -1,4 +1,4 @@ -import { activeElements } from './active-elements.js'; +import { getDeepestActiveElement } from './active-elements.js'; import { getTabbableElements } from './tabbable.js'; let activeModals: HTMLElement[] = []; @@ -66,22 +66,6 @@ export default class Modal { this.checkFocus(); }; - get currentFocusIndex() { - 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. - private startElementAlreadyFocused(startElement: HTMLElement) { - for (const activeElement of activeElements()) { - if (startElement === activeElement) { - return true; - } - } - - return false; - } - private handleKeyDown = (event: KeyboardEvent) => { if (event.key !== 'Tab' || this.isExternalActivated) return; @@ -94,29 +78,30 @@ export default class Modal { event.preventDefault(); const tabbableElements = getTabbableElements(this.element); - const start = tabbableElements[0]; - // 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; + // Because sometimes focus can actually be taken over from outside sources, + // we don't want to rely on `this.currentFocus`. Instead we check the actual `activeElement` and + // recurse through shadowRoots. + const currentActiveElement = getDeepestActiveElement() + let currentFocusIndex = tabbableElements.findIndex((el) => el === currentActiveElement) - if (focusIndex === -1) { - this.currentFocus = start; + if (currentFocusIndex === -1) { + this.currentFocus = tabbableElements[0]; this.currentFocus.focus({ preventScroll: true }); return; } const addition = this.tabDirection === 'forward' ? 1 : -1; - if (focusIndex + addition >= tabbableElements.length) { - focusIndex = 0; - } else if (this.currentFocusIndex + addition < 0) { - focusIndex = tabbableElements.length - 1; + if (currentFocusIndex + addition >= tabbableElements.length) { + currentFocusIndex = 0; + } else if (currentFocusIndex + addition < 0) { + currentFocusIndex = tabbableElements.length - 1; } else { - focusIndex += addition; + currentFocusIndex += addition; } - this.currentFocus = tabbableElements[focusIndex]; + this.currentFocus = tabbableElements[currentFocusIndex]; this.currentFocus?.focus({ preventScroll: true }); setTimeout(() => this.checkFocus()); diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index 7807a39eeb..c6054b8793 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -1,9 +1,12 @@ import { elementUpdated, expect, fixture } from '@open-wc/testing'; import '../../dist/shoelace.js'; -import { activeElements } from './active-elements.js'; +import { activeElements, getDeepestActiveElement } from './active-elements.js'; import { html } from 'lit'; import { sendKeys } from '@web/test-runner-commands'; +import type SlInput from '../components/input/input.component.js'; +import type SlButton from '../components/button/button.component.js'; +import type SlIconButton from '../components/icon-button/icon-button.component.js'; async function holdShiftKey(callback: () => Promise) { await sendKeys({ down: 'Shift' }); @@ -19,10 +22,6 @@ function activeElementsArray() { return [...activeElements()]; } -function getDeepestActiveElement() { - return activeElementsArray().pop(); -} - window.customElements.define( 'tab-test-1', class extends HTMLElement { @@ -145,3 +144,36 @@ it('Should allow tabbing to slotted elements', async () => { await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(focusSix); }); + +it("Should account for when focus is changed from outside sources (like clicking)", async () => { + const dialog = await fixture(html` + + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + + Close + + `) + + const inputEl = dialog.querySelector("sl-input") as SlInput + const closeButton = dialog.shadowRoot?.querySelector("sl-icon-button") as SlIconButton + const footerButton = dialog.querySelector("sl-button") as SlButton + + expect(activeElementsArray()).to.not.include(inputEl); + + // Sets focus to the input element + inputEl.focus() + + expect(activeElementsArray()).to.include(inputEl); + + await sendKeys({ press: tabKey }); + + expect(activeElementsArray()).not.to.include(inputEl); + expect(activeElementsArray()).to.include(footerButton); + + // Reset focus back to input el + inputEl.focus() + expect(activeElementsArray()).to.include(inputEl); + + await holdShiftKey(async () => await sendKeys({ press: tabKey })); + expect(activeElementsArray()).to.include(closeButton); +}) diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts index a9e11fa563..fca4ec5c19 100644 --- a/src/internal/tabbable.ts +++ b/src/internal/tabbable.ts @@ -14,11 +14,6 @@ function isTabbable(el: HTMLElement) { return false; } - // Elements with aria-disabled are not tabbable - if (el.hasAttribute('aria-disabled') && el.getAttribute('aria-disabled') !== 'false') { - return false; - } - // Radios without a checked attribute are not tabbable if (tag === 'input' && el.getAttribute('type') === 'radio' && !el.hasAttribute('checked')) { return false; @@ -107,14 +102,12 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) { // Collect all elements including the root walk(root); - 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 tabbableElements.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.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 b4653549efcd358e3e2f1fb5bb8236cc11063d8a Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 27 Sep 2023 13:29:55 -0400 Subject: [PATCH 2/7] prettier --- index.html | 29 +++++++++++++++++++++++++++++ package.json | 16 +++------------- src/internal/active-elements.ts | 7 +++---- src/internal/modal.ts | 4 ++-- src/internal/tabbable.test.ts | 19 ++++++++----------- 5 files changed, 45 insertions(+), 30 deletions(-) create mode 100644 index.html diff --git a/index.html b/index.html new file mode 100644 index 0000000000..d205a8a8e5 --- /dev/null +++ b/index.html @@ -0,0 +1,29 @@ + + + + + + + + + + + + + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + + Close + + + Open Dialog + + + + diff --git a/package.json b/package.json index 426f756330..8e3db89a0a 100644 --- a/package.json +++ b/package.json @@ -25,15 +25,8 @@ "./dist/react/*": "./dist/react/*", "./dist/translations/*": "./dist/translations/*" }, - "files": [ - "dist", - "cdn" - ], - "keywords": [ - "web components", - "custom elements", - "components" - ], + "files": ["dist", "cdn"], + "keywords": ["web components", "custom elements", "components"], "repository": { "type": "git", "url": "git+https://github.com/shoelace-style/shoelace.git" @@ -140,9 +133,6 @@ "user-agent-data-types": "^0.3.0" }, "lint-staged": { - "*.{ts,js}": [ - "eslint --max-warnings 0 --cache --fix", - "prettier --write" - ] + "*.{ts,js}": ["eslint --max-warnings 0 --cache --fix", "prettier --write"] } } diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index cba176ab9e..52d5634dbc 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -22,12 +22,11 @@ export function* activeElements(activeElement: Element | null = document.activeE } export function getDeepestActiveElement() { - let activeElement + let activeElement; for (const element of activeElements()) { - activeElement = element + activeElement = element; } - return activeElement + return activeElement; } - diff --git a/src/internal/modal.ts b/src/internal/modal.ts index acd8905911..fe463ad232 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -82,8 +82,8 @@ export default class Modal { // Because sometimes focus can actually be taken over from outside sources, // we don't want to rely on `this.currentFocus`. Instead we check the actual `activeElement` and // recurse through shadowRoots. - const currentActiveElement = getDeepestActiveElement() - let currentFocusIndex = tabbableElements.findIndex((el) => el === currentActiveElement) + const currentActiveElement = getDeepestActiveElement(); + let currentFocusIndex = tabbableElements.findIndex(el => el === currentActiveElement); if (currentFocusIndex === -1) { this.currentFocus = tabbableElements[0]; diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index c6054b8793..c6705ffc54 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -4,9 +4,6 @@ import '../../dist/shoelace.js'; import { activeElements, getDeepestActiveElement } from './active-elements.js'; import { html } from 'lit'; import { sendKeys } from '@web/test-runner-commands'; -import type SlInput from '../components/input/input.component.js'; -import type SlButton from '../components/button/button.component.js'; -import type SlIconButton from '../components/icon-button/icon-button.component.js'; async function holdShiftKey(callback: () => Promise) { await sendKeys({ down: 'Shift' }); @@ -145,23 +142,23 @@ it('Should allow tabbing to slotted elements', async () => { expect(activeElementsArray()).to.include(focusSix); }); -it("Should account for when focus is changed from outside sources (like clicking)", async () => { +it('Should account for when focus is changed from outside sources (like clicking)', async () => { const dialog = await fixture(html` Lorem ipsum dolor sit amet, consectetur adipiscing elit. Close - `) + `); - const inputEl = dialog.querySelector("sl-input") as SlInput - const closeButton = dialog.shadowRoot?.querySelector("sl-icon-button") as SlIconButton - const footerButton = dialog.querySelector("sl-button") as SlButton + const inputEl = dialog.querySelector('sl-input')!; + const closeButton = dialog.shadowRoot!.querySelector('sl-icon-button')!; + const footerButton = dialog.querySelector('sl-button')!; expect(activeElementsArray()).to.not.include(inputEl); // Sets focus to the input element - inputEl.focus() + inputEl.focus(); expect(activeElementsArray()).to.include(inputEl); @@ -171,9 +168,9 @@ it("Should account for when focus is changed from outside sources (like clicking expect(activeElementsArray()).to.include(footerButton); // Reset focus back to input el - inputEl.focus() + inputEl.focus(); expect(activeElementsArray()).to.include(inputEl); await holdShiftKey(async () => await sendKeys({ press: tabKey })); expect(activeElementsArray()).to.include(closeButton); -}) +}); From d3368af8af34e1bba82c79c34a0e5f16c1000126 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 27 Sep 2023 13:36:52 -0400 Subject: [PATCH 3/7] remove index.html --- index.html | 29 ----------------------------- 1 file changed, 29 deletions(-) delete mode 100644 index.html diff --git a/index.html b/index.html deleted file mode 100644 index d205a8a8e5..0000000000 --- a/index.html +++ /dev/null @@ -1,29 +0,0 @@ - - - - - - - - - - - - - Lorem ipsum dolor sit amet, consectetur adipiscing elit. - - Close - - - Open Dialog - - - - From 69eff2518775b6bbf5ee29b3ed46b4148c161fd1 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Thu, 28 Sep 2023 10:00:30 -0400 Subject: [PATCH 4/7] fix activeElements --- src/internal/active-elements.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index 52d5634dbc..afc672931f 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -22,11 +22,5 @@ export function* activeElements(activeElement: Element | null = document.activeE } export function getDeepestActiveElement() { - let activeElement; - - for (const element of activeElements()) { - activeElement = element; - } - - return activeElement; + return [...activeElements()].pop() } From 75d2e278ca8cc929982037dee506696eb8e09364 Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Thu, 28 Sep 2023 10:00:43 -0400 Subject: [PATCH 5/7] prettier --- src/internal/active-elements.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index afc672931f..b5dc1f188b 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -22,5 +22,5 @@ export function* activeElements(activeElement: Element | null = document.activeE } export function getDeepestActiveElement() { - return [...activeElements()].pop() + return [...activeElements()].pop(); } From 2e71a633707f8d1f9a2080d448ba2e5124c1e17e Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Fri, 29 Sep 2023 09:59:53 -0400 Subject: [PATCH 6/7] update changelog --- docs/pages/resources/changelog.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 22933c6acd..8c381e56a9 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -15,6 +15,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti ## Next - Fixed a bug [in the localize dependency](https://github.com/shoelace-style/localize/issues/20) that caused underscores in language codes to throw a `RangeError` +- Fixed a bug in the focus trapping utility used by modals that caused unexpected focus behavior. [#1583] - Updated `@shoelace-style/localize` to 3.1.0 ## 2.9.0 @@ -1644,4 +1645,4 @@ The following pages demonstrate why this change was necessary. ## 2.0.0-beta.1 -- Initial release +- Initial release \ No newline at end of file From e7d0eabaf921fa664aa10cc0f378850d2d399f7b Mon Sep 17 00:00:00 2001 From: konnorrogers Date: Wed, 4 Oct 2023 13:51:17 -0400 Subject: [PATCH 7/7] prettier --- docs/pages/resources/changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 3a0b5a8ab5..1cd2645567 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -1646,4 +1646,4 @@ The following pages demonstrate why this change was necessary. ## 2.0.0-beta.1 -- Initial release \ No newline at end of file +- Initial release