Skip to content

fix(core): restrict focusableSelector href matching to real links#3714

Open
bhamodi wants to merge 1 commit into
facebook:mainfrom
bhamodi:a11y/focusable-selector-href
Open

fix(core): restrict focusableSelector href matching to real links#3714
bhamodi wants to merge 1 commit into
facebook:mainfrom
bhamodi:a11y/focusable-selector-href

Conversation

@bhamodi

@bhamodi bhamodi commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

FOCUSABLE_SELECTOR (packages/core/src/hooks/focusableSelector.ts:25) built the canonical focusable-elements selector with a bare [href] term. That matches any element carrying an href attribute — a <link> in the head, a <span href>, or a custom element — not just focusable links. Only <a href> and <area href> are actually focusable via href.

useFocusTrap feeds this selector straight into querySelectorAll to compute trap boundaries (the first/last tab stop). A non-focusable element carrying href could therefore be picked as the first or last "tab stop", breaking Tab-wrap inside a trap — the boundary element never receives focus, so the wrap condition never fires.

This narrows the term to a[href], area[href]. The rest of the selector is unchanged.

Changes

  • packages/core/src/hooks/focusableSelector.ts: [href]a[href], area[href] in FOCUSABLE_SELECTOR. The doc-comment describes this term as "link", which stays accurate.
  • packages/core/src/hooks/useFocusTrap.test.tsx: added a FOCUSABLE_SELECTOR href matching block — asserts the selector matches <a href>/<area href> but excludes <span href> and <link href>, plus an end-to-end check that focusFirst lands on an <a href> inside a trap.

Test plan

  • node_modules/.bin/vitest run --root . packages/core/src/hooks118 pass (12 files), including the 2 new tests. useFocusTrap.test.tsx now has 8 tests (was 6). This is the only importer of focusableSelector, and its tests live in this directory.
  • Confirmed failing-first: temporarily reverting the selector to [href] and running the new test fails on expect(matches).not.toContain(span), proving it catches the regression.
  • tsc --project packages/core/tsconfig.json --noEmit — clean.
  • eslint on both changed files — clean.

Notes

Found during a broader a11y audit of core hooks. Scoped to this one selector fix — related findings (e.g. excluding aria-disabled elements) are intentionally left out.

@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown

@bhamodi is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 9, 2026
@bhamodi bhamodi force-pushed the a11y/focusable-selector-href branch from c150ee0 to 83cbb9f Compare July 9, 2026 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant