fix: #8686 prevent useGridCell from overriding child focus restoration loops by jsmitrah · Pull Request #10228 · adobe/react-spectrum

jsmitrah · 2026-06-18T10:24:53Z

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Tab into the table row and use the arrow keys to focus on the "Open Dialog" button.
Press Enter to open the modal dialog, then press Escape to close it.
Verify that focus returns exactly to the "Open Dialog" button instead of jumping to the "Action 1" button.

🧢 Your Project:

snowystinger

Thanks for the interest, if you run yarn format it should get rid of all the noisy whitespace changes that I'm guessing your IDE applied

jsmitrah · 2026-06-19T05:40:45Z

Thanks @snowystinger, all tests are passed

snowystinger · 2026-06-25T22:38:38Z

      return;
    }

-    // If the cell itself is focused, wait a frame so that focus finishes propagatating


This seems like an important comment. JSDOM and the act environment is not a reliable test against this as it's simulating real browsers.
Did you run this against real browsers? If I recall, document.activeElement can be updated at different times depending on the browser you're in as well.

Good catch, @snowystinger. To avoid the timing issues of the document.activeElement across different real browsers, I updated the approach to use e.relatedTarget.

By checking e.relatedTarget synchronously, we can instantly determine if focus is moving directly into a nested child element. This provides identical, reliable accuracy in both JSDOM and real browsers without needing to wait for a frame

I've pushed the update.

I don't see any update with relatedTarget?

Apologies for the confusion, @snowystinger . I initially tried an implementation using e.relatedTarget, but it completely broke the React 16 test suite (test-16) on CircleCI due to synthetic event pooling limitations in older environments.

Because asynchronous timeouts/frames break the synchronous act() testing clock, I reverted to the synchronous approach using getActiveElement(). I have also restored the original comment regarding focus propagation to keep the documentation clear.

Thanks for the explanation. I'm going to be level with you. I have low confidence in this PR. It appears to be AI driven with a focus just on the tests and not on the browser. In addition, it doesn't explain how we've moved away from this requestAnimationFrame in a satisfactory way. The comment still says "wait a frame" which isn't what it's doing. Finally, the test seems fairly contrived as I note in my other comment.

This may very well be the correct solution, but we need a lot more due diligence. Help us help you get this PR merged.
Thank you for understanding.

…focus-to-button

snowystinger · 2026-07-01T02:57:43Z

-      // useSelectableItem only handles setting the focused key when
-      // the focused element is the gridcell itself. We also want to
-      // set the focused key when a child element receives focus.
-      // If focus is currently visible (e.g. the user is navigating with the keyboard),
-      // then skip this. We want to restore focus to the previously focused row/cell
-      // in that case since the table should act like a single tab stop.


Why was this deleted?

"My apologies, @snowystinger! That was completely accidental during a refactor pass in my editor. I have fully restored the original comment block.

snowystinger · 2026-07-01T03:00:58Z

      return;
    }

-    // If the cell itself is focused, wait a frame so that focus finishes propagatating


I don't see any update with relatedTarget?

…focus-to-button

snowystinger · 2026-07-03T01:08:10Z

      return;
    }

-    // If the cell itself is focused, wait a frame so that focus finishes propagatating


Thanks for the explanation. I'm going to be level with you. I have low confidence in this PR. It appears to be AI driven with a focus just on the tests and not on the browser. In addition, it doesn't explain how we've moved away from this requestAnimationFrame in a satisfactory way. The comment still says "wait a frame" which isn't what it's doing. Finally, the test seems fairly contrived as I note in my other comment.

This may very well be the correct solution, but we need a lot more due diligence. Help us help you get this PR merged.
Thank you for understanding.

snowystinger · 2026-07-03T01:11:17Z

+    // 2. Simulate focus returning directly to the second target element (Switch 2)
+    // exactly how the focus-restoration logic inside an overlay does it.
+    act(() => {
+      switches[1].focus();
+    });
+    expect(document.activeElement).toBe(switches[1]);


what overlay? focus wasn't originally on the second switch so why would it be "restored" there?

it might be better to write these tests in a higher level than the hook tests so that we get integration behaviours, somewhere like react-aria-components/test/Table.test.js

snowystinger · 2026-07-03T01:12:13Z

+    // 3. Fire a focus event directly on the gridcell container to trigger your
+    // onFocus handler in useGridCell.ts and simulate event bubbling
+    act(() => {
+      cells[0].dispatchEvent(new FocusEvent('focus', {bubbles: true}));


focus just happened on the switch, this would be inaccurate

snowystinger · 2026-07-03T01:13:05Z

+      cells[0].dispatchEvent(new FocusEvent('focus', {bubbles: true}));
+    });
+
+    // 4. Force Jest's timer and requestAnimationFrame microtask cycles to execute completely


from the above code changes, why is this still needed?

…focus-to-button

…tton' of https://github.com/jsmitrah/react-spectrum into bug-fix/8686/table-cell-incorrectly-returns-focus-to-button

…focus-to-button

snowystinger · 2026-07-13T08:43:49Z


 let renderTable = props => render(<TestTable {...props} />);

+if (typeof Symbol.dispose === 'undefined') {


what is this?

I tested the fix on different machines that have different Node versions (20.19.3) where this test was failing. I added these lines only to make the test work across those machines. Now I've removed them to keep the PR clean.

snowystinger · 2026-07-13T08:51:27Z

+        // e.g. because an overlay closed and removed the element that had focus. In
+        // that case, restore the child that was last focused instead of defaulting to
+        // childFocusStrategy's first/last child.
+        let ownerDocument = ref.current.ownerDocument;


It seems like there have been some new changes since I last looked at this PR. What prompted these changes? Is it different from the original issue? The comments don't really explain. What's the real world case that caused this? There's a force push, so I can't follow the commits as a train of thought either to see when it was added.

After testing the previous fix in a real browser, I found the issue was still happening after opening the dialog and closing it; focus went back to the first button instead of the button that was previously focused. So I have handled this in a different approach to fix that

snowystinger · 2026-07-13T08:53:09Z

+      // actual DOM descendants of the cell -- portalled content (e.g. a dialog opened
+      // from within the cell) can still reach this handler because React dispatches
+      // events along the component tree rather than the DOM tree for portals, even
+      // though it isn't really inside this cell in the DOM.


I'm not sure what the portalled comment is about? the line below nodeContains(ref.current, target) is a guard for portals

Yes, the nodeContains(ref.current, target) check prevents that. It ensures we remember only elements that belong to this cell, so focus is restored to the correct button when the dialog closes.

snowystinger · 2026-07-13T08:54:08Z


-    // If the cell itself is focused, wait a frame so that focus finishes propagatating
-    // up to the tree, and move focus to a focusable child if possible.
-    requestAnimationFrame(() => {


I still don't see an explanation for this change

Keeping requestAnimationFrame doesn't really solve the issue. It still depends on timing, which can behave differently across browsers. That was one of the main concerns from the beginning. It also makes the tests more complicated because they have to wait for the delayed code to run, and now, with the new approach, the delay is no longer needed. The code already knows which element should receive focus, so it can restore it immediately instead of waiting; keeping requestAnimationFrame would only add extra complexity without providing any real benefit. That's why I chose the synchronous approach.

Connecting comment explaining where the raf may have originally come from that we'll want to verify against.
#10159 (comment)

I'm not sure what verifying it looks like yet.

@snowystinger and @LFDanLu, I looked into the focusedKey concern first. The autofocus children behavior depends on focusedKey being updated correctly, so I tracked that flow and confirmed it is the focusMode behavior used for checkbox, drag handle, and menu cells when clicking the "tab" button.

I reran the existing tests after removing requestAnimationFrame, and they all pass, including the test that checks focus moves directly to the next row's checkbox with ArrowDown, confirming focusedKey is still updated correctly.

To verify the behavior in a real browser, I also attached a sample video that reproduces how focusedKey behaves during the same keyboard navigation flow.

@LFDanLu Please check this.

table-cell-focus.mp4

I reran the existing tests after removing requestAnimationFrame, and they all pass, including the test that checks focus moves directly to the next row's checkbox with ArrowDown, confirming focusedKey is still updated correctly.

Yeah, unfortunately in unit tests we have pretty tight control over time, so I wouldn't expect to see this surface there. This will require manual testing. We'll probably have to try building the codebase just before that requestAnimationFrame was added or right around when it was added but specifically remove that delay and see if we can reproduce any issues there so we know what to look out for since there aren't any instructions.

After digging into it a bit more, I realized I was mixing two different things.

RAF was originally added to let the current focus event finish before moving focus to a child. That helps avoid timing issues during focus handling and gives other focus-related logic, like updating focusedKey, a chance to complete first.

I initially removed it because I thought the timing was related to the issue. After tracing the flow, I found the real problem was that focus() didn't remember which child had focus before. When focus was restored, it always defaulted to the first child.

So I've kept RAF as it was originally. The fix is now in the focus restoration logic: focus() remembers the last focused child and restores focus to that element when focus is disrupted. This fixes the original issue without changing the existing RAF behavior.

I have committed my changes without changing the RAF. Please take a look

…focus-to-button

snowystinger

react-spectrum/packages/react-aria/src/selection/useSelectableCollection.ts

Line 700 in 6279555

    
           // Intercept FocusScope restoration since virtualized collections can reuse DOM nodes.

This is the reason that FocusScope can't just restore focus to the original target. Instead, it sets the manager to focused and the useGridCell then kicks in and focuses itself according to the rules for that type of item.

snowystinger · 2026-08-06T06:51:10Z

+        // Focus counts as "disrupted" (as opposed to a fresh keyboard entry from a
+        // sibling row/cell) when it's on the cell itself or fell back to body/nothing,
+        // e.g. because an overlay closed and removed the element that had focus. In
+        // that case, restore the child that was last focused instead of defaulting to
+        // childFocusStrategy's first/last child.


This isn't really saying anything

Suggested change

// Focus counts as "disrupted" (as opposed to a fresh keyboard entry from a

// sibling row/cell) when it's on the cell itself or fell back to body/nothing,

// e.g. because an overlay closed and removed the element that had focus. In

// that case, restore the child that was last focused instead of defaulting to

// childFocusStrategy's first/last child.

Removed it.

snowystinger · 2026-08-06T06:54:59Z

+        // that case, restore the child that was last focused instead of defaulting to
+        // childFocusStrategy's first/last child.
+        let ownerDocument = getOwnerDocument(ref.current);
+        let isDisrupted =


Suggested change

let isDisrupted =

let shouldRestoreToLastFocused =

when is it on the cell? wouldn't that mean that Tabbing into a cell would re-focus whatever the last focused element was instead of starting with the first focusable?

I've removed that check and renamed the variable to shouldRestoreToLastFocused, so focus is restored only when it moves to document.body

snowystinger · 2026-08-06T07:01:19Z

+
+      // Remember which child was focused so that if something later forces focus
+      // back to this cell (e.g. a closing overlay), focus() can restore it directly
+      // instead of falling back to the first/last focusable child. Only do this for
+      // actual DOM descendants of the cell -- portalled content (e.g. a dialog opened
+      // from within the cell) can still reach this handler because React dispatches
+      // events along the component tree rather than the DOM tree for portals, even
+      // though it isn't really inside this cell in the DOM.


Suggested change

// Remember which child was focused so that if something later forces focus

// back to this cell (e.g. a closing overlay), focus() can restore it directly

// instead of falling back to the first/last focusable child. Only do this for

// actual DOM descendants of the cell -- portalled content (e.g. a dialog opened

// from within the cell) can still reach this handler because React dispatches

// events along the component tree rather than the DOM tree for portals, even

// though it isn't really inside this cell in the DOM.

Removed the comment line.

snowystinger · 2026-08-06T07:03:04Z

    expect(document.activeElement).toBe(tree.getAllByRole('gridcell')[0]);
  });
+
+  it('should restore focus to the child that was last focused within a cell, not the first child', async () => {


Add this test to RAC

it('should restore focus to the associated dialog trigger inside a cell', async () => { let tree = render( <> <button>Before</button> <Table aria-label="Files" selectionMode="multiple"> <MyTableHeader> <MyColumn id="name" isRowHeader> Name </MyColumn> <MyColumn>Type</MyColumn> <MyColumn>Date Modified</MyColumn> </MyTableHeader> <TableBody> <MyRow id="1" textValue="Games"> <Cell>Games</Cell> <Cell>File folder</Cell> <Cell> <DialogTrigger> <Button>Open 1</Button> <Modal> <Dialog> <Checkbox> <Label>Agree 1</Label> </Checkbox> </Dialog> </Modal> </DialogTrigger> <DialogTrigger> <Button>Open 2</Button> <Modal> <Dialog> <Checkbox> <Label>Agree 2</Label> </Checkbox> </Dialog> </Modal> </DialogTrigger> <DialogTrigger> <Button>Open 3</Button> <Modal> <Dialog> <Checkbox> <Label>Agree 3</Label> </Checkbox> </Dialog> </Modal> </DialogTrigger> </Cell> </MyRow> </TableBody> </Table> <button>After</button> </> ); await user.click(tree.getByRole('button', {name: 'Open 2'})); let dialog = tree.getByRole('dialog'); expect(document.activeElement).toBe(dialog); await user.keyboard('{Escape}'); act(() => jest.runAllTimers()); expect(dialog).not.toBeInTheDocument(); expect(document.activeElement).toBe(tree.getByRole('button', {name: 'Open 2'})); });

Added this in RAC, and the test file passed.

…focus-to-button

snowystinger

Looks like the "file mode" of packages/react-aria/test/grid/useGrid.test.js was changed, please revert that. Likely it was made executable or something

…focus-to-button

jsmitrah · 2026-08-12T06:20:47Z

I’ve reverted the file mode

snowystinger reviewed Jun 18, 2026

View reviewed changes

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch 5 times, most recently from 0a7cf3b to 714723c Compare June 18, 2026 15:09

jsmitrah requested a review from snowystinger June 19, 2026 05:35

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from ccb8bbd to 4b28c4a Compare June 19, 2026 08:22

jsmitrah added 2 commits June 24, 2026 16:23

fix: prevent useGridCell from overriding child focus restoration loops

11a8867

fix: resolve lint and CI test compilation errors

7e8bcfe

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from 71950d3 to 22b6e5f Compare June 24, 2026 10:54

snowystinger reviewed Jun 25, 2026

View reviewed changes

changed the type for focus event.

adbba17

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from b06221e to adbba17 Compare June 26, 2026 14:56

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

bce1556

…focus-to-button

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from 6ab9b4c to bce1556 Compare June 26, 2026 15:21

snowystinger reviewed Jul 1, 2026

View reviewed changes

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from 535198e to 244fa42 Compare July 2, 2026 13:32

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

24667bf

…focus-to-button

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from 244fa42 to 24667bf Compare July 2, 2026 13:39

jsmitrah requested a review from snowystinger July 2, 2026 13:50

snowystinger reviewed Jul 6, 2026

View reviewed changes

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

e9c5635

…focus-to-button

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from ac51242 to 217b6bb Compare July 10, 2026 13:45

fix: improve button interaction handling.

cf9fb80

jsmitrah force-pushed the bug-fix/8686/table-cell-incorrectly-returns-focus-to-button branch from 3507110 to cf9fb80 Compare July 10, 2026 13:57

jsmitrah and others added 3 commits July 10, 2026 19:28

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

6a6f158

…focus-to-button

chore: clear remote build cache

70b8a96

Merge branch 'bug-fix/8686/table-cell-incorrectly-returns-focus-to-bu…

04dda2d

…tton' of https://github.com/jsmitrah/react-spectrum into bug-fix/8686/table-cell-incorrectly-returns-focus-to-button

jsmitrah requested a review from snowystinger July 10, 2026 14:12

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

ce7ac24

…focus-to-button

snowystinger reviewed Jul 13, 2026

View reviewed changes

snowystinger mentioned this pull request Jul 15, 2026

feat: support Textfields and other interactive components in Table #10159

Merged

5 tasks

jsmitrah and others added 2 commits July 15, 2026 12:29

cleaned the test file.

480b06f

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

0cf561f

…focus-to-button

jsmitrah requested a review from snowystinger July 15, 2026 11:54

jsmitrah and others added 12 commits July 16, 2026 12:47

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

4228eb9

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

d5fe949

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

eb1152a

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

11f3ad3

…focus-to-button

modified the fix without removing the RAF.

4bb3a9d

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

75b2056

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

983bad4

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

e3b8dc3

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

e6fac10

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

8e6faf0

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

b04e589

…focus-to-button

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

e113f83

…focus-to-button

snowystinger reviewed Aug 6, 2026

View reviewed changes

jsmitrah and others added 3 commits August 10, 2026 17:14

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

4328795

…focus-to-button

Worked on PR comments.

3d00705

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

11857eb

…focus-to-button

jsmitrah requested a review from snowystinger August 11, 2026 06:27

snowystinger reviewed Aug 11, 2026

View reviewed changes

jsmitrah and others added 3 commits August 12, 2026 11:28

Reverted the useGrid.test.js file.

801acd2

Merge branch 'main' into bug-fix/8686/table-cell-incorrectly-returns-…

7b0b5e1

…focus-to-button

resolved lint

49ded82

jsmitrah requested a review from snowystinger August 12, 2026 06:20


		let renderTable = props => render(<TestTable {...props} />);

		if (typeof Symbol.dispose === 'undefined') {

Uh oh!

Conversation

jsmitrah commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

jsmitrah commented Jun 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmitrah Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmitrah Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmitrah commented Jun 18, 2026 •

edited

Loading

jsmitrah Jun 26, 2026 •

edited

Loading

snowystinger Jul 3, 2026 •

edited

Loading

snowystinger Jul 3, 2026 •

edited

Loading

jsmitrah Jul 15, 2026 •

edited

Loading