Repository navigation
fix(os-modal): repair focus trap for shadow-DOM slotted components - #905
Open
KarunyaChavan wants to merge 6 commits into
Open
KarunyaChavan wants to merge 6 commits into
KarunyaChavan wants to merge 6 commits into
Conversation
- Traverse slotted content and nested shadow roots for focusable elements. - Improve initial focus to prefer autofocus and the first body control. - Preserve tab and shift-tab wrap-around across shadow DOM boundaries. - Restore focus to the original opener when the modal closes.
- Remove `isVisible` and its `offsetParent` check, which was unreliable inside shadow roots and broke subtree traversal in jsdom. Subtree pruning now relies solely on explicit accessibility signals (`hidden` and `aria-hidden="true"`). - Collapse identical child-iteration paths for `HTMLElement` and `DocumentFragment` in `collectFocusables` to adhere to DRY principles. - Use the existing `eventSource` helper in `_onBackdrop` instead of duplicating the `composedPath()[0]` resolution logic. - Remove redundant focus restoration from `disconnectedCallback` since `attributeChangedCallback` already handles it, preventing potential exceptions when the predecessor element is detached.
KarunyaChavan
marked this pull request as ready for review
September 25, 2026 08:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #852
This PR fixes a keyboard accessibility bug in
<os-modal>where focus could escape the modal - or never move into it at all - when the modal body contained custom<os-*>web components such as<os-range-field>,<os-color-field>, or<os-button>.Why?
When a modal opens, keyboard users and screen reader users expect to be trapped inside it - pressing Tab should cycle through the modal's controls and never reach anything behind it. This is called a focus trap.
The old focus trap worked fine for plain HTML elements (
<button>,<input>, etc.). But our design system wraps those controls inside custom web components (<os-*>), which use shadow DOM - a private, isolated subtree that the browser deliberately hides from normal DOM queries.The old code used this to find focusable elements:
querySelectorAllstops at shadow boundaries. So when<os-range-field>was slotted into the modal, its internal<input type="range">was invisible to the focus trap - it was never added to the Tab cycle, and focus was never moved there when the modal opened.Why this matters for accessibility
aria-modal="true"is already set on the element - this tells screen readers the dialog is modal. Without a working focus trap, the keyboard behaviour directly contradicts what those ARIA attributes promise.What Changed?
os-modal.tsAdded
collectFocusables()— recursive shadow DOM traversalquerySelectorAllwith a depth-first tree walk that crosses shadow boundaries (vianode.shadowRoot) and resolves<slot>elements usingslot.assignedElements({ flatten: true })<input>inside<os-range-field>,<button>inside<os-button>, etc.hiddenattribute oraria-hidden="true"— the two standard signals that a subtree is intentionally invisible to usersAdded
deepActiveElement()helperdocument.activeElementstops at the shadow host element (the custom element tag), not the actual focused control inside it_prevFocus(what gets focused when the modal closes) is always the real control, not an unfocusable wrapperAdded
eventSource()helperevent.composedPath()[0]is the correct way to find where a keyboard or mouse event really originated when shadow DOM is involvedImproved
_focusFirst()— smarter initial focus placement[autofocus]element first → first body control (skipping the close button) → close button as last resort_focusFirstcould run before the shadow root was ready and find nothingImproved Tab-key handler
looseguard: if the focused element is not in the tracked list (e.g. focus arrived via a click inside a shadow subtree), Tab still wraps correctly instead of doing nothingpreventDefault()-ed so it can't escape the modal entirelyFixed
FOCUSABLEselector:not([type="hidden"])to theinputpart of the selector<input type="hidden">) are not interactive and should never be part of a Tab cycleos-modal.test.ts5 new integration tests covering the fixed behaviour:
[autofocus]attribute is respected and takes priority<os-range-field>,<os-color-field>,<os-button>) are discovered and included in the Tab cycleAdded
tick()helper_focusFirstretries time to complete