From 06d104e8ec89df4bc5176e014c83e8b6393e555f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 27 Jul 2020 17:33:54 +0100 Subject: [PATCH] Don't emulate bubbling of the scroll event (#19464) * Don't emulate bubbling of the scroll event * Put behind a flag --- .../__tests__/ReactDOMEventListener-test.js | 114 ++++++++++++++++-- .../src/events/plugins/SimpleEventPlugin.js | 24 ++-- packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 11 files changed, 132 insertions(+), 16 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 3733005fb223..ec94ccc6721a 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -539,7 +539,6 @@ describe('ReactDOMEventListener', () => { const container = document.createElement('div'); const ref = React.createRef(); const onPlay = jest.fn(); - const onScroll = jest.fn(); const onCancel = jest.fn(); const onClose = jest.fn(); const onToggle = jest.fn(); @@ -548,14 +547,12 @@ describe('ReactDOMEventListener', () => { ReactDOM.render(
{ bubbles: false, }), ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); ref.current.dispatchEvent( new Event('cancel', { bubbles: false, @@ -591,7 +583,6 @@ describe('ReactDOMEventListener', () => { // Regression test: ensure we still emulate bubbling with non-bubbling // media expect(onPlay).toHaveBeenCalledTimes(2); - expect(onScroll).toHaveBeenCalledTimes(2); expect(onCancel).toHaveBeenCalledTimes(2); expect(onClose).toHaveBeenCalledTimes(2); expect(onToggle).toHaveBeenCalledTimes(2); @@ -643,4 +634,109 @@ describe('ReactDOMEventListener', () => { document.body.removeChild(container); } }); + + // We're moving towards aligning more closely with the browser. + // Currently we emulate bubbling for all non-bubbling events except scroll. + // We may expand this list in the future, removing emulated bubbling altogether. + it('should not emulate bubbling of scroll events', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + const log = []; + const onScroll = jest.fn(e => + log.push(['bubble', e.currentTarget.className]), + ); + const onScrollCapture = jest.fn(e => + log.push(['capture', e.currentTarget.className]), + ); + document.body.appendChild(container); + try { + ReactDOM.render( +
+
+
+
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + if (gate(flags => flags.disableOnScrollBubbling)) { + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ['capture', 'child'], + ['bubble', 'child'], + ]); + } else { + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ['capture', 'child'], + ['bubble', 'child'], + ['bubble', 'parent'], + ['bubble', 'grand'], + ]); + } + } finally { + document.body.removeChild(container); + } + }); + + // We're moving towards aligning more closely with the browser. + // Currently we emulate bubbling for all non-bubbling events except scroll. + // We may expand this list in the future, removing emulated bubbling altogether. + it('should not emulate bubbling of scroll events (no own handler)', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + const log = []; + const onScroll = jest.fn(e => + log.push(['bubble', e.currentTarget.className]), + ); + const onScrollCapture = jest.fn(e => + log.push(['capture', e.currentTarget.className]), + ); + document.body.appendChild(container); + try { + ReactDOM.render( +
+
+ {/* Intentionally no handler on the child: */} +
+
+
, + container, + ); + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ]); + } finally { + document.body.removeChild(container); + } + }); }); diff --git a/packages/react-dom/src/events/plugins/SimpleEventPlugin.js b/packages/react-dom/src/events/plugins/SimpleEventPlugin.js index 281065828322..39b74059dc48 100644 --- a/packages/react-dom/src/events/plugins/SimpleEventPlugin.js +++ b/packages/react-dom/src/events/plugins/SimpleEventPlugin.js @@ -42,7 +42,10 @@ import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../EventSystemFlags'; import getEventCharCode from '../getEventCharCode'; import {IS_CAPTURE_PHASE} from '../EventSystemFlags'; -import {enableCreateEventHandleAPI} from 'shared/ReactFeatureFlags'; +import { + enableCreateEventHandleAPI, + disableOnScrollBubbling, +} from 'shared/ReactFeatureFlags'; function extractEvents( dispatchQueue: DispatchQueue, @@ -165,13 +168,20 @@ function extractEvents( inCapturePhase, ); } else { - // TODO: We may also want to re-use the accumulateTargetOnly flag to - // special case bubbling for onScroll/media events at a later point. - // In which case we will want to make this flag boolean and ensure - // we change the targetInst to be of the container instance. Like: - const accumulateTargetOnly = false; + // Some events don't bubble in the browser. + // In the past, React has always bubbled them, but this can be surprising. + // We're going to try aligning closer to the browser behavior by not bubbling + // them in React either. We'll start by not bubbling onScroll, and then expand. + let accumulateTargetOnly = false; + if (disableOnScrollBubbling) { + accumulateTargetOnly = + !inCapturePhase && + // TODO: ideally, we'd eventually add all events from + // nonDelegatedEvents list in DOMPluginEventSystem. + // Then we can remove this special list. + topLevelType === DOMTopLevelEventTypes.TOP_SCROLL; + } - // We traverse only capture or bubble phase listeners accumulateSinglePhaseListeners( targetInst, dispatchQueue, diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index e0c38e9baaaf..51d427defb6e 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -128,3 +128,5 @@ export const deferRenderPhaseUpdateToNextBatch = true; // Replacement for runWithPriority in React internals. export const decoupleUpdatePriorityFromScheduler = false; + +export const disableOnScrollBubbling = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 14180245136c..bbb44c17ec36 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -45,6 +45,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index e1f4100fdaaa..8d299fd6bcbe 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 2c94f623ef3a..5be95d45a413 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index eeb6866fc968..95dc4830d032 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 56e3f0284d9e..2784962087c0 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 414f5db9b0b6..bcfb78a80324 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; +export const disableOnScrollBubbling = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 201a40fdbfa5..e016a0f07156 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -18,6 +18,7 @@ export const disableInputAttributeSyncing = __VARIANT__; export const enableFilterEmptyStringAttributesDOM = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; export const decoupleUpdatePriorityFromScheduler = __VARIANT__; +export const disableOnScrollBubbling = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 6f0d031aff51..8f919e0cc950 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -27,6 +27,7 @@ export const { decoupleUpdatePriorityFromScheduler, enableDebugTracing, enableSchedulingProfilerComponentStacks, + disableOnScrollBubbling, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.