diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index de5821c4..1cd26455 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] - Fixed a bug in `` that prevented exported tooltip parts from being styled [#1586] - Updated `@shoelace-style/localize` to 3.1.0 diff --git a/src/internal/active-elements.ts b/src/internal/active-elements.ts index e108ca47..b5dc1f18 100644 --- a/src/internal/active-elements.ts +++ b/src/internal/active-elements.ts @@ -20,3 +20,7 @@ export function* activeElements(activeElement: Element | null = document.activeE yield* activeElements(activeElement.shadowRoot.activeElement); } } + +export function getDeepestActiveElement() { + return [...activeElements()].pop(); +} diff --git a/src/internal/modal.ts b/src/internal/modal.ts index 40128393..fe463ad2 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 7807a39e..c6705ffc 100644 --- a/src/internal/tabbable.test.ts +++ b/src/internal/tabbable.test.ts @@ -1,7 +1,7 @@ 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'; @@ -19,10 +19,6 @@ function activeElementsArray() { return [...activeElements()]; } -function getDeepestActiveElement() { - return activeElementsArray().pop(); -} - window.customElements.define( 'tab-test-1', class extends HTMLElement { @@ -145,3 +141,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')!; + 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(); + + 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 a9e11fa5..fca4ec5c 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; + }); }