fix(#2444): useRestoreFocus when nodeToRestore has been unmounted #2445

majornista · 2021-10-13T14:55:04Z

Restore focus to previous nodeToRestore when expected nodeToRestore is no longer present, as with a Menu that opens a Dialog.

Closes #2444

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue FocusScope fails to restore focus when the element to restore has unmounted #2444.
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:

Open https://reactspectrum.blob.core.windows.net/reactspectrum/4b09194340f607fab1c56739c71f99e8a7d58337/storybook/index.html?path=/story/dialogcontainer--in-a-menu
Click or navigate to the "Open menu" button and press Enter or Space to expand the menu.
Click or arrow down to and activate the "Open dialog..." menu item.
Press Esc key to close the Dialog.
Focus should be restored to the "Open menu" button.
Open https://reactspectrum.blob.core.windows.net/reactspectrum/4b09194340f607fab1c56739c71f99e8a7d58337/storybook/index.html?path=/story/dialogcontainer--nested-dialog-containers
Click or navigate to the "Open menu" button and press Enter or Space to expand the menu.
Click or arrow down to and activate the "Do this..." or "Do that..." menu item.
Click or activate the "Do that" or "Do this" button to replace the Dialog with the nested Dialog.
Repeat step 4 a few times for good measure.
Press Esc key, or use the "Dismiss" button to close the Dialog.
Focus should be restored to the "Open menu" button.

Verify that https://reactspectrum.blob.core.windows.net/reactspectrum/4b09194340f607fab1c56739c71f99e8a7d58337/storybook/index.html?path=/story/focusscope--keyboard-navigation and https://reactspectrum.blob.core.windows.net/reactspectrum/4b09194340f607fab1c56739c71f99e8a7d58337/storybook/index.html?path=/story/focusscope--keyboard-navigation-inside-portal manage and restore focus appropriately.

🧢 Your Project:

Adobe/Accessibility

packages/@react-aria/focus/src/FocusScope.tsx

Per #2444: Restore focus to previous nodeToRestore when expected nodeToRestore is no longer present, as with a Menu that opens a Dialog. fix(#2444): allow restoreFocus prop to specify a ref to receive focus fix(#2444): update Dialog to support restoreFocus prop, add stories and tests

…esent Per code comment: #2445 (comment)

adobe-bot · 2022-06-04T00:08:41Z

Build successful! 🎉

snowystinger · 2022-06-07T23:40:52Z

packages/@react-spectrum/picker/test/Picker.test.js

      triggerPress(item3);
      expect(onSelectionChange).toHaveBeenCalledTimes(1);
      act(() => jest.runAllTimers());
+      act(() => jest.runAllTimers());


note: this test used to be technically wrong, after the first runAllTimers here, focus was on the body, we never ran the queued restore focus which is queued up as a result of the render contained in the act.

Because we now have a stack of nodes to restore focus to, we need to make sure to run our cleanup in the raf to clear that stack out, otherwise we have extra elements in the stack that will cause problems further down in the test

it's the same for all other tests here where it's runalltimersx2

So to clarify, where I now have act(() => {jest.runAllTimers();}); twice is appropriate for this case? I saw instances where runAllTimers was called twice, in ActionBar.test and Picker.test, and figured I'd give it a try to fix some failing tests here. Sure enough, it worked.

snowystinger · 2022-06-07T23:43:27Z

packages/@react-spectrum/overlays/stories/Modal.stories.tsx

      <DialogTrigger type="popover" isOpen={isPopoverOpen} onOpenChange={setPopoverOpen}>
        <ActionButton>Open popover</ActionButton>
-        <Dialog>
+        <Dialog isDismissable={false}>


these should already be false? what are these changes for?

The responsive Popover hides the footer containing the "Open modal" button when rendered as a Dialog which defaults to isDismissible={true}, making it difficult to test if your screen width is too narrow. I should probably open a separate issue for this.

snowystinger · 2022-06-07T23:54:39Z

packages/@react-aria/focus/src/FocusScope.tsx

+            }
+            return;
+          }
+          // Otherwise try to focus the nodeToRestore, and clean up


i don't quite understand this case, can you explain when we'd hit this?

We have to account for FocusScopes where contain={false}, as @LFDanLu pointed out in this comment: #2445 (review)

I'll need to dig into it a some more but the following story breaks in the following case:

Open the dialog with you keyboard

Open several layers of nested dialogs via the "open dialog" buttons in each new dialog

Shift + Tab back to the first "dialog" you opened and hit the "close" button with the enter key

Note that focus is lost instead of being restored to the first "open dialog" button

The above doesn't reproduce in an earlier version of this PR: https://reactspectrum.blob.core.windows.net/reactspectrum/c465126e8286623894aca7b692cb096121643211/storybook/index.html?path=/story/focusscope--keyboard-navigation-no-contain

EDIT:
Seems to be due to something in a2fad80

We iterate backward through the nodeToRestoreArray using pop() only when the nodeToRestore for the closing scope is the last item in the array.

snowystinger · 2022-06-09T00:14:02Z

I looked into this with @devongovett , we came to the conclusion a stack, while close, isn't quite right. We want to try refactoring our Map scopes (which is currently a poor man's tree) into a real, doubly linked, tree. This way we can have sibling focus scopes and they won't try to restore to each other depending on the order they were added/removed. We also noticed that our Map isn't always up to date currently.

adobe-bot · 2022-06-09T16:25:32Z

Build successful! 🎉

majornista · 2022-06-09T19:53:50Z

I looked into this with @devongovett , we came to the conclusion a stack, while close, isn't quite right. We want to try refactoring our Map scopes (which is currently a poor man's tree) into a real, doubly linked, tree. This way we can have sibling focus scopes and they won't try to restore to each other depending on the order they were added/removed. We also noticed that our Map isn't always up to date currently.

@devongovett and @snowystinger While I agree that refactoring the restoreFocus behavior in FocusScopes around the composite pattern is desirable, please consider that this PR has been open since October 2021, ~7 months. Provided it fixes the problem and does not introduce breaking changes, I think it would be better to accept the PR and address the more significant refactoring as a separate issue.

adobe-bot · 2022-06-27T14:37:14Z

Build successful! 🎉

adobe-bot · 2022-07-12T16:06:32Z

Build successful! 🎉

adobe-bot · 2022-07-14T16:45:41Z

Build successful! 🎉

adobe-bot · 2022-07-20T15:23:46Z

Build successful! 🎉

adobe-bot · 2022-08-04T22:34:20Z

Build successful! 🎉

devongovett · 2022-08-08T20:02:55Z

Fixed by #3323

majornista requested review from dannify, devongovett and snowystinger as code owners October 13, 2021 14:55