From 3ae668f382fade6325457b07efd0a39b8d8e966b Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 28 Feb 2025 20:50:13 -0500 Subject: [PATCH] Fix event listener untracking --- .../src/client/ReactFiberConfigDOM.js | 7 +- .../__tests__/ReactDOMFragmentRefs-test.js | 81 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 5bd624000b6d9..c19354f1e767a 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -1619,8 +1619,7 @@ FragmentInstancePseudoElement.prototype.removeEventListener = function ( listener: EventListener, optionsOrUseCapture?: EventListenerOptionsOrUseCapture, ): void { - const listeners = this._eventListeners; - if (listeners === undefined || listeners.length === 0) { + if (this._eventListeners === undefined || this._eventListeners.length === 0) { return; } traverseFragmentInstanceChildren( @@ -1633,12 +1632,12 @@ FragmentInstancePseudoElement.prototype.removeEventListener = function ( optionsOrUseCapture, ), ); - const index = indexOfEventListener(listeners, { + const index = indexOfEventListener(this._eventListeners, { type, listener, optionsOrUseCapture, }); - this._eventListeners = listeners.splice(index, 1); + this._eventListeners.splice(index, 1); }; function removeEventListenerFromChild( type: string, diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index 9f7ec809c96e1..4ae00f7b01215 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -412,6 +412,87 @@ describe('FragmentRefs', () => { expect(logs).toEqual(['Host A', 'Host B']); }); + // @gate enableFragmentRefs + it('allows adding and cleaning up listeners in effects', async () => { + const root = ReactDOMClient.createRoot(container); + + let logs = []; + function logClick(e) { + logs.push(e.currentTarget.id); + } + + let rerender; + let removeEventListeners; + + function Test() { + const fragmentRef = React.useRef(null); + const [_, setState] = React.useState(0); + rerender = () => { + setState(p => p + 1); + }; + removeEventListeners = () => { + fragmentRef.current.removeEventListener('click', logClick); + }; + React.useEffect(() => { + fragmentRef.current.addEventListener('click', logClick); + + return removeEventListeners; + }); + + return ( + +
+ + ); + } + + // The event listener was applied + await act(() => root.render()); + expect(logs).toEqual([]); + document.querySelector('#child-a').click(); + expect(logs).toEqual(['child-a']); + + // The event listener can be removed and re-added + logs = []; + await act(rerender); + document.querySelector('#child-a').click(); + expect(logs).toEqual(['child-a']); + + // The event listener + // Split into two tests + // 1. Removed event listeners are not added to new children + // 2. It can remove and reapply event listeners in effects + }); + + it('does not apply removed event listeners to new children', async () => { + const root = ReactDOMClient.createRoot(container); + const fragmentRef = React.createRef(null); + function Test() { + return ( + +
+ + ); + } + + let logs = []; + function logClick(e) { + logs.push(e.currentTarget.id); + } + await act(() => { + root.render(); + }); + fragmentRef.current.addEventListener('click', logClick); + const childA = document.querySelector('#child-a'); + childA.click(); + expect(logs).toEqual(['child-a']); + + logs = []; + fragmentRef.current.removeEventListener('click', logClick); + childA.click(); + expect(logs).toEqual([]); + }); + describe('with activity', () => { // @gate enableFragmentRefs && enableActivity it('does not apply event listeners to hidden trees', async () => {