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

Modern Event System: use focusin/focusout for onFocus/onBlur #19186

Merged
merged 1 commit into from Jul 16, 2020

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jun 24, 2020

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 and onBlur to focus and blur, 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 the document.

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 to focus and blur in the capture phase, they actually occur in reverse order, which can lead to inconsistent UIs if onFocus or onBlur are used like focusin and focusout work (which we have recommended users to do in the past).

This PR alters how the modern event system handles onFocus and onBlur so that they now use focusin and focusout rather than focus and blur. This means we no longer need to listen to these events in the capture phase, ensuring consistentcy when using onFocus and onBlur 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 and focusout.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 24, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 24, 2020

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:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Jun 24, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against e8205ae

@sizebot
Copy link

sizebot commented Jun 24, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against e8205ae

Copy link
Contributor

@bvaughn bvaughn left a 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.

@trueadm trueadm force-pushed the focusin-focusout branch 7 times, most recently from 1326bb2 to 04209cb Compare July 6, 2020 16:44
Copy link
Collaborator

@gaearon gaearon left a 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.

packages/react-dom/src/events/DOMEventProperties.js Outdated Show resolved Hide resolved
@trueadm trueadm force-pushed the focusin-focusout branch 9 times, most recently from 671aacc to ff22629 Compare July 7, 2020 01:37
@trueadm trueadm force-pushed the focusin-focusout branch 2 times, most recently from f709fb1 to a196777 Compare July 8, 2020 19:42
This was referenced Mar 15, 2021
adri1wald added a commit to adri1wald/slate that referenced this pull request Mar 31, 2022
See facebook/react#19186 for details on changes to `onFocus` and `onBlur`
dylans pushed a commit to ianstormtaylor/slate that referenced this pull request Apr 3, 2022
…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
thang2162 added a commit to thang2162/m3r that referenced this pull request May 24, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants