Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow reassignment of focus and blur methods on `HTMLElement… #1265

Merged

Conversation

jakeboone02
Copy link
Contributor

What

Reinstates the ability to reassign HTMLElement.prototype.focus and HTMLElement.prototype.blur that was removed in #1252.

Why

Some UI libraries (Chakra UI, for example) override the focus and/or blur methods on the HTMLElement prototype. #1252 introduced a patched focus implementation that only included a getter, which means these libraries error when run with @testing-library/user-event because there is no setter.

How

This adds a setter to the patched focus and blur methods.

Checklist

  • Tests
  • Ready to be merged

I'm not super confident that this is the correct or preferred way to resolve the issue, but it works for my use cases.

This line from @zag-js/radio-group (a dependency of @chakra-ui/react) was the operative code in my case: https://github.com/chakra-ui/zag/blob/c1ffbfe1eb09028c837b305942f34bb77b9ac4fc/packages/utilities/focus-visible/src/index.ts#L143

Copy link

codesandbox-ci bot commented Jan 24, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@mwdiaz
Copy link

mwdiaz commented Jan 28, 2025

I ran into the same issue when trying to upgrade @testing-library/user-event in a project that uses React Aria: https://github.com/adobe/react-spectrum/blob/fd7075c5f0e06998ea8ac6a341dee165919fb6c1/packages/%40react-aria/interactions/src/useFocusVisible.ts#L133-L141

Copy link
Member

@ph-fritsche ph-fritsche left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to this library.

Replacing the accessor descriptor with a data descriptor removes the extra logic.
IMHO this also makes the test redundant.

Supporting the reassignment would also require us to expose proper setup and teardown, as the linked teardown function in Chakra-UI would break our implementation if their setup function runs before our setup.
But this is out of scope here – I will tackle this in a separate PR.

src/document/patchFocus.ts Outdated Show resolved Hide resolved
src/document/patchFocus.ts Outdated Show resolved Hide resolved
src/document/patchFocus.ts Outdated Show resolved Hide resolved
@jakeboone02
Copy link
Contributor Author

Thanks for the comments, @ph-fritsche. I made the requested changes and removed the test.

@ph-fritsche ph-fritsche merged commit 63ac399 into testing-library:main Feb 5, 2025
3 checks passed
@jakeboone02 jakeboone02 deleted the allow-custom-focus-blur branch February 5, 2025 09:21
@Averethel
Copy link

@ph-fritsche can this fix get released soon?

@ph-fritsche
Copy link
Member

@Averethel

Supporting the reassignment would also require us to expose proper setup and teardown, as the linked teardown function in Chakra-UI would break our implementation if their setup function runs before our setup.

#1270 should fix that but I didn't have time to check yet

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.

4 participants