Skip to content

Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec #716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
everettbu wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec #716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce7699a
Add parameters to options string iFix FragmentInstance listener key m…
Dhakshin2007 Mar 15, 2026
c380e05
Add tests for removing capture listAdd tests for capture option norma…
Dhakshin2007 Mar 15, 2026
fcefce9
fix: key listener identity on capture only, drop passive/once from ke…
Dhakshin2007 Mar 17, 2026
47cd78d
test: add passive option mismatch test for FragmentRef addEventListener
Dhakshin2007 Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
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
124 changes: 124 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,86 @@
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([]);
},
);
Comment on lines +817 to +855
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 15, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation with sibling tests

Both new it() blocks are indented at 8 spaces, but every other it() inside this describe('add/remove event listeners') block uses 6 spaces (e.g., lines 788, 898). This is a minor style nit but could cause confusion when scanning the file.

Suggested change
// @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 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([]);
},
);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Line: 817-855

Comment:
**Inconsistent indentation with sibling tests**

Both new `it()` blocks are indented at 8 spaces, but every other `it()` inside this `describe('add/remove event listeners')` block uses 6 spaces (e.g., lines 788, 898). This is a minor style nit but could cause confusion when scanning the file.

```suggestion
      // @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([]);
        },
      );
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex


// @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([]);
},
);
Comment on lines +857 to +895
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 15, 2026

Choose a reason for hiding this comment

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

Same indentation issue as above

This second test block also has 8-space indentation where it should be 6 spaces to match the rest of the describe('add/remove event listeners') block.

Suggested change
// @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(
'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([]);
},
);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Line: 857-895

Comment:
**Same indentation issue as above**

This second test block also has 8-space indentation where it should be 6 spaces to match the rest of the `describe('add/remove event listeners')` block.

```suggestion
      // @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([]);
        },
      );
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex


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

// @gate enableFragmentRefs
it('does not deduplicate listeners with mismatched passive option', async () => {
const fragmentRef = React.createRef();
const container = document.createElement('div');

Check failure on line 2767 in packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js

View workflow job for this annotation

GitHub Actions / Run eslint 

'container' is already declared in the upper scope on line 17 column 5
document.body.appendChild(container);
const root = ReactDOMClient.createRoot(container);

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

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

// Register with passive: false
fragmentRef.current.addEventListener('click', handler, {passive: false});
// Register with passive: true - DOM spec: these are DIFFERENT listeners
fragmentRef.current.addEventListener('click', handler, {passive: true});

document.querySelector('#child').click();
// Both should fire because passive:false and passive:true are distinct
expect(logs).toEqual(['fired', 'fired']);

// Remove with passive: false only removes the passive:false registration
fragmentRef.current.removeEventListener('click', handler, {passive: false});

logs.length = 0;
document.querySelector('#child').click();
// passive:true listener should still fire
expect(logs).toEqual(['fired']);

fragmentRef.current.removeEventListener('click', handler, {passive: true});

logs.length = 0;
document.querySelector('#child').click();
expect(logs).toEqual([]);

document.body.removeChild(container);
});
});
});
Loading