Skip to content

fix(useFocusVisible): use Reflect.defineProperty to wrap HTMLElement.prototype.focus#10041

Open
antododo wants to merge 2 commits into
adobe:mainfrom
antododo:fix/use-focus-visible-define-property
Open

fix(useFocusVisible): use Reflect.defineProperty to wrap HTMLElement.prototype.focus#10041
antododo wants to merge 2 commits into
adobe:mainfrom
antododo:fix/use-focus-visible-define-property

Conversation

@antododo
Copy link
Copy Markdown

@antododo antododo commented May 11, 2026

Closes #9649

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. yarn jest packages/react-aria/test/interactions/useFocusVisible.test.js — all 18 tests pass, including the new regression test does not throw when HTMLElement.prototype.focus is an accessor-only property.
  2. Confirm the regression: temporarily revert just the src/ change, keep the new test, and rerun the command above. The new test should fail with TypeError: Cannot set property focus of [object HTMLElement] which has only a getter. Re-apply the src/ change and the test passes.
  3. Smoke-test the wider interactions suite to confirm no regression: yarn jest packages/react-aria/test/interactions/ — all 237 tests pass.
  4. In a real browser, nothing should change — focus rings, keyboard vs. pointer modality, and screen-reader virtual focus detection behave identically (the Reflect.defineProperty call is equivalent to the previous assignment against the default HTMLElement.prototype.focus data descriptor).

🧢 Your Project:

While building components on react-aria-components for Swissquote.ch internal design system where consumers' Jest+JSDOM test suites using @testing-library/user-event's setup() crash on import of any RAC-using component.

@antododo antododo force-pushed the fix/use-focus-visible-define-property branch from 1300068 to 6fbb592 Compare May 11, 2026 10:13
@nwidynski
Copy link
Copy Markdown
Contributor

At this point, why override at all instead of using an apply trap?

@antododo
Copy link
Copy Markdown
Author

antododo commented May 22, 2026

At this point, why override at all instead of using an apply trap?

@nwidynski The bug is the install mechanism (getter-only focus accessor breaks plain assignment), not the wrapper shape, so a Proxy would still need the same defineProperty + try/catch. Happy to switch if you see a concrete benefit.

@nwidynski
Copy link
Copy Markdown
Contributor

@antododo Not with a Proxy, but via Reflect API. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/defineProperty

That wont throw when failing to install the override.

@antododo
Copy link
Copy Markdown
Author

@nwidynski thanks for the suggestion, PR updated.

@reidbarber reidbarber changed the title fix(useFocusVisible): use Object.defineProperty to wrap HTMLElement.prototype.focus fix(useFocusVisible): use Reflect.defineProperty to wrap HTMLElement.prototype.focus May 22, 2026
Copy link
Copy Markdown
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

I'm fine with this, hopefully userEvent gets their act together soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useFocusVisible crashes in tests with @testing-library/user-event + Vitest/jsdom

3 participants