-
Notifications
You must be signed in to change notification settings - Fork 50k
Modern Event System: use focusin/focusout for onFocus/onBlur #19186
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
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:
|
bvaughn
left a comment
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
gaearon
left a comment
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
onFocusandonBlurtofocusandblur, 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
documentfor events, but rather with listen to the React root (or portal) container element. This means that if we listen tofocusandblurin the capture phase, they actually occur in reverse order, which can lead to inconsistent UIs ifonFocusoronBlurare used likefocusinandfocusoutwork (which we have recommended users to do in the past).This PR alters how the modern event system handles
onFocusandonBlurso that they now usefocusinandfocusoutrather thanfocusandblur. This means we no longer need to listen to these events in the capture phase, ensuring consistentcy when usingonFocusandonBlurwith 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
focusinandfocusout.