Skip to content

Fix focus set for delegated and already focused elements#688

Closed
everettbu wants to merge 1 commit into
mainfrom
setfocusfixes
Closed

Fix focus set for delegated and already focused elements#688
everettbu wants to merge 1 commit into
mainfrom
setfocusfixes

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#36010
Original author: jackpope


I found two focus bugs when working on documentation for Fragment Refs.

  1. If an element delegates focus handling, it will return false from setFocusIfFocusable even though a focus event has occured on a different element. The fix for this is a document level event listener rather than only listening on the current element.

For example, if you have a form with multiple nested label>inputs. Calling focus on the label will focus its input but not fire an event on the label. setFocusIfFocusable returns false and you end up continuing to attempt focus down the form tree.

  1. If an element is already focused, setFocusIfFocusable will return false. The fix for this is checking the document's activeElement with an early return.

In the same form example, if the first input is already focused and you call fragmentInstance.focus() at the form level, the second input would end up getting focused since the focus event on the first is not triggered.

I found two focus bugs when working on documentation for Fragment Refs.

1) If an element delegates focus handling, it will return false from setFocusIfFocusable even though a focus event has occured on a different element. The fix for this is a document level event listener rather than only listening on the current element.

For example, if you have a form with multiple nested label>inputs. Calling focus on the label will focus its input but not fire an event on the label. setFocusIfFocusable returns false and you end up continuing to attempt focus down the form tree.

2) If an element is already focused, setFocusIfFocusable will return false. The fix for this is checking the document's activeElement with an early return.

In the same form example, if the first input is already focused and you call fragmentInstance.focus() at the form level, the second input would end up getting focused since the focus event on the first is not triggered.
@greptile-apps

greptile-apps Bot commented Mar 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes two related bugs in setFocusIfFocusable within the Fragment Refs focus-traversal path, improving correctness when fragment focus is invoked on a tree that already has focus or contains elements that delegate focus.

  • Already-focused early return (activeElement === element): Before this fix, calling .focus() on an already-focused element would silently no-op (no focus event fires), causing setFocusIfFocusable to return false and the traversal to continue past it, potentially stealing focus from the correct element. The new early return short-circuits correctly.
  • Document-level capture listener: Before this fix, the listener was on the element itself, so delegated focus (e.g. <label><input>, shadow host with delegatesFocus) would land on a different element and the handler would never fire, again causing setFocusIfFocusable to return false. Listening on document in the capture phase correctly observes focus wherever it lands.
  • The try/finally pattern ensures the document listener is always cleaned up, even if .focus() throws.
  • Two tests directly cover the already-focused fix; the fieldset test covers general deep traversal. A direct test for label/shadow-host delegation (the motivation for fix React Devtools Component: Add hotkeys to start inspecting node #1) is missing from the new suite.

Confidence Score: 4/5

  • This PR is safe to merge; both fixes are minimal, well-scoped, and correctly bounded by cleanup logic.
  • The implementation logic is sound: the early-return and document-level listener independently address the two described bugs, the finally block guarantees listener cleanup, and the existing test suite is expanded. The score is 4 rather than 5 only because no test directly exercises the delegated-focus code path (the scenario that motivated switching to a document-level listener), leaving that specific branch without regression coverage.
  • No files require special attention; the one gap is test coverage for the label/shadow-host delegation scenario in ReactDOMFragmentRefs-test.js.

Important Files Changed

Filename Overview
packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js setFocusIfFocusable gains two targeted fixes: an early return when the element is already the active element, and a document-level capture-phase listener to detect delegated focus events. Both changes are minimal, correctly bounded by try/finally cleanup, and address the described bugs.
packages/react-dom/src/tests/ReactDOMFragmentRefs-test.js Three new test cases are added for fragment focus behavior. Two tests cover the "already focused" fix directly. The fieldset test covers general deep traversal. A direct test for the label-delegation scenario (fix #1) is absent, relying on the fieldset case as a proxy.

Fix All in Claude Code Fix All in Codex

Last reviewed commit: ea8b0fb

Comment on lines +305 to +336
<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>

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

@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Mar 12, 2026
@everettbu
everettbu deleted the setfocusfixes branch March 12, 2026 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants