Skip to content

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

Merged
cixzhang merged 1 commit into
facebook:mainfrom
bhamodi:a11y/focusable-selector-href
Jul 10, 2026
Merged

fix(core): restrict focusableSelector href matching to real links#3714
cixzhang merged 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

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bhamodi — nicely scoped selector narrowing; a[href]/area[href] are the only focusable-via-href elements, nothing legit dropped. 🙏

@cixzhang cixzhang merged commit 48e4426 into facebook:main Jul 10, 2026
14 of 15 checks passed
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.

2 participants