From 4235b3e14fe6a9c3119a5550731f5fd91a5b0bcb Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Mon, 22 Jan 2024 11:27:45 -0500 Subject: [PATCH 1/4] Convert ReactDOMEventListener to createRoot --- .../__tests__/ReactDOMEventListener-test.js | 1022 +++++++++-------- 1 file changed, 572 insertions(+), 450 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index e4412632e17ee..1cec7bd9fed83 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -11,31 +11,41 @@ describe('ReactDOMEventListener', () => { let React; - let ReactDOM; + let ReactDOMX; + let ReactDOMClient; let ReactDOMServer; + let act; + let waitForPaint; beforeEach(() => { jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMX = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); + act = require('internal-test-utils').act; + const InternalTestUtils = require('internal-test-utils'); + waitForPaint = InternalTestUtils.waitForPaint; }); describe('Propagation', () => { - it('should propagate events one level down', () => { + it('should propagate events one level down', async () => { const mouseOut = jest.fn(); const onMouseOut = event => mouseOut(event.currentTarget); const childContainer = document.createElement('div'); const parentContainer = document.createElement('div'); - const childNode = ReactDOM.render( -
Child
, - childContainer, - ); - const parentNode = ReactDOM.render( -
div
, - parentContainer, - ); + + const childRoot = ReactDOMClient.createRoot(childContainer); + const parentRoot = ReactDOMClient.createRoot(parentContainer); + + await act(() => { + childRoot.render(
Child
); + parentRoot.render(
Parent
); + }); + const parentNode = parentContainer.firstChild; + const childNode = childContainer.firstChild; + parentNode.appendChild(childContainer); document.body.appendChild(parentContainer); @@ -44,34 +54,35 @@ describe('ReactDOMEventListener', () => { nativeEvent.initEvent('mouseout', true, true); childNode.dispatchEvent(nativeEvent); - expect(mouseOut).toBeCalled(); expect(mouseOut).toHaveBeenCalledTimes(2); - expect(mouseOut.mock.calls[0][0]).toEqual(childNode); - expect(mouseOut.mock.calls[1][0]).toEqual(parentNode); + expect(mouseOut).toHaveBeenNthCalledWith(1, childNode); + expect(mouseOut).toHaveBeenNthCalledWith(2, parentNode); } finally { document.body.removeChild(parentContainer); } }); - it('should propagate events two levels down', () => { + it('should propagate events two levels down', async () => { const mouseOut = jest.fn(); const onMouseOut = event => mouseOut(event.currentTarget); const childContainer = document.createElement('div'); const parentContainer = document.createElement('div'); const grandParentContainer = document.createElement('div'); - const childNode = ReactDOM.render( -
Child
, - childContainer, - ); - const parentNode = ReactDOM.render( -
Parent
, - parentContainer, - ); - const grandParentNode = ReactDOM.render( -
Parent
, - grandParentContainer, - ); + + const childRoot = ReactDOMClient.createRoot(childContainer); + const parentRoot = ReactDOMClient.createRoot(parentContainer); + const grandParentRoot = ReactDOMClient.createRoot(grandParentContainer); + + await act(() => { + childRoot.render(
Child
); + parentRoot.render(
Parent
); + grandParentRoot.render(
Grandparent
); + }); + const childNode = childContainer.firstChild; + const parentNode = parentContainer.firstChild; + const grandParentNode = grandParentContainer.firstChild; + parentNode.appendChild(childContainer); grandParentNode.appendChild(parentContainer); @@ -82,18 +93,17 @@ describe('ReactDOMEventListener', () => { nativeEvent.initEvent('mouseout', true, true); childNode.dispatchEvent(nativeEvent); - expect(mouseOut).toBeCalled(); expect(mouseOut).toHaveBeenCalledTimes(3); - expect(mouseOut.mock.calls[0][0]).toEqual(childNode); - expect(mouseOut.mock.calls[1][0]).toEqual(parentNode); - expect(mouseOut.mock.calls[2][0]).toEqual(grandParentNode); + expect(mouseOut).toHaveBeenNthCalledWith(1, childNode); + expect(mouseOut).toHaveBeenNthCalledWith(2, parentNode); + expect(mouseOut).toHaveBeenNthCalledWith(3, grandParentNode); } finally { document.body.removeChild(grandParentContainer); } }); // Regression test for https://github.com/facebook/react/issues/1105 - it('should not get confused by disappearing elements', () => { + it('should not get confused by disappearing elements', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -104,10 +114,10 @@ describe('ReactDOMEventListener', () => { this.setState({clicked: true}); }; componentDidMount() { - expect(ReactDOM.findDOMNode(this)).toBe(container.firstChild); + expect(ReactDOMX.findDOMNode(this)).toBe(container.firstChild); } componentDidUpdate() { - expect(ReactDOM.findDOMNode(this)).toBe(container.firstChild); + expect(ReactDOMX.findDOMNode(this)).toBe(container.firstChild); } render() { if (this.state.clicked) { @@ -119,41 +129,59 @@ describe('ReactDOMEventListener', () => { } } } - ReactDOM.render(, container); - container.firstChild.dispatchEvent( - new MouseEvent('click', { - bubbles: true, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + await act(() => { + container.firstChild.dispatchEvent( + new MouseEvent('click', { + bubbles: true, + }), + ); + }); expect(container.firstChild.textContent).toBe('clicked!'); } finally { document.body.removeChild(container); } }); - it('should batch between handlers from different roots', () => { + it('should batch between handlers from different roots', async () => { const mock = jest.fn(); const childContainer = document.createElement('div'); - const handleChildMouseOut = () => { - ReactDOM.render(
1
, childContainer); - mock(childNode.textContent); - }; - const parentContainer = document.createElement('div'); - const handleParentMouseOut = () => { - ReactDOM.render(
2
, childContainer); - mock(childNode.textContent); - }; + const childRoot = ReactDOMClient.createRoot(childContainer); + const parentRoot = ReactDOMClient.createRoot(parentContainer); + let childSetState; + + function Parent() { + // eslint-disable-next-line no-unused-vars + const [state, _] = React.useState('Parent'); + const handleMouseOut = () => { + childSetState(2); + mock(childContainer.firstChild.textContent); + }; + return
{state}
; + } - const childNode = ReactDOM.render( -
Child
, - childContainer, - ); - const parentNode = ReactDOM.render( -
Parent
, - parentContainer, - ); + function Child() { + const [state, setState] = React.useState('Child'); + childSetState = setState; + const handleMouseOut = () => { + setState(1); + mock(childContainer.firstChild.textContent); + }; + return
{state}
; + } + + await act(() => { + childRoot.render(); + parentRoot.render(); + }); + + const childNode = childContainer.firstChild; + const parentNode = parentContainer.firstChild; parentNode.appendChild(childContainer); document.body.appendChild(parentContainer); @@ -164,23 +192,20 @@ describe('ReactDOMEventListener', () => { // Child and parent should both call from event handlers. expect(mock).toHaveBeenCalledTimes(2); - // The first call schedules a render of '1' into the 'Child'. - // However, we're batching so it isn't flushed yet. - expect(mock.mock.calls[0][0]).toBe('Child'); - // As we have two roots, it means we have two event listeners. - // This also means we enter the event batching phase twice, - // flushing the child to be 1. - - // We don't have any good way of knowing if another event will - // occur because another event handler might invoke - // stopPropagation() along the way. After discussions internally - // with Sebastian, it seems that for now over-flushing should - // be fine, especially as the new event system is a breaking - // change anyway. We can maybe revisit this later as part of - // the work to refine this in the scheduler (maybe by leveraging - // isInputPending?). - expect(mock.mock.calls[1][0]).toBe('1'); - // By the time we leave the handler, the second update is flushed. + + // However, we're batching, so they aren't flushed yet. + expect(mock).toHaveBeenNthCalledWith(1, 'Child'); + expect(mock).toHaveBeenNthCalledWith(2, 'Child'); + expect(parentNode.textContent).toBe('ParentChild'); + expect(childNode.textContent).toBe('Child'); + mock.mockClear(); + + // Flush the batched updates. + await waitForPaint([]); + + // The batched updates are applied. + expect(mock).not.toBeCalled(); + expect(parentNode.textContent).toBe('Parent2'); expect(childNode.textContent).toBe('2'); } finally { document.body.removeChild(parentContainer); @@ -188,7 +213,7 @@ describe('ReactDOMEventListener', () => { }); }); - it('should not fire duplicate events for a React DOM tree', () => { + it('should not fire duplicate events for a React DOM tree', async () => { const mouseOut = jest.fn(); const onMouseOut = event => mouseOut(event.target); @@ -211,25 +236,30 @@ describe('ReactDOMEventListener', () => { } const container = document.createElement('div'); - const instance = ReactDOM.render(, container); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + + const instance = container.firstChild.firstChild; document.body.appendChild(container); try { const nativeEvent = document.createEvent('Event'); nativeEvent.initEvent('mouseout', true, true); - instance.getInner().dispatchEvent(nativeEvent); + await act(() => { + instance.dispatchEvent(nativeEvent); + }); - expect(mouseOut).toBeCalled(); - expect(mouseOut).toHaveBeenCalledTimes(1); - expect(mouseOut.mock.calls[0][0]).toEqual(instance.getInner()); + expect(mouseOut).toBeCalledWith(instance); } finally { document.body.removeChild(container); } }); // Regression test for https://github.com/facebook/react/pull/12877 - it('should not fire form events twice', () => { + it('should not fire form events twice', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -239,43 +269,54 @@ describe('ReactDOMEventListener', () => { const handleInvalid = jest.fn(); const handleReset = jest.fn(); const handleSubmit = jest.fn(); - ReactDOM.render( -
- -
, - container, - ); - inputRef.current.dispatchEvent( - new Event('invalid', { - // https://developer.mozilla.org/en-US/docs/Web/Events/invalid - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); + + await act(() => { + inputRef.current.dispatchEvent( + new Event('invalid', { + // https://developer.mozilla.org/en-US/docs/Web/Events/invalid + bubbles: false, + }), + ); + }); expect(handleInvalid).toHaveBeenCalledTimes(1); - formRef.current.dispatchEvent( - new Event('reset', { - // https://developer.mozilla.org/en-US/docs/Web/Events/reset - bubbles: true, - }), - ); + await act(() => { + formRef.current.dispatchEvent( + new Event('reset', { + // https://developer.mozilla.org/en-US/docs/Web/Events/reset + bubbles: true, + }), + ); + }); expect(handleReset).toHaveBeenCalledTimes(1); - formRef.current.dispatchEvent( - new Event('submit', { - // https://developer.mozilla.org/en-US/docs/Web/Events/submit - bubbles: true, - }), - ); + await act(() => { + formRef.current.dispatchEvent( + new Event('submit', { + // https://developer.mozilla.org/en-US/docs/Web/Events/submit + bubbles: true, + }), + ); + }); expect(handleSubmit).toHaveBeenCalledTimes(1); - formRef.current.dispatchEvent( - new Event('submit', { - // Might happen on older browsers. - bubbles: true, - }), - ); + await act(() => { + formRef.current.dispatchEvent( + new Event('submit', { + // Might happen on older browsers. + bubbles: true, + }), + ); + }); expect(handleSubmit).toHaveBeenCalledTimes(2); // It already fired in this test. document.body.removeChild(container); @@ -284,7 +325,7 @@ describe('ReactDOMEventListener', () => { // This tests an implementation detail that submit/reset events are listened to // at the document level, which is necessary for event replaying to work. // They bubble in all modern browsers. - it('should not receive submit events if native, interim DOM handler prevents it', () => { + it('should not receive submit events if native, interim DOM handler prevents it', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -294,30 +335,34 @@ describe('ReactDOMEventListener', () => { const handleSubmit = jest.fn(); const handleReset = jest.fn(); - ReactDOM.render( -
-
-
, - container, - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); interimRef.current.onsubmit = nativeEvent => nativeEvent.stopPropagation(); interimRef.current.onreset = nativeEvent => nativeEvent.stopPropagation(); - formRef.current.dispatchEvent( - new Event('submit', { - // https://developer.mozilla.org/en-US/docs/Web/Events/submit - bubbles: true, - }), - ); + await act(() => { + formRef.current.dispatchEvent( + new Event('submit', { + // https://developer.mozilla.org/en-US/docs/Web/Events/submit + bubbles: true, + }), + ); - formRef.current.dispatchEvent( - new Event('reset', { - // https://developer.mozilla.org/en-US/docs/Web/Events/reset - bubbles: true, - }), - ); + formRef.current.dispatchEvent( + new Event('reset', { + // https://developer.mozilla.org/en-US/docs/Web/Events/reset + bubbles: true, + }), + ); + }); expect(handleSubmit).not.toHaveBeenCalled(); expect(handleReset).not.toHaveBeenCalled(); @@ -326,7 +371,7 @@ describe('ReactDOMEventListener', () => { } }); - it('should dispatch loadstart only for media elements', () => { + it('should dispatch loadstart only for media elements', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -336,35 +381,41 @@ describe('ReactDOMEventListener', () => { const handleImgLoadStart = jest.fn(); const handleVideoLoadStart = jest.fn(); - ReactDOM.render( -
- -
, - container, - ); - - // Note for debugging: loadstart currently doesn't fire in Chrome. - // https://bugs.chromium.org/p/chromium/issues/detail?id=458851 - imgRef.current.dispatchEvent( - new ProgressEvent('loadstart', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); + + await act(() => { + // Note for debugging: loadstart currently doesn't fire in Chrome. + // https://bugs.chromium.org/p/chromium/issues/detail?id=458851 + imgRef.current.dispatchEvent( + new ProgressEvent('loadstart', { + bubbles: false, + }), + ); + }); expect(handleImgLoadStart).toHaveBeenCalledTimes(0); - videoRef.current.dispatchEvent( - new ProgressEvent('loadstart', { - bubbles: false, - }), - ); + await act(() => { + videoRef.current.dispatchEvent( + new ProgressEvent('loadstart', { + bubbles: false, + }), + ); + }); expect(handleVideoLoadStart).toHaveBeenCalledTimes(1); } finally { document.body.removeChild(container); } }); - it('should not attempt to listen to unnecessary events on the top level', () => { + it('should not attempt to listen to unnecessary events on the top level', async () => { const container = document.createElement('div'); document.body.appendChild(container); @@ -451,22 +502,25 @@ describe('ReactDOMEventListener', () => { try { // We expect that mounting this tree will // *not* attach handlers for any top-level events. - ReactDOM.render( -
-
, - container, - ); - - // Also verify dispatching one of them works - videoRef.current.dispatchEvent( - new Event('play', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+
, + ); + }); + await act(() => { + // Also verify dispatching one of them works + videoRef.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + }); expect(handleVideoPlay).toHaveBeenCalledTimes(1); // Unlike browsers, we delegate media events. // (This doesn't make a lot of sense but it would be a breaking change not to.) @@ -478,26 +532,28 @@ describe('ReactDOMEventListener', () => { } }); - it('should dispatch load for embed elements', () => { + it('should dispatch load for embed elements', async () => { const container = document.createElement('div'); document.body.appendChild(container); try { const ref = React.createRef(); const handleLoad = jest.fn(); - - ReactDOM.render( -
- -
, - container, - ); - - ref.current.dispatchEvent( - new ProgressEvent('load', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new ProgressEvent('load', { + bubbles: false, + }), + ); + }); expect(handleLoad).toHaveBeenCalledTimes(1); } finally { @@ -507,24 +563,29 @@ describe('ReactDOMEventListener', () => { // Unlike browsers, we delegate media events. // (This doesn't make a lot of sense but it would be a breaking change not to.) - it('should delegate media events even without a direct listener', () => { + it('should delegate media events even without a direct listener', async () => { const container = document.createElement('div'); const ref = React.createRef(); const handleVideoPlayDelegated = jest.fn(); document.body.appendChild(container); try { - ReactDOM.render( -
- {/* Intentionally no handler on the target: */} -
, - container, - ); - ref.current.dispatchEvent( - new Event('play', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ {/* Intentionally no handler on the target: */} +
, + ); + }); + + await act(() => { + ref.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + }); // Regression test: ensure React tree delegation still works // even if the actual DOM element did not have a handler. expect(handleVideoPlayDelegated).toHaveBeenCalledTimes(1); @@ -533,30 +594,34 @@ describe('ReactDOMEventListener', () => { } }); - it('should delegate dialog events even without a direct listener', () => { + it('should delegate dialog events even without a direct listener', async () => { const container = document.createElement('div'); const ref = React.createRef(); const onCancel = jest.fn(); const onClose = jest.fn(); document.body.appendChild(container); try { - ReactDOM.render( -
- {/* Intentionally no handler on the target: */} - -
, - container, - ); - ref.current.dispatchEvent( - new Event('close', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('cancel', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ {/* Intentionally no handler on the target: */} + +
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('close', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('cancel', { + bubbles: false, + }), + ); + }); // Regression test: ensure React tree delegation still works // even if the actual DOM element did not have a handler. expect(onCancel).toHaveBeenCalledTimes(1); @@ -566,52 +631,60 @@ describe('ReactDOMEventListener', () => { } }); - it('should bubble non-native bubbling toggle events', () => { + it('should bubble non-native bubbling toggle events', async () => { const container = document.createElement('div'); const ref = React.createRef(); const onToggle = jest.fn(); document.body.appendChild(container); try { - ReactDOM.render( -
-
-
, - container, - ); - ref.current.dispatchEvent( - new Event('toggle', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('toggle', { + bubbles: false, + }), + ); + }); expect(onToggle).toHaveBeenCalledTimes(2); } finally { document.body.removeChild(container); } }); - it('should bubble non-native bubbling cancel/close events', () => { + it('should bubble non-native bubbling cancel/close events', async () => { 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, - }), - ); - ref.current.dispatchEvent( - new Event('close', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('cancel', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('close', { + bubbles: false, + }), + ); + }); expect(onCancel).toHaveBeenCalledTimes(2); expect(onClose).toHaveBeenCalledTimes(2); } finally { @@ -619,53 +692,62 @@ describe('ReactDOMEventListener', () => { } }); - it('should bubble non-native bubbling media events events', () => { + it('should bubble non-native bubbling media events events', async () => { 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('play', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + }); expect(onPlay).toHaveBeenCalledTimes(2); } finally { document.body.removeChild(container); } }); - it('should bubble non-native bubbling invalid events', () => { + it('should bubble non-native bubbling invalid events', async () => { 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, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ +
, + ); + }); + + await act(() => { + 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', () => { + it('should handle non-bubbling capture events correctly', async () => { const container = document.createElement('div'); const innerRef = React.createRef(); const outerRef = React.createRef(); @@ -673,30 +755,36 @@ describe('ReactDOMEventListener', () => { const log = []; document.body.appendChild(container); try { - ReactDOM.render( -
-
-
-
-
, - container, - ); - innerRef.current.dispatchEvent( - new Event('play', { - bubbles: false, - }), - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+
+
+
+
, + ); + }); + await act(() => { + innerRef.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + }); expect(onPlayCapture).toHaveBeenCalledTimes(3); expect(log).toEqual([ outerRef.current, outerRef.current.firstChild, innerRef.current, ]); - outerRef.current.dispatchEvent( - new Event('play', { - bubbles: false, - }), - ); + await act(() => { + outerRef.current.dispatchEvent( + new Event('play', { + bubbles: false, + }), + ); + }); expect(onPlayCapture).toHaveBeenCalledTimes(4); expect(log).toEqual([ outerRef.current, @@ -712,7 +800,7 @@ describe('ReactDOMEventListener', () => { // 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', () => { + it('should not emulate bubbling of scroll events', async () => { const container = document.createElement('div'); const ref = React.createRef(); const log = []; @@ -730,41 +818,46 @@ describe('ReactDOMEventListener', () => { ); document.body.appendChild(container); try { - ReactDOM.render( -
+ const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(
-
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); + onScrollEndCapture={onScrollEndCapture}> +
+
+
, + ); + }); + + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([ ['onScroll', 'capture', 'grand'], ['onScroll', 'capture', 'parent'], @@ -783,7 +876,7 @@ describe('ReactDOMEventListener', () => { // 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)', () => { + it('should not emulate bubbling of scroll events (no own handler)', async () => { const container = document.createElement('div'); const ref = React.createRef(); const log = []; @@ -801,35 +894,39 @@ describe('ReactDOMEventListener', () => { ); document.body.appendChild(container); try { - ReactDOM.render( -
+ const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(
- {/* Intentionally no handler on the child: */} -
-
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); +
+ {/* Intentionally no handler on the child: */} +
+
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([ ['onScroll', 'capture', 'grand'], ['onScroll', 'capture', 'parent'], @@ -841,7 +938,7 @@ describe('ReactDOMEventListener', () => { } }); - it('should subscribe to scroll during updates', () => { + it('should subscribe to scroll during updates', async () => { const container = document.createElement('div'); const ref = React.createRef(); const log = []; @@ -859,51 +956,57 @@ describe('ReactDOMEventListener', () => { ); document.body.appendChild(container); try { - ReactDOM.render( -
+ const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(
-
-
-
, - container, - ); +
+
+
+
, + ); + }); - // Update to attach. - ReactDOM.render( -
onScroll(e)} - onScrollCapture={e => onScrollCapture(e)} - onScrollEnd={e => onScrollEnd(e)} - onScrollEndCapture={e => onScrollEndCapture(e)}> + await act(() => { + // Update to attach. + root.render(
onScroll(e)} onScrollCapture={e => onScrollCapture(e)} onScrollEnd={e => onScrollEnd(e)} onScrollEndCapture={e => onScrollEndCapture(e)}>
onScroll(e)} onScrollCapture={e => onScrollCapture(e)} onScrollEnd={e => onScrollEnd(e)} - onScrollEndCapture={e => onScrollEndCapture(e)} - ref={ref} - /> -
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); + onScrollEndCapture={e => onScrollEndCapture(e)}> +
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)} + onScrollEnd={e => onScrollEnd(e)} + onScrollEndCapture={e => onScrollEndCapture(e)} + ref={ref} + /> +
+
, + ); + }); + + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([ ['onScroll', 'capture', 'grand'], ['onScroll', 'capture', 'parent'], @@ -917,43 +1020,46 @@ describe('ReactDOMEventListener', () => { // Update to verify deduplication. log.length = 0; - ReactDOM.render( -
onScroll(e)} - onScrollCapture={e => onScrollCapture(e)} - onScrollEnd={e => onScrollEnd(e)} - onScrollEndCapture={e => onScrollEndCapture(e)}> + await act(() => { + root.render(
onScroll(e)} onScrollCapture={e => onScrollCapture(e)} onScrollEnd={e => onScrollEnd(e)} onScrollEndCapture={e => onScrollEndCapture(e)}>
onScroll(e)} onScrollCapture={e => onScrollCapture(e)} onScrollEnd={e => onScrollEnd(e)} - onScrollEndCapture={e => onScrollEndCapture(e)} - ref={ref} - /> -
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); + onScrollEndCapture={e => onScrollEndCapture(e)}> +
onScroll(e)} + onScrollCapture={e => onScrollCapture(e)} + onScrollEnd={e => onScrollEnd(e)} + onScrollEndCapture={e => onScrollEndCapture(e)} + ref={ref} + /> +
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([ ['onScroll', 'capture', 'grand'], ['onScroll', 'capture', 'parent'], @@ -967,24 +1073,27 @@ describe('ReactDOMEventListener', () => { // Update to detach. log.length = 0; - ReactDOM.render( -
+ await act(() => { + root.render(
-
-
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); +
+
+
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([]); } finally { document.body.removeChild(container); @@ -992,7 +1101,7 @@ describe('ReactDOMEventListener', () => { }); // Regression test. - it('should subscribe to scroll during hydration', () => { + it('should subscribe to scroll during hydration', async () => { const container = document.createElement('div'); const ref = React.createRef(); const log = []; @@ -1036,17 +1145,22 @@ describe('ReactDOMEventListener', () => { document.body.appendChild(container); try { container.innerHTML = ReactDOMServer.renderToString(tree); - ReactDOM.hydrate(tree, container); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); + let root; + await act(() => { + root = ReactDOMClient.hydrateRoot(container, tree); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([ ['onScroll', 'capture', 'grand'], ['onScroll', 'capture', 'parent'], @@ -1059,31 +1173,34 @@ describe('ReactDOMEventListener', () => { ]); log.length = 0; - ReactDOM.render( -
+ await act(() => { + root.render(
-
-
-
, - container, - ); - ref.current.dispatchEvent( - new Event('scroll', { - bubbles: false, - }), - ); - ref.current.dispatchEvent( - new Event('scrollend', { - bubbles: false, - }), - ); +
+
+
+
, + ); + }); + await act(() => { + ref.current.dispatchEvent( + new Event('scroll', { + bubbles: false, + }), + ); + ref.current.dispatchEvent( + new Event('scrollend', { + bubbles: false, + }), + ); + }); expect(log).toEqual([]); } finally { document.body.removeChild(container); } }); - it('should not subscribe to selectionchange twice', () => { + it('should not subscribe to selectionchange twice', async () => { const log = []; const originalDocAddEventListener = document.addEventListener; @@ -1099,8 +1216,13 @@ describe('ReactDOMEventListener', () => { } }; try { - ReactDOM.render(, document.createElement('div')); - ReactDOM.render(, document.createElement('div')); + const rootOne = ReactDOMClient.createRoot(document.createElement('div')); + const rootTwo = ReactDOMClient.createRoot(document.createElement('div')); + + await act(() => { + rootOne.render(); + rootTwo.render(); + }); } finally { document.addEventListener = originalDocAddEventListener; } From ed36661e59efb4eccc2455ce36639c457f494024 Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Tue, 23 Jan 2024 23:13:04 -0500 Subject: [PATCH 2/4] Fix innerRef and ReactDOMX --- .../__tests__/ReactDOMEventListener-test.js | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 1cec7bd9fed83..4335864cec981 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -11,7 +11,7 @@ describe('ReactDOMEventListener', () => { let React; - let ReactDOMX; + let ReactDOM; let ReactDOMClient; let ReactDOMServer; let act; @@ -20,7 +20,7 @@ describe('ReactDOMEventListener', () => { beforeEach(() => { jest.resetModules(); React = require('react'); - ReactDOMX = require('react-dom'); + ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); act = require('internal-test-utils').act; @@ -114,10 +114,10 @@ describe('ReactDOMEventListener', () => { this.setState({clicked: true}); }; componentDidMount() { - expect(ReactDOMX.findDOMNode(this)).toBe(container.firstChild); + expect(ReactDOM.findDOMNode(this)).toBe(container.firstChild); } componentDidUpdate() { - expect(ReactDOMX.findDOMNode(this)).toBe(container.firstChild); + expect(ReactDOM.findDOMNode(this)).toBe(container.firstChild); } render() { if (this.state.clicked) { @@ -217,18 +217,13 @@ describe('ReactDOMEventListener', () => { const mouseOut = jest.fn(); const onMouseOut = event => mouseOut(event.target); + const innerRef = React.createRef(); class Wrapper extends React.Component { - innerRef = React.createRef(); - getInner = () => { - return this.innerRef.current; - }; - render() { - const inner =
Inner
; return (
- {inner} +
Inner
); @@ -241,18 +236,16 @@ describe('ReactDOMEventListener', () => { root.render(); }); - const instance = container.firstChild.firstChild; - document.body.appendChild(container); try { const nativeEvent = document.createEvent('Event'); nativeEvent.initEvent('mouseout', true, true); await act(() => { - instance.dispatchEvent(nativeEvent); + innerRef.current.dispatchEvent(nativeEvent); }); - expect(mouseOut).toBeCalledWith(instance); + expect(mouseOut).toBeCalledWith(innerRef.current); } finally { document.body.removeChild(container); } From 3cfeb68a6e7e8474937813887d26389ac20e22c8 Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 21:32:03 -0500 Subject: [PATCH 3/4] Revert batching test back, will follow up --- .../__tests__/ReactDOMEventListener-test.js | 82 ++++++++----------- 1 file changed, 36 insertions(+), 46 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 4335864cec981..345da264473c8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -146,42 +146,29 @@ describe('ReactDOMEventListener', () => { } }); - it('should batch between handlers from different roots', async () => { + it('should batch between handlers from different roots', () => { const mock = jest.fn(); const childContainer = document.createElement('div'); - const parentContainer = document.createElement('div'); - const childRoot = ReactDOMClient.createRoot(childContainer); - const parentRoot = ReactDOMClient.createRoot(parentContainer); - let childSetState; - - function Parent() { - // eslint-disable-next-line no-unused-vars - const [state, _] = React.useState('Parent'); - const handleMouseOut = () => { - childSetState(2); - mock(childContainer.firstChild.textContent); - }; - return
{state}
; - } - - function Child() { - const [state, setState] = React.useState('Child'); - childSetState = setState; - const handleMouseOut = () => { - setState(1); - mock(childContainer.firstChild.textContent); - }; - return
{state}
; - } - - await act(() => { - childRoot.render(); - parentRoot.render(); - }); + const handleChildMouseOut = () => { + ReactDOM.render(
1
, childContainer); + mock(childNode.textContent); + }; - const childNode = childContainer.firstChild; - const parentNode = parentContainer.firstChild; + const parentContainer = document.createElement('div'); + const handleParentMouseOut = () => { + ReactDOM.render(
2
, childContainer); + mock(childNode.textContent); + }; + + const childNode = ReactDOM.render( +
Child
, + childContainer, + ); + const parentNode = ReactDOM.render( +
Parent
, + parentContainer, + ); parentNode.appendChild(childContainer); document.body.appendChild(parentContainer); @@ -192,20 +179,23 @@ describe('ReactDOMEventListener', () => { // Child and parent should both call from event handlers. expect(mock).toHaveBeenCalledTimes(2); - - // However, we're batching, so they aren't flushed yet. - expect(mock).toHaveBeenNthCalledWith(1, 'Child'); - expect(mock).toHaveBeenNthCalledWith(2, 'Child'); - expect(parentNode.textContent).toBe('ParentChild'); - expect(childNode.textContent).toBe('Child'); - mock.mockClear(); - - // Flush the batched updates. - await waitForPaint([]); - - // The batched updates are applied. - expect(mock).not.toBeCalled(); - expect(parentNode.textContent).toBe('Parent2'); + // The first call schedules a render of '1' into the 'Child'. + // However, we're batching so it isn't flushed yet. + expect(mock.mock.calls[0][0]).toBe('Child'); + // As we have two roots, it means we have two event listeners. + // This also means we enter the event batching phase twice, + // flushing the child to be 1. + + // We don't have any good way of knowing if another event will + // occur because another event handler might invoke + // stopPropagation() along the way. After discussions internally + // with Sebastian, it seems that for now over-flushing should + // be fine, especially as the new event system is a breaking + // change anyway. We can maybe revisit this later as part of + // the work to refine this in the scheduler (maybe by leveraging + // isInputPending?). + expect(mock.mock.calls[1][0]).toBe('1'); + // By the time we leave the handler, the second update is flushed. expect(childNode.textContent).toBe('2'); } finally { document.body.removeChild(parentContainer); From 9cd87d67510a3a37891677fb6119d1e82b9a35fe Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 22:33:26 -0500 Subject: [PATCH 4/4] Fix lint --- packages/react-dom/src/__tests__/ReactDOMEventListener-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index 345da264473c8..d884b92d7fd49 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -15,7 +15,6 @@ describe('ReactDOMEventListener', () => { let ReactDOMClient; let ReactDOMServer; let act; - let waitForPaint; beforeEach(() => { jest.resetModules(); @@ -24,8 +23,6 @@ describe('ReactDOMEventListener', () => { ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); act = require('internal-test-utils').act; - const InternalTestUtils = require('internal-test-utils'); - waitForPaint = InternalTestUtils.waitForPaint; }); describe('Propagation', () => {