Skip to content

Fix focus set for delegated and already focused elements #688

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

Closed
everettbu wants to merge 1 commit into from
Closed

Fix focus set for delegated and already focused elements #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 15 additions & 3 deletions 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 @@ -4520,18 +4520,30 @@ export function setFocusIfFocusable(
//
// We could compare the node to document.activeElement after focus,
// but this would not handle the case where application code managed focus to automatically blur.
const element = ((node: any): HTMLElement);

// If this element is already the active element, it's focusable and already
// focused. Calling .focus() on it would be a no-op (no focus event fires),
// so we short-circuit here.
if (element.ownerDocument.activeElement === element) {
return true;
}

let didFocus = false;
const handleFocus = () => {
didFocus = true;
};

const element = ((node: any): HTMLElement);
try {
element.addEventListener('focus', handleFocus);
// Listen on the document in the capture phase so we detect focus even when
// it lands on a different element than the one we called .focus() on. This
// happens with <label> elements (focus delegates to the associated input)
// and shadow hosts with delegatesFocus.
element.ownerDocument.addEventListener('focus', handleFocus, true);
// $FlowFixMe[method-unbinding]
(element.focus || HTMLElement.prototype.focus).call(element, focusOptions);
} finally {
element.removeEventListener('focus', handleFocus);
element.ownerDocument.removeEventListener('focus', handleFocus, true);
}

return didFocus;
Expand Down
96 changes: 96 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 @@ -255,6 +255,102 @@ describe('FragmentRefs', () => {
expect(document.activeElement.id).toEqual('child-b');
document.activeElement.blur();
});

// @gate enableFragmentRefs
it('keeps focus on the first focusable child if already focused', async () => {
const fragmentRef = React.createRef();
const root = ReactDOMClient.createRoot(container);

function Test() {
return (
<Fragment ref={fragmentRef}>
<a id="child-a" href="/">
A
</a>
<a id="child-b" href="/">
B
</a>
</Fragment>
);
}

await act(() => {
root.render(<Test />);
});

// Focus the first child manually
document.getElementById('child-a').focus();
expect(document.activeElement.id).toEqual('child-a');

// Calling fragment.focus() should keep focus on child-a,
// not skip to child-b
await act(() => {
fragmentRef.current.focus();
});
expect(document.activeElement.id).toEqual('child-a');
document.activeElement.blur();
});

// @gate enableFragmentRefs
it('keeps focus on a nested child if already focused', async () => {
const fragmentRef = React.createRef();
const root = ReactDOMClient.createRoot(container);

function Test() {
return (
<Fragment ref={fragmentRef}>
<div>
<input id="nested-input" />
</div>
<a id="sibling-link" href="/">
Link
</a>
</Fragment>
);
}

await act(() => {
root.render(<Test />);
});

// Focus the nested input manually
document.getElementById('nested-input').focus();
expect(document.activeElement.id).toEqual('nested-input');

// Calling fragment.focus() should keep focus on nested-input
await act(() => {
fragmentRef.current.focus();
});
expect(document.activeElement.id).toEqual('nested-input');
document.activeElement.blur();
});

// @gate enableFragmentRefs
it('focuses the first focusable child in a fieldset', async () => {
const fragmentRef = React.createRef();
const root = ReactDOMClient.createRoot(container);

function Test() {
return (
<Fragment ref={fragmentRef}>
<fieldset>
Comment on lines +305 to +336

@greptile-apps greptile-apps Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing direct test for label-delegation fix

Bug fix #1 (the document-level capture listener) is specifically motivated by <label> elements delegating focus to their associated <input>. The new fieldset test exercises the existing depth-first traversal but does not exercise the new code path — a <fieldset> is simply not focusable, so fieldset.focus() fires no focus event at all. The document-level listener only matters when .focus() on element A causes a focus event to fire on a different element B (e.g. label → input).

Consider adding a test like the one below to directly validate the new listener:

// @gate enableFragmentRefs
it('focuses the associated input when calling focus on a label', async () => {
  const fragmentRef = React.createRef();
  const root = ReactDOMClient.createRoot(container);

  function Test() {
    return (
      <Fragment ref={fragmentRef}>
        <label htmlFor="name-input">Name</label>
        <input id="name-input" />
      </Fragment>
    );
  }

  await act(() => {
    root.render(<Test />);
  });
  await act(() => {
    fragmentRef.current.focus();
  });
  expect(document.activeElement.id).toEqual('name-input');
  document.activeElement.blur();
});

(Note: if JSDOM does not implement <label> focus delegation, a shadow-host with delegatesFocus would be the alternative vehicle for this test.)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Line: 305-336

Comment:
**Missing direct test for label-delegation fix**

Bug fix #1 (the document-level capture listener) is specifically motivated by `<label>` elements delegating focus to their associated `<input>`. The new fieldset test exercises the existing depth-first traversal but does not exercise the new code path — a `<fieldset>` is simply not focusable, so `fieldset.focus()` fires no focus event at all. The document-level listener only matters when `.focus()` on element A causes a focus event to fire on a **different** element B (e.g. `label → input`).

Consider adding a test like the one below to directly validate the new listener:

```js
// @gate enableFragmentRefs
it('focuses the associated input when calling focus on a label', async () => {
  const fragmentRef = React.createRef();
  const root = ReactDOMClient.createRoot(container);

  function Test() {
    return (
      <Fragment ref={fragmentRef}>
        <label htmlFor="name-input">Name</label>
        <input id="name-input" />
      </Fragment>
    );
  }

  await act(() => {
    root.render(<Test />);
  });
  await act(() => {
    fragmentRef.current.focus();
  });
  expect(document.activeElement.id).toEqual('name-input');
  document.activeElement.blur();
});
```

(Note: if JSDOM does not implement `<label>` focus delegation, a shadow-host with `delegatesFocus` would be the alternative vehicle for this test.)

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

Fix in Claude Code Fix in Codex

<legend>Shipping</legend>
<input id="street" name="street" />
<input id="city" name="city" />
</fieldset>
</Fragment>
);
}

await act(() => {
root.render(<Test />);
});
await act(() => {
fragmentRef.current.focus();
});
expect(document.activeElement.id).toEqual('street');
document.activeElement.blur();
});
});

describe('focusLast()', () => {
Expand Down
Loading