Skip to content

fix(os-modal): repair focus trap for shadow-DOM slotted components - #905

Open
KarunyaChavan wants to merge 6 commits into
WordPress:trunkfrom
KarunyaChavan:fix/os-modal-focus-trap-shadow-dom
Open

KarunyaChavan wants to merge 6 commits into
WordPress:trunkfrom
KarunyaChavan:fix/os-modal-focus-trap-shadow-dom

Conversation

@KarunyaChavan

@KarunyaChavan KarunyaChavan commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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:

// Before — cannot see inside shadow DOM of slotted custom elements
this.querySelectorAll( FOCUSABLE )

querySelectorAll stops 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

  • WCAG 2.1 SC 2.1.2 - No Keyboard Trap (reference): Users must be able to move focus into and within any UI component using only the keyboard. A modal where focus never lands correctly fails this.
  • ARIA Authoring Practices Guide - Dialog (Modal) Pattern (reference): The APG requires that when a dialog opens, focus moves to the first focusable element inside it, and Tab / Shift+Tab cycle only within the dialog until it is closed.
  • 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.ts

  • Added collectFocusables() — recursive shadow DOM traversal

    • Replaces the old flat querySelectorAll with a depth-first tree walk that crosses shadow boundaries (via node.shadowRoot) and resolves <slot> elements using slot.assignedElements({ flatten: true })
    • Correctly finds <input> inside <os-range-field>, <button> inside <os-button>, etc.
    • Subtree pruning stops only at elements with the hidden attribute or aria-hidden="true" — the two standard signals that a subtree is intentionally invisible to users
  • Added deepActiveElement() helper

    • document.activeElement stops at the shadow host element (the custom element tag), not the actual focused control inside it
    • This helper walks shadow boundaries to find the genuinely focused element, so _prevFocus (what gets focused when the modal closes) is always the real control, not an unfocusable wrapper
  • Added eventSource() helper

    • event.composedPath()[0] is the correct way to find where a keyboard or mouse event really originated when shadow DOM is involved
    • Used in both Tab-key wrapping and backdrop-click detection, replacing a pattern that was duplicated in two places
  • Improved _focusFirst() — smarter initial focus placement

    • On open, focus now lands on: [autofocus] element first → first body control (skipping the close button) → close button as last resort
    • Includes a microtask retry loop (up to 5 tries) because shadow DOM components render asynchronously — without this, _focusFirst could run before the shadow root was ready and find nothing
    • Guards against running after the modal has already been closed (race condition fix)
  • Improved Tab-key handler

    • Added a loose guard: 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 nothing
    • When there are zero focusable elements, Tab is now preventDefault()-ed so it can't escape the modal entirely
  • Fixed FOCUSABLE selector

    • Added :not([type="hidden"]) to the input part of the selector
    • Hidden inputs (<input type="hidden">) are not interactive and should never be part of a Tab cycle

os-modal.test.ts

  • 5 new integration tests covering the fixed behaviour:

    • Focus moves to the first body element (not the close button) on open
    • [autofocus] attribute is respected and takes priority
    • Focusable elements inside slotted shadow-DOM components (<os-range-field>, <os-color-field>, <os-button>) are discovered and included in the Tab cycle
    • Tab wraps from the last element back to the first; Shift+Tab wraps from the first to the last
    • Closing the modal restores focus to whatever the user was focused on before the modal opened
  • Added tick() helper

    • A named 10 ms settle used before asserting focus state, giving shadow DOM components time to render and _focusFirst retries time to complete
Open WordPress Playground Preview

KarunyaChavan and others added 5 commits September 24, 2026 14:55
- 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
KarunyaChavan marked this pull request as ready for review September 25, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modal focus trap fails to loop when interacting with custom components

1 participant