-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Modern Event System: use focusin/focusout for onFocus/onBlur #19186
Conversation
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. Latest deployment of this branch, based on commit e8205ae:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks okay, obviously barring the JSDOM issue.
packages/react-dom/src/events/plugins/ModernChangeEventPlugin.js
Outdated
Show resolved
Hide resolved
packages/react-dom/src/events/plugins/ModernChangeEventPlugin.js
Outdated
Show resolved
Hide resolved
1326bb2
to
04209cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok but I don't see why we don't remove TOP_FOCUS and TOP_BLUR altogether.
671aacc
to
ff22629
Compare
f709fb1
to
a196777
Compare
See facebook/react#19186 for details on changes to `onFocus` and `onBlur`
…lers in React >= 17 (#4920) * use focusin and focusout without capture if react >= 17 See facebook/react#19186 for details on changes to `onFocus` and `onBlur` * more accurate name for react version check const * add changeset * add comment about react >= 17 focus event listeners
Switched focus and blur out for focusin and focusout due to unintended side effect. React appears to have switch to using it even though it fires a warning. facebook/react#19186
React 17 switched to using `focusin`/`focusout` events for `onFocus`/`onBlur` [1]. floating-ui's focus manager appears to depend on this implicitly, causing focus to revert to body when tab-ing back into a floating portal with Reacht 16. Manually using `focusin`/`focusout` appears to fix the problem. [1] facebook/react#19186
Note: this PR requires changes in JSDOM, otherwise Jest tests fail. Everything passes when I make the JSDOM changes locally. This PR also includes #19221.
Before the changes in this PR, we mapped
onFocus
andonBlur
tofocus
andblur
, but we did so in the capture phase. Doing so in the capture phase enables us to listen to all events as if they bubbled, which works great when you listen on thedocument
.With the modern event system, we no longer listen on the
document
for events, but rather with listen to the React root (or portal) container element. This means that if we listen tofocus
andblur
in the capture phase, they actually occur in reverse order, which can lead to inconsistent UIs ifonFocus
oronBlur
are used likefocusin
andfocusout
work (which we have recommended users to do in the past).This PR alters how the modern event system handles
onFocus
andonBlur
so that they now usefocusin
andfocusout
rather thanfocus
andblur
. This means we no longer need to listen to these events in the capture phase, ensuring consistentcy when usingonFocus
andonBlur
with the expectation that bubbling order should correctly traverse through containers as expected.In terms of breaking changes, this change will mean that React will not support Firefox versions earlier than 52. Earlier versions do not support
focusin
andfocusout
.