diff --git a/src/components/dialog/dialog.component.ts b/src/components/dialog/dialog.component.ts index 6d31d02a..1884c6b3 100644 --- a/src/components/dialog/dialog.component.ts +++ b/src/components/dialog/dialog.component.ts @@ -300,7 +300,7 @@ export default class SlDialog extends ShoelaceElement { ` : ''} ${ - '' /* The tabindex="-1" is here because the body is technically scrollable if overflowing. However, if there's no focusable elements inside, you won't actually be able to scroll it via keyboard. Previously this was just a , but tabindex="-1" on the slot causes children to not be focusable. https://github.com/shoelace-style/shoelace/issues/1753#issuecomment-1836803277 */ + '' /* The tabindex="-1" is here because the body is technically scrollable if overflowing. However, if there's no focusable elements inside, you won't actually be able to scroll it via keyboard. Previously this was just a , but tabindex="-1" on the slot causes children to not be focusable. https://github.com/shoelace-style/shoelace/issues/1753#issuecomment-1836803277 */ }
diff --git a/src/internal/modal.ts b/src/internal/modal.ts index 10cca881..cefd2bf0 100644 --- a/src/internal/modal.ts +++ b/src/internal/modal.ts @@ -8,9 +8,13 @@ export default class Modal { isExternalActivated: boolean; tabDirection: 'forward' | 'backward' = 'forward'; currentFocus: HTMLElement | null; + previousFocus: HTMLElement | null; + elementsWithTabbableControls: string[]; constructor(element: HTMLElement) { this.element = element; + + this.elementsWithTabbableControls = ['iframe']; } /** Activates focus trapping. */ @@ -56,7 +60,7 @@ export default class Modal { if (typeof target?.focus === 'function') { this.currentFocus = target; - target.focus({ preventScroll: true }); + target.focus({ preventScroll: false }); } } } @@ -67,22 +71,25 @@ export default class Modal { this.checkFocus(); }; + private possiblyHasTabbableChildren(element: HTMLElement) { + return ( + this.elementsWithTabbableControls.includes(element.tagName.toLowerCase()) || element.hasAttribute('controls') + // Should we add a data-attribute for people to set just in case they have an element where we don't know if it has possibly tabbable elements? + ); + } + private handleKeyDown = (event: KeyboardEvent) => { if (event.key !== 'Tab' || this.isExternalActivated) return; if (!this.isActive()) return; - const elementsWithTabbableControls = [ - "audio", - "video", - "iframe" - ] + // 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(); + this.previousFocus = currentActiveElement as HTMLElement | null; - const possiblyHasTabbableChildren = (element: HTMLElement) => { - return ( - elementsWithTabbableControls.includes(element.tagName.toLowerCase()) - || element.hasAttribute("controls") - // Should we add a data-attribute for people to set just in case they have an element where we don't know if it has possibly tabbable elements? - ) + if (this.previousFocus && this.possiblyHasTabbableChildren(this.previousFocus)) { + return; } if (event.shiftKey) { @@ -93,23 +100,21 @@ export default class Modal { const tabbableElements = getTabbableElements(this.element); - // 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); + this.previousFocus = this.currentFocus; + if (currentFocusIndex === -1) { this.currentFocus = tabbableElements[0]; // We don't call event.preventDefault() here because it messes with tabbing to the