From 5a38bac3fd48713fb660723a279fd3d21d081878 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 27 Jul 2020 16:03:44 +0100 Subject: [PATCH] Don't emulate bubbling of the scroll event --- .../__tests__/ReactDOMEventListener-test.js | 110 +++++++++++++++--- .../src/events/plugins/SimpleEventPlugin.js | 16 ++- 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 86ddbbff30025..a1d8453e2b912 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -539,21 +539,15 @@ 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(); document.body.appendChild(container); try { ReactDOM.render( -
+
@@ -565,11 +559,6 @@ describe('ReactDOMEventListener', () => { bubbles: false, }), ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); ref.current.dispatchEvent( new Event('cancel', { bubbles: false, @@ -583,7 +572,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); } finally { @@ -634,4 +622,100 @@ 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, + }), + ); + // + expect(log).toEqual([ + ['capture', 'grand'], + ['capture', 'parent'], + ['capture', 'child'], + ['bubble', 'child'], + ]); + } 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 281065828322a..0bcb4dc2845c6 100644 --- a/packages/react-dom/src/events/plugins/SimpleEventPlugin.js +++ b/packages/react-dom/src/events/plugins/SimpleEventPlugin.js @@ -165,13 +165,17 @@ 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. + const accumulateTargetOnly = + !inCapturePhase && + // TODO: ideally, we'd eventually add all events from + // nonDelegatedEvents list in ReactDOMEventListener. + // Then we can remove this special list. + topLevelType === DOMTopLevelEventTypes.TOP_SCROLL; - // We traverse only capture or bubble phase listeners accumulateSinglePhaseListeners( targetInst, dispatchQueue,