Skip to content

fix: onFocus/onBlur/onBeforeInput have a matching event type#19561

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

Merged
merged 8 commits into from
Aug 10, 2020

Conversation

eps1lon
Copy link
Collaborator

@eps1loneps1lon commented Aug 8, 2020

Summary

So far event.type always matched the listener name in React even though the underlying native event might be a different type (e.g. onMouseEnter). The recent changes to focus events made onFocus and onBlur the only two events whose event.type did not match (onFocus -> focusin, onBlur -> focusout).

This PR restores the previous event types.

Closes#19560

Test Plan

  • CI green
  • observe expected behavior codesandbox in original issue

It might make sense to write a regression test suite (similar to #19483) for all event types since it seems that event.type isn't tested. I could split this work up into two PRs (one for the test, one for the fix) in case you need to revert the fix.

@eps1loneps1lon changed the title Fix/focus event typefix: onFocus/onBlur have a matching event typeAug 8, 2020
@codesandbox-ci
Copy link

codesandbox-cibot commented Aug 8, 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 30c564b:

SandboxSource
ReactConfiguration
event.type in react@nextIssue #19560
@eps1loneps1lon marked this pull request as draft August 8, 2020 08:01
@sizebot
Copy link

sizebot commented Aug 8, 2020

Details of bundled changes.

Comparing: 0cd9a6d...30c564b

react-dom

FileFilesize DiffGzip DiffPrev SizeCurrent SizePrev GzipCurrent GzipENV
react-dom.development.js0.0%0.0%912.3 KB912.53 KB207.26 KB207.29 KBNODE_DEV
ReactDOMForked-prod.js🔺+0.1%🔺+0.1%379.41 KB379.96 KB70.05 KB70.12 KBFB_WWW_PROD
react-dom-server.node.development.js0.0%0.0%138.51 KB138.51 KB36.61 KB36.61 KBNODE_DEV
react-dom.production.min.js0.0%0.0%123.84 KB123.88 KB39.67 KB39.67 KBNODE_PROD
ReactDOMForked-profiling.js+0.1%+0.1%394.32 KB394.88 KB72.78 KB72.84 KBFB_WWW_PROFILING
react-dom-server.browser.development.js0.0%0.0%144.69 KB144.69 KB36.8 KB36.8 KBUMD_DEV
react-dom-server.node.production.min.js0.0%0.0%20.66 KB20.66 KB7.65 KB7.66 KBNODE_PROD
react-dom-test-utils.production.min.js🔺+0.1%🔺+0.1%12.86 KB12.87 KB5.04 KB5.05 KBUMD_PROD
ReactDOMTesting-dev.js0.0%0.0%920.55 KB920.85 KB207.05 KB207.09 KBFB_WWW_DEV
react-dom-test-utils.development.js+0.1%+0.1%60.81 KB60.86 KB17.63 KB17.65 KBNODE_DEV
ReactDOMTesting-prod.js🔺+0.1%🔺+0.1%378.57 KB379.13 KB71.2 KB71.26 KBFB_WWW_PROD
react-dom-unstable-fizz.node.development.js0.0%+0.1%5.52 KB5.52 KB1.84 KB1.84 KBNODE_DEV
react-dom-test-utils.production.min.js🔺+0.1%0.0%12.71 KB12.72 KB4.97 KB4.97 KBNODE_PROD
react-dom-unstable-fizz.browser.development.js0.0%+0.3%5.27 KB5.27 KB1.78 KB1.78 KBUMD_DEV
react-dom-unstable-fizz.browser.production.min.js0.0%🔺+0.1%1.2 KB1.2 KB704 B705 BUMD_PROD
ReactTestUtils-dev.js+0.1%+0.1%55.13 KB55.21 KB15.46 KB15.48 KBFB_WWW_DEV
react-dom-unstable-fizz.browser.development.js0.0%+0.1%4.78 KB4.78 KB1.68 KB1.68 KBNODE_DEV
react-dom.development.js0.0%0.0%958.48 KB958.72 KB209.8 KB209.84 KBUMD_DEV
react-dom-unstable-fizz.browser.production.min.js0.0%🔺+0.2%1.01 KB1.01 KB615 B616 BNODE_PROD
react-dom.production.min.js0.0%0.0%123.72 KB123.76 KB40.37 KB40.39 KBUMD_PROD
react-dom.profiling.min.js0.0%+0.1%128.9 KB128.95 KB41.98 KB42.02 KBUMD_PROFILING
ReactDOMForked-dev.js0.0%0.0%967.15 KB967.44 KB215.22 KB215.25 KBFB_WWW_DEV
react-dom.profiling.min.js0.0%-0.0%129.22 KB129.26 KB41.26 KB41.26 KBNODE_PROFILING
react-dom-server.browser.production.min.js0.0%0.0%20.33 KB20.33 KB7.54 KB7.54 KBUMD_PROD
ReactDOM-dev.js0.0%0.0%960.24 KB960.54 KB214.31 KB214.34 KBFB_WWW_DEV
ReactDOM-prod.js🔺+0.1%🔺+0.1%376.43 KB376.99 KB69.46 KB69.52 KBFB_WWW_PROD
react-dom-server.browser.development.js0.0%0.0%137.24 KB137.24 KB36.36 KB36.36 KBNODE_DEV
ReactDOM-profiling.js+0.1%+0.1%390.48 KB391.04 KB72.1 KB72.16 KBFB_WWW_PROFILING
react-dom-server.browser.production.min.js0.0%0.0%20.24 KB20.24 KB7.5 KB7.5 KBNODE_PROD
ReactDOMServer-dev.js0.0%0.0%142.94 KB142.94 KB36.34 KB36.34 KBFB_WWW_DEV
ReactDOMServer-prod.js0.0%0.0%46.44 KB46.44 KB10.83 KB10.83 KBFB_WWW_PROD
react-dom-test-utils.development.js+0.1%+0.1%65.82 KB65.88 KB18.16 KB18.18 KBUMD_DEV

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Size changes (experimental)

Generated by 🚫 dangerJS against 30c564b

@@ -36,7 +36,7 @@ function initializeModules(hasPointerEvents) {
const forcePointerEvents = true;
const table = [[forcePointerEvents], [!forcePointerEvents]];

describe.each(table)(`useFocus`, hasPointerEvents => {
describe.each(table)(`useFocus hasPointerEvents=%s`, hasPointerEvents => {
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

was slightly confusing why we had two tests with the same name.

@sizebot
Copy link

sizebot commented Aug 8, 2020

Details of bundled changes.

Comparing: 0cd9a6d...30c564b

react-dom

FileFilesize DiffGzip DiffPrev SizeCurrent SizePrev GzipCurrent GzipENV
react-dom.development.js0.0%0.0%876.49 KB876.72 KB200.71 KB200.74 KBNODE_DEV
ReactDOMForked-prod.js🔺+0.1%🔺+0.1%390.55 KB391.11 KB71.8 KB71.87 KBFB_WWW_PROD
react-dom-server.node.development.js0.0%0.0%137 KB137 KB36.4 KB36.4 KBNODE_DEV
react-dom.production.min.js0.0%0.0%119.23 KB119.27 KB38.31 KB38.31 KBNODE_PROD
ReactDOMForked-profiling.js+0.1%+0.1%405.53 KB406.09 KB74.52 KB74.59 KBFB_WWW_PROFILING
react-dom-server.browser.development.js0.0%0.0%143.1 KB143.1 KB36.6 KB36.61 KBUMD_DEV
react-dom-server.node.production.min.js0.0%0.0%20.2 KB20.2 KB7.58 KB7.58 KBNODE_PROD
react-dom-test-utils.production.min.js🔺+0.1%🔺+0.1%12.84 KB12.86 KB5.03 KB5.04 KBUMD_PROD
ReactDOMTesting-dev.js0.0%0.0%948.61 KB948.9 KB212.55 KB212.61 KBFB_WWW_DEV
react-dom-test-utils.development.js+0.1%+0.1%60.79 KB60.85 KB17.63 KB17.64 KBNODE_DEV
ReactDOMTesting-prod.js🔺+0.1%🔺+0.1%391.2 KB391.76 KB73.29 KB73.34 KBFB_WWW_PROD
react-dom-test-utils.production.min.js🔺+0.1%0.0%12.7 KB12.71 KB4.96 KB4.96 KBNODE_PROD
ReactTestUtils-dev.js+0.1%+0.1%55.13 KB55.21 KB15.46 KB15.48 KBFB_WWW_DEV
react-dom.development.js0.0%0.0%920.92 KB921.16 KB203.21 KB203.25 KBUMD_DEV
react-dom.production.min.js0.0%🔺+0.1%119.17 KB119.22 KB38.94 KB38.96 KBUMD_PROD
react-dom.profiling.min.js0.0%+0.1%123.07 KB123.11 KB40.16 KB40.19 KBUMD_PROFILING
ReactDOMForked-dev.js0.0%0.0%992.64 KB992.93 KB219.99 KB220.04 KBFB_WWW_DEV
react-dom.profiling.min.js0.0%0.0%123.3 KB123.34 KB39.55 KB39.57 KBNODE_PROFILING
react-dom-server.browser.production.min.js0.0%0.0%19.87 KB19.87 KB7.45 KB7.45 KBUMD_PROD
ReactDOM-dev.js0.0%0.0%985.73 KB986.02 KB218.99 KB219.05 KBFB_WWW_DEV
ReactDOM-prod.js🔺+0.1%🔺+0.1%387.58 KB388.14 KB71.16 KB71.23 KBFB_WWW_PROD
react-dom-server.browser.development.js0.0%0.0%135.73 KB135.73 KB36.15 KB36.15 KBNODE_DEV
ReactDOM-profiling.js+0.1%+0.1%401.69 KB402.25 KB73.79 KB73.86 KBFB_WWW_PROFILING
react-dom-server.browser.production.min.js0.0%0.0%19.78 KB19.78 KB7.42 KB7.42 KBNODE_PROD
ReactDOMServer-dev.js0.0%0.0%146.97 KB146.97 KB37.34 KB37.34 KBFB_WWW_DEV
ReactDOMServer-prod.js0.0%0.0%47.3 KB47.3 KB11.04 KB11.04 KBFB_WWW_PROD
react-dom-test-utils.development.js+0.1%+0.1%65.81 KB65.86 KB18.15 KB18.17 KBUMD_DEV

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Size changes (stable)

Generated by 🚫 dangerJS against 30c564b

@eps1loneps1lon marked this pull request as ready for review August 8, 2020 08:59
@eps1loneps1lonforce-pushed the fix/focus-event-type branch from ff641a5 to cc2ae0cCompareAugust 8, 2020 09:59
@gaearon
Copy link
Collaborator

Hmm. I was hoping we could do this with a bit less code than creating two more functions etc. Any other ideas?

@eps1lon
Copy link
CollaboratorAuthor

Hmm. I was hoping we could do this with a bit less code than creating two more functions etc. Any other ideas?

  • Go the same route as the EnterLeaveEventPlugin
    This is probably even more code.
  • always set event.type = reactName.slice(2).toLowerCase()
    We're back to the original approach where we do unnecessary work for almost all events.

In the end we had one component that relied on event.type and we should be able to refactor that code to use two different event handlers instead of one that branches based on event.type. Since it seems like you're preparing a major release it's somewhat safe to do. Maybe wait a bit to see if others have feedback integrating with react@next?

@gaearongaearonforce-pushed the fix/focus-event-type branch from f98cb9b to 30c564bCompareAugust 10, 2020 11:42
@gaearon
Copy link
Collaborator

I pushed a different fix. We already have a switch so we can just reuse it to set the type. In fact we can make type an argument to SyntheticEvent and always force plugins to pass it. This actually exposes an issue in onBeforeInput which has always had the same bug. Since it's inconsistent (and I couldn't find any place at FB that relies on this), I patched it too.

Added a regression suite.

@gaearongaearon changed the title fix: onFocus/onBlur have a matching event typefix: onFocus/onBlur/onBeforeInput have a matching event typeAug 10, 2020
@gaearongaearon merged commit 7f696bd into facebook:masterAug 10, 2020
@eps1loneps1lon deleted the fix/focus-event-type branch August 10, 2020 11:59
This was referenced Mar 15, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants
@eps1lon@sizebot@gaearon@trueadm@facebook-github-bot
close