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: make on*Capture events use capture phase #19221

Merged
merged 1 commit into from Jul 8, 2020

Conversation

@trueadm
Copy link
Member

@trueadm trueadm commented Jun 30, 2020

Note: this PR is rebased on #19244.

This PR alters the modern event system so that events that are of the format on*Capture (capture events) are actually put into the browser's native capture event phase when React creates the event listener. This is a deparature of the previous logic, which "virtualized" the capture and bubble phases within the native bubble event listener.

This is an important change to the modern event system, as the modern event system delegates to React roots (or portals) rather than the document. This means events that are setup to be capture listener, i.e. onClickCapture now correctly behave has expected. This also fixes some long-standing issues with how React and the browser differ in regards to how events are handled in co-ordination with native event listeners.

Lastly, we still (for now) enforce blur and focus to be capture phase listeners (so they emulate two phase propagation in the native capture phase, as we did before). We also emulate two phase propagation in the native bubble phase, as we did before, for the ChangeEventPlugin, BeforeInputEventPlugin and SelectEventPlugin event plugins.

@codesandbox
Copy link

@codesandbox codesandbox bot commented Jun 30, 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 d8e5480:

Sandbox Source
React Configuration
@sizebot
Copy link

@sizebot sizebot commented Jun 30, 2020

Details of bundled changes.

Comparing: 4eb9b1d...d8e5480

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.5% +0.5% 876.52 KB 880.72 KB 200.86 KB 201.78 KB NODE_DEV
ReactDOMForked-prod.js 🔺+0.7% 🔺+0.5% 401.14 KB 403.97 KB 74.25 KB 74.61 KB FB_WWW_PROD
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 118.53 KB 118.89 KB 38.09 KB 38.2 KB NODE_PROD
ReactDOMForked-profiling.js +0.7% +0.5% 411.74 KB 414.57 KB 76.02 KB 76.38 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 142.96 KB 142.96 KB 36.55 KB 36.55 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 20.22 KB 20.22 KB 7.59 KB 7.59 KB NODE_PROD
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.34 KB 10.34 KB 4.06 KB 4.06 KB UMD_PROD
ReactDOMTesting-dev.js +0.5% +0.5% 976.02 KB 980.5 KB 218.45 KB 219.44 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 51.13 KB 51.13 KB 14.89 KB 14.89 KB NODE_DEV
ReactDOMTesting-prod.js 🔺+0.7% 🔺+0.4% 409.12 KB 411.95 KB 76.95 KB 77.3 KB FB_WWW_PROD
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.19 KB 10.19 KB 3.98 KB 3.98 KB NODE_PROD
react-dom.development.js +0.5% +0.5% 920.89 KB 925.25 KB 203.23 KB 204.19 KB UMD_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.4% 118.44 KB 118.76 KB 38.82 KB 38.95 KB UMD_PROD
react-dom.profiling.min.js +0.3% +0.3% 122.34 KB 122.67 KB 40.01 KB 40.13 KB UMD_PROFILING
ReactDOMForked-dev.js +0.5% +0.4% 994.23 KB 998.72 KB 221.79 KB 222.77 KB FB_WWW_DEV
react-dom.profiling.min.js +0.3% +0.4% 122.59 KB 122.95 KB 39.26 KB 39.42 KB NODE_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 19.88 KB 19.88 KB 7.44 KB 7.45 KB UMD_PROD
ReactDOM-dev.js +0.4% +0.4% 1000.73 KB 1005.21 KB 223 KB 223.98 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.7% 🔺+0.5% 401.34 KB 404.17 KB 74.27 KB 74.63 KB FB_WWW_PROD
ReactDOM-profiling.js +0.7% +0.5% 411.94 KB 414.77 KB 76.04 KB 76.4 KB FB_WWW_PROFILING
ReactDOMServer-dev.js 0.0% 0.0% 146.66 KB 146.66 KB 37.25 KB 37.25 KB FB_WWW_DEV

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

Size changes (stable)

Generated by 🚫 dangerJS against d8e5480

@sizebot
Copy link

@sizebot sizebot commented Jun 30, 2020

Details of bundled changes.

Comparing: 4eb9b1d...d8e5480

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.5% +0.4% 908.83 KB 913.02 KB 206.67 KB 207.59 KB NODE_DEV
ReactDOMForked-prod.js 🔺+0.7% 🔺+0.5% 389.96 KB 392.79 KB 72.72 KB 73.1 KB FB_WWW_PROD
react-dom.production.min.js 🔺+0.3% 🔺+0.2% 123.26 KB 123.62 KB 39.53 KB 39.61 KB NODE_PROD
ReactDOMForked-profiling.js +0.7% +0.5% 400.49 KB 403.32 KB 74.47 KB 74.84 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 144.55 KB 144.55 KB 36.75 KB 36.75 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.35 KB 10.35 KB 4.07 KB 4.07 KB UMD_PROD
ReactDOMTesting-dev.js +0.5% +0.5% 947.98 KB 952.38 KB 212.57 KB 213.53 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 51.15 KB 51.15 KB 14.9 KB 14.9 KB NODE_DEV
ReactDOMTesting-prod.js 🔺+0.7% 🔺+0.5% 395.87 KB 398.65 KB 75.05 KB 75.4 KB FB_WWW_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 10.2 KB 10.2 KB 3.99 KB 3.99 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 5.36 KB 5.36 KB 1.8 KB 1.8 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.17 KB 1.17 KB 667 B 666 B NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.2 KB 1.2 KB 705 B 706 B UMD_PROD
react-dom.development.js +0.5% +0.5% 954.63 KB 958.99 KB 209.14 KB 210.15 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.01 KB 1.01 KB 616 B 617 B NODE_PROD
react-dom.production.min.js 🔺+0.3% 🔺+0.5% 123.1 KB 123.43 KB 40.23 KB 40.42 KB UMD_PROD
react-dom.profiling.min.js +0.3% +0.3% 127.05 KB 127.38 KB 41.49 KB 41.62 KB UMD_PROFILING
ReactDOMForked-dev.js +0.5% +0.5% 968.69 KB 973.17 KB 216.56 KB 217.57 KB FB_WWW_DEV
react-dom.profiling.min.js +0.3% +0.3% 127.38 KB 127.74 KB 40.76 KB 40.88 KB NODE_PROFILING
ReactDOM-dev.js +0.5% +0.5% 975.18 KB 979.67 KB 217.75 KB 218.77 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.7% 🔺+0.5% 390.16 KB 392.99 KB 72.74 KB 73.1 KB FB_WWW_PROD
ReactDOM-profiling.js +0.7% +0.5% 400.69 KB 403.52 KB 74.48 KB 74.85 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 20.26 KB 20.26 KB 7.51 KB 7.51 KB NODE_PROD
ReactDOMServer-dev.js 0.0% 0.0% 142.62 KB 142.62 KB 36.25 KB 36.25 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 46.58 KB 46.58 KB 10.9 KB 10.9 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 55.53 KB 55.53 KB 15.37 KB 15.37 KB UMD_DEV

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

Size changes (experimental)

Generated by 🚫 dangerJS against d8e5480

@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 7 times, most recently from 15ac292 to c1fac38 Jun 30, 2020
@trueadm trueadm marked this pull request as ready for review Jun 30, 2020
@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 2 times, most recently from ef10361 to 52b692b Jul 1, 2020
@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 2 times, most recently from 9768ac4 to 814b227 Jul 2, 2020
Copy link
Member

@gaearon gaearon left a comment

This seems to add a lot of Map access in hot paths. Is there any way at all to avoid that?

@trueadm
Copy link
Member Author

@trueadm trueadm commented Jul 6, 2020

@gaearon What Map access were you referencing specifically? I was also of the understanding that the difference was tiny, especially when the inline cache is utilized. I remember this being a perf issue a few years back with V8, but Sets and Maps are really fast now.

@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 5 times, most recently from 6dbd8ce to f888b6c Jul 6, 2020
@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 2 times, most recently from 71ec4d2 to 4fee2c9 Jul 6, 2020
WIP

WIP

FIX

FIX

Fix

WIP

Virtualize plugins

Fix lint

Clear up conflict

Clearn things up

Fix logic for plugins

Fix test

Simplify logic

Simplify logic

Optimize hot path

Revise
@trueadm trueadm force-pushed the trueadm:use-capture-phase-b branch 2 times, most recently from 4fee2c9 to e2f0a46 Jul 8, 2020
@gaearon
gaearon approved these changes Jul 8, 2020
Copy link
Member

@gaearon gaearon left a comment

This is probably a step in the right direction semantically but I'm getting really lost in the naming. I presume this is because it's a work in progress and we should be able to clean this up a bit more.

event: ReactSyntheticEvent,
capture: DispatchQueueItemPhase,
bubble: DispatchQueueItemPhase,
phase: DispatchQueueItemPhase,

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

I find the phase naming confusing both in this argument and in the property. When I read phase I think of 'capture' | 'bubble'. But it is actually more like an array of handlers. Can we rename this to listeners?

Same with the type name, I thought it refers to the string constants.

@@ -362,11 +399,19 @@ export function listenToReactPropEvent(
// this React prop event again.
listenerMap.set(reactPropEvent, null);
const dependencies = registrationNameDependencies[reactPropEvent];
const dependenciesLength = dependencies.length;
// If the dependencies length is 1, that means we're not using a polyfill

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

This heuristic feels fragile. Although I guess it makes sense.

const capture = capturePhaseEvents.has(dependency);

const capture =
capturePhaseEvents.has(dependency) || registrationCapturePhase;

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

We already calculated registrationCapturePhase so let's reorder so it goes first and we don't always have to check the Set?

@@ -348,6 +380,11 @@ export function listenToTopLevelEvent(
}
}

function isCaptureRegistrationName(registrationName: string): boolean {
const len = registrationName.length;
return registrationName.substr(len - 7) === 'Capture';

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

Nit: you can just write registrationName.substr(-7)

// SelectEventPlugin. SimpleEventPlugin always only has a single dependency.
// Given this, we know that we never need to apply capture phase event
// listeners to anything other than the SimpleEventPlugin.
const registrationCapturePhase =

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

Nit: can we rename this to better signal what it represents? Like isSomething. I wouldn't nitpick but I genuinely struggle to piece together what exactly this Boolean is telling us

@@ -667,32 +716,35 @@ export function accumulateTwoPhaseListeners(
const listener = listenersArr[i];
const {callback, capture, type} = listener;
if (type === targetType) {
if (capture === true) {
capturePhase.push(
if (capture && inCapturePhase) {

This comment has been minimized.

@gaearon

gaearon Jul 8, 2020
Member

I'm really struggling to follow what's happening here due to very similar naming. E.g. capture, captured, and inCapturePhase probably represent something different but I'm getting lost which is which because they're all used in the same function.

@trueadm
Copy link
Member Author

@trueadm trueadm commented Jul 8, 2020

I'll merge this PR and address the concerns in a follow up, as much of what I've done there already fixes the comments from Dan.

@trueadm trueadm merged commit f5ea39c into facebook:master Jul 8, 2020
32 checks passed
32 checks passed
Facebook CLA Check Contributor License Agreement is valid!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_persistent Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: sizebot_stable Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.