From 33665fdfc4c455e8957c8d1f73f01c5108104e95 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 27 Jul 2020 16:57:08 +0100 Subject: [PATCH 1/3] Bail-out of attaching non-delegated listeners Revise comment --- .../react-dom/src/events/DOMPluginEventSystem.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/react-dom/src/events/DOMPluginEventSystem.js b/packages/react-dom/src/events/DOMPluginEventSystem.js index c70a00607eeb..f75f32b8a3f1 100644 --- a/packages/react-dom/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMPluginEventSystem.js @@ -384,6 +384,18 @@ export function listenToNativeEvent( !isCapturePhaseListener && nonDelegatedEvents.has(topLevelType) ) { + // For all non-delegated events, apart from scroll, we attach + // their event listeners to the respective elements that their + // events fire on. That means we can skip this step, as event + // listener has already been added previously. However, we + // special case the scroll event because the reality is that any + // element can scroll. + // TODO: ideally, we'd eventually apply the same logic to all + // events from the nonDelegatedEvents list. Then we can remove + // this special case and use the same logic for all events. + if (topLevelType !== TOP_SCROLL) { + return; + } eventSystemFlags |= IS_NON_DELEGATED; target = targetElement; } From 51f08fd33dc525a69ef163b6ad7e9bfb142b9a0c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 27 Jul 2020 17:10:14 +0100 Subject: [PATCH 2/3] Fix tests/add tests --- .../__tests__/ReactDOMEventListener-test.js | 73 +++++++++++-------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index ec94ccc6721a..f2ba43cb5926 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -348,12 +348,7 @@ describe('ReactDOMEventListener', () => { bubbles: false, }), ); - // As of the modern event system refactor, we now support - // this on . The reason for this, is because we allow - // events to be attached to nodes regardless of if they - // necessary support them. This is a strange test, as this - // would never occur from normal browser behavior. - expect(handleImgLoadStart).toHaveBeenCalledTimes(1); + expect(handleImgLoadStart).toHaveBeenCalledTimes(0); videoRef.current.dispatchEvent( new ProgressEvent('loadstart', { @@ -535,36 +530,42 @@ describe('ReactDOMEventListener', () => { } }); - it('should bubble non-native bubbling events', () => { + it('should bubble non-native bubbling toggle events', () => { const container = document.createElement('div'); const ref = React.createRef(); - const onPlay = jest.fn(); - const onCancel = jest.fn(); - const onClose = jest.fn(); const onToggle = jest.fn(); document.body.appendChild(container); try { ReactDOM.render( -
-
+
+
, container, ); ref.current.dispatchEvent( - new Event('play', { + new Event('toggle', { bubbles: false, }), ); + expect(onToggle).toHaveBeenCalledTimes(2); + } finally { + document.body.removeChild(container); + } + }); + + it('should bubble non-native bubbling cancel/close events', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + const onCancel = jest.fn(); + const onClose = jest.fn(); + document.body.appendChild(container); + try { + ReactDOM.render( +
+ +
, + container, + ); ref.current.dispatchEvent( new Event('cancel', { bubbles: false, @@ -575,17 +576,31 @@ describe('ReactDOMEventListener', () => { bubbles: false, }), ); + expect(onCancel).toHaveBeenCalledTimes(2); + expect(onClose).toHaveBeenCalledTimes(2); + } finally { + document.body.removeChild(container); + } + }); + + it('should bubble non-native bubbling media events events', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + const onPlay = jest.fn(); + document.body.appendChild(container); + try { + ReactDOM.render( +
+
, + container, + ); ref.current.dispatchEvent( - new Event('toggle', { + new Event('play', { bubbles: false, }), ); - // Regression test: ensure we still emulate bubbling with non-bubbling - // media expect(onPlay).toHaveBeenCalledTimes(2); - expect(onCancel).toHaveBeenCalledTimes(2); - expect(onClose).toHaveBeenCalledTimes(2); - expect(onToggle).toHaveBeenCalledTimes(2); } finally { document.body.removeChild(container); } From 9032cdbb4352d85569da652f3f74048b0285738d Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 27 Jul 2020 17:28:20 +0100 Subject: [PATCH 3/3] Add onInvalid test --- .../__tests__/ReactDOMEventListener-test.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index f2ba43cb5926..5cb207409da7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -606,6 +606,29 @@ describe('ReactDOMEventListener', () => { } }); + it('should bubble non-native bubbling invalid events', () => { + const container = document.createElement('div'); + const ref = React.createRef(); + const onInvalid = jest.fn(); + document.body.appendChild(container); + try { + ReactDOM.render( +
+ +
, + container, + ); + ref.current.dispatchEvent( + new Event('invalid', { + bubbles: false, + }), + ); + expect(onInvalid).toHaveBeenCalledTimes(2); + } finally { + document.body.removeChild(container); + } + }); + it('should handle non-bubbling capture events correctly', () => { const container = document.createElement('div'); const innerRef = React.createRef();