Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -3093,7 +3093,7 @@ function normalizeListenerOptions(
return `c=${opts ? '1' : '0'}`;
}

return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`;
return `c=${opts.capture ? '1' : '0'}`;
}
function indexOfEventListener(
eventListeners: Array<StoredEventListener>,
Expand Down
164 changes: 164 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,86 @@ describe('FragmentRefs', () => {
expect(logs).toEqual([]);
});

// @gate enableFragmentRefs
it(
'removes a capture listener registered with boolean when removed with options object',
async () => {
const fragmentRef = React.createRef(null);
function Test() {
return (
<Fragment ref={fragmentRef}>
<div id="child-a" />
</Fragment>
);
}
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<Test />);
});

const logs = [];
function logCapture() {
logs.push('capture');
}

// Register with boolean `true` (capture phase)
fragmentRef.current.addEventListener('click', logCapture, true);
document.querySelector('#child-a').click();
expect(logs).toEqual(['capture']);

logs.length = 0;

// Remove with equivalent options object {capture: true}
// Per DOM spec, these are identical - the listener MUST be removed
fragmentRef.current.removeEventListener('click', logCapture, {
capture: true,
});
document.querySelector('#child-a').click();
// Listener should have been removed - logs must remain empty
expect(logs).toEqual([]);
},
);

// @gate enableFragmentRefs
it(
'removes a capture listener registered with options object when removed with boolean',
async () => {
const fragmentRef = React.createRef(null);
function Test() {
return (
<Fragment ref={fragmentRef}>
<div id="child-b" />
</Fragment>
);
}
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<Test />);
});

const logs = [];
function logCapture() {
logs.push('capture');
}

// Register with options object {capture: true}
fragmentRef.current.addEventListener('click', logCapture, {
capture: true,
});
document.querySelector('#child-b').click();
expect(logs).toEqual(['capture']);

logs.length = 0;

// Remove with boolean `true`
// Per DOM spec, these are identical - the listener MUST be removed
fragmentRef.current.removeEventListener('click', logCapture, true);
document.querySelector('#child-b').click();
// Listener should have been removed - logs must remain empty
expect(logs).toEqual([]);
},
);

// @gate enableFragmentRefs
it('applies event listeners to portaled children', async () => {
const fragmentRef = React.createRef();
Expand Down Expand Up @@ -2680,5 +2760,89 @@ describe('FragmentRefs', () => {
window.scrollTo = originalScrollTo;
restoreRange();
});

// @gate enableFragmentRefs
it(
'treats passive:true and passive:false as same listener per DOM spec',
async () => {
const fragmentRef = React.createRef();
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(
<Fragment ref={fragmentRef}>
<div id="child" />
</Fragment>,
);
});

const logs = [];
const handler = () => logs.push('fired');

const child = document.querySelector('#child');
const spy = jest.spyOn(child, 'addEventListener');
// Per DOM spec, listener identity is (type, callback, capture).
// passive is NOT part of the key, so these are the SAME listener.
fragmentRef.current.addEventListener('click', handler, {passive: false});
// Second add is a no-op: same (type, callback, capture) identity.
fragmentRef.current.addEventListener('click', handler, {passive: true});
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith('click', handler, {passive: false});

document.querySelector('#child').click();
// First handler fires once (second add was a no-op).
expect(logs).toEqual(['fired']);
Comment on lines +2786 to +2794
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should make it clear which handler is fired: The first or second one? By spec, the second handler shouldn't have been appended:

The event listener is appended to target’s event listener list and is not appended if it has the same type, callback, and capture.

-- https://dom.spec.whatwg.org/#concept-event-listener:~:text=The%20event%20listener%20is%20appended%20to%20target%E2%80%99s%20event%20listener%20list%20and%20is%20not%20appended%20if%20it%20has%20the%20same%20type%2C%20callback%2C%20and%20capture.

It also looks like there's some asymmetry with removing. For adding we just look at capture. But removing apparently looks at all the options:

Removes the event listener in target’s event listener list with the same type, callback, and options.

-- https://dom.spec.whatwg.org/#concept-event-listener:~:text=Removes%20the%20event%20listener%20in%20target%E2%80%99s%20event%20listener%20list%20with%20the%20same%20type%2C%20callback%2C%20and%20options.

Can you do some testing how popular browsers behave?


// removeEventListener also ignores passive when matching
fragmentRef.current.removeEventListener('click', handler, {
passive: true,
});

logs.length = 0;
document.querySelector('#child').click();
expect(logs).toEqual([]);
},
);
// @gate enableFragmentRefs
it(
'removes a listener registered with passive:false when removed with passive:true',
async () => {
const fragmentRef = React.createRef(null);
function Test() {
return (
<>
<div id="child-x" />
</>
);
}
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(
<Fragment ref={fragmentRef}>
<Test />
</Fragment>,
);
});
const logs = [];
function handler() {
logs.push('fired');
}
// Register with passive: false
fragmentRef.current.addEventListener('click', handler, {
passive: false,
});
document.querySelector('#child-x').click();
expect(logs).toEqual(['fired']);
logs.length = 0;
// Remove with passive: true - per DOM spec, passive is NOT part of identity
// so this MUST remove the listener regardless of passive mismatch.
fragmentRef.current.removeEventListener('click', handler, {
passive: true,
});
document.querySelector('#child-x').click();
// Listener removed - no more invocations
expect(logs).toEqual([]);
},
);
});
});