Fix some cases where FocusScope focus restoration and tabbing misbehaves #2416

kherock · 2021-10-02T20:22:25Z

Closes #2415

✅ 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:

The unit tests added in this PR are hopefully self explanatory, let me know if I need to clarify anything else!

🧢 Your Project:

N/A

kherock · 2021-10-04T16:43:17Z

There's actually one more issue I'm facing with FocusScope, but it actually seems to be by design?

react-spectrum/packages/@react-aria/focus/test/FocusScope.test.js

Lines 432 to 448 in e1fc06c

    
           it('should move focus to the previous element after the previously focused node on Shift+Tab', function () { 
        
             function Test({show}) { 
        
               return ( 
        
                 <div> 
        
                   <input data-testid="before" /> 
        
                   <button data-testid="trigger" /> 
        
                   <input data-testid="after" /> 
        
                   {show && 
        
                     <FocusScope restoreFocus autoFocus> 
        
                       <input data-testid="input1" /> 
        
                       <input data-testid="input2" /> 
        
                       <input data-testid="input3" /> 
        
                     </FocusScope> 
        
                   } 
        
                 </div> 
        
               ); 
        
             }

Here, after seems like the logical target for moving focus backwards since that's the native behavior. FocusScope's implementation says that Shift+Tabing out of the scope should focus the element before the trigger, which in general feels incorrect. There's an annoying bug that happens in this case:

          <div>
            <button data-testid="trigger" />
            {show &&
              <FocusScope restoreFocus autoFocus>
                <input data-testid="input1" />
                <input data-testid="input2" />
                <input data-testid="input3" />
              </FocusScope>
            }
            <input data-testid="after" />
          </div>

The current implementation says that Shift+Tabing out of the scope should focus the element before the trigger. No element is defined, so the FocusScope overrides the native behavior of focusing the trigger and blurs the active element.

kherock · 2021-10-04T17:06:57Z

~~Could I suggest refactoring the current code dealing with focus restoration to only run when some element anchoring it to the tab order is provided? e.g.~~

          <div>
            <input data-testid="before" />
            <button data-testid="trigger" />
            <input data-testid="after" ref={focusDomRef} /> 
            {show &&
              <OverlayContainer>
                <FocusScope domRef={focusDomRef} restoreFocus autoFocus>
                  <input data-testid="input1" />
                  <input data-testid="input2" />
                  <input data-testid="input3" />
                </FocusScope>
              </OverlayContainer>
            }
          </div>

~~Here, domRef would be documented along the lines of~~

The position in the DOM where this component's descendants should be inserted into the tab order. This has no effect when focus is contained.

~~Its default value would be the node to restore if restoreFocus is enabled.~~

This way, when the domRef prop is omitted, developers can use the intuitive (in most cases) behavior of native tab ordering. Then in more complicated cases such as the one above, they can provide a ref to the position where they'd like focus to be restored when focus leaves the scope.

edit: a prototype of this is implemented in #2459

kherock · 2021-10-23T20:05:45Z

@LFDanLu any feedback on these changes in particular?

LFDanLu

Apologies for the late review, here are a couple things I noticed with the behavior if contain is removed. Behavior with contain seems pretty good though

LFDanLu · 2021-10-25T18:58:11Z

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

+      } else if (domRef?.current) {
+        walker.currentNode = domRef.current;
+
+        // Skip over elements within the scope, in case the scope immediately follows the domRef.
+        do {
+          nextElement = (e.shiftKey ? walker.previousNode() : walker.nextNode()) as HTMLElement;
+        } while (isElementInScope(nextElement, scope));


Was testing this locally and noticed that for FocusScope w/ contain=false and restoreFocus, shift tabbing in a FocusScope would always cause the focus to leave the FocusScope, even if there are preceeding elements to focus in the FocusScope. You can reproduce this via removing the contain here and testing the FocusScope stories in the storybook.

Should this conditional for this block check if nextElement is an element in scope and if so proceed to line 227 instead?

Yep, good catch. Things behave nearly as expected for nested scopes now. However, there's a quirk with the focus scope stories where getScopeRoot resolves to the same element for all nested scopes due to them rendering as siblings:

The causes tabbing between children of nested scopes to behave as if they were all part of the same scope. Should I create a new variant of the story for testing when the focus scopes nest within the DOM?

Ah yes, I see what you are talking about, makes those stories pretty confusing since I was expecting tabbing out of the child dialog to move focus to the input after the trigger button in the parent dialog. Yeah I think it would be a good idea to create a separate set of stories that nests the "dialogs" in a parent/child fashion rather than siblings. Mind creating adding a toggle in those stories/creating another set of stories that removes the contain from the FocusScope as well so it is easier to compare and contrast the behavior?

Actually the nested scopes only render as siblings for the "Keyboard Navigation Inside Portal" right? The "Keyboard Navigation" story renders the nested scopes in a nested fashion within the DOM so why is scope in useTabOrderSplice returning the entire div tree (all FocusScopes) and not just the child dialog FocusScope?

I think some heavier refactoring is needed here. I added a short-circuit when e.defaultPrevented is true, but this is causing only the parent focus scope to run their handler. The reason why scope contains the entire div is because the parent handler runs first and it calls e.preventDefault() before it exits.

Contained scopes behave fine because the parent short-circuits when contain && scope !== containedScope fails.

Without the e.defaultPrevented check for uncontained scopes, the keydown handler runs once for every level of nesting. You can test this by removing the check and creating a dialog with two levels of nesting. A dialog nested 2 levels deep will skip 1 element for each tab press. We need a way to ensure that the parent scope handlers don't run if a child scope can handle the event.

LFDanLu · 2021-10-25T19:06:53Z

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

-  useAutoFocus(scopeRef, autoFocus);
+  useAutoFocus(scopeRef.current, autoFocus);
+  let nodeToRestoreRef = useRestoreFocus(scopeRef.current, restoreFocus);
+  useTabOrderSplice(scopeRef.current, contain, nodeToRestoreRef);


nitpick:
Can we keep the useFocusContainment name here?

I can revert, but I do think there's a better name out there for it since the hook now handles some of the tab ordering stuff that useRestoreFocus was doing before.

That's true, I personally still lean towards useFocusContainment. Names are hard though, so I'm fine waiting for a third opinion.

LFDanLu · 2021-10-25T19:25:35Z

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

+    if (contain) {
+      document.addEventListener('focusin', onFocus, false);
+      document.addEventListener('focusout', onBlur, false);
+    }


Do we really only want to add these listeners if contain is true? Won't this mean we don't properly update the containedScope if focus moves into a child FocusScope?

I'm seeing an issue locally where you can't tab forward through the child dialog's element in the FocusScope storybook "Keyboard Navigation Inside Portal" story if you remove contain from the FocusScope.
Repro steps:

Remove contain from https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/focus/stories/FocusScope.stories.tsx#L49

Go to the "Keyboard Navigation Inside Portal" story. Open the first dialog via Enter on "open dialog"

Tab to the next "open dialog" button and open the child dialog.

Attempt to tab through the child dialog element. Note that it moves focus to the close button of the first dialog

I'm confident that those listeners should only be added when contain is true as the previous version of uncontained code only relied on keyboard events:

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

Lines 451 to 458 in bd8c099

if (!contain) {

document.addEventListener('keydown', onKeyDown, true);

}

return () => {

if (!contain) {

document.removeEventListener('keydown', onKeyDown, true);

}

Additionally, the implementations of onFocus and onBlur don't have additional responsibility outside of containing focus.

The issue you found above is a tough case. However, it's technically behaving as intended, and I think altering this behavior in any way will break unit tests. Though I was missing an isElementInScope check which was causing asymmetric behavior with shift+tab. I'll try to illustrate:

<x> = nodeToRestore >x< = focusedElement /* case: one dialog */ 1 <2> 3 4 >5< /* * Pressing tab will exit focus from the scope and past the end of the document. We intercept * this and move focus to the element immediately following <2>, which is 3. We have a loop * preventing focus from re-entering in this manner, and it will continue iterating until we * focus the element following >5<. This is the end of the document body, so it ultimately wraps * to focus 1. * * Shift+tabbing from >3< moves focus to the element preceding <2>, so focus also moves to 1 as expected * (this was broken during the initial review). */ /* case: two dialogs, nested */ 1 <2> 3 <4> 5 6 7 >8< /* * Pressing tab now will again attempt to move focus past the end of the document. We * focus the element immediately following <4>. So 5 receives focus, and this matches the * behavior seen where the close button is focused after tabbing out of the innermost dialog. */

I'm confident that those listeners should only be added when contain is true

Yep, you're right, I overlooked that old useFocusContainment early returns when contain was false and thus never adds focusin/focusout for the elements in scope.

Pressing tab will exit focus from the scope and past the end of the document. We intercept
this and move focus to the element immediately following [2], which is 3. We have a loop
preventing focus from re-entering in this manner, and it will continue iterating until we
focus the element following {5}. This is the end of the document body, so it ultimately wraps
to focus 1.

Yeah this is a strange case for sure, will need to discuss with the team on what the correct behavior here should be (We definitely wouldn't want to focus element 3 but it IS technically the element that follows the nodeToRestore...)

kherock

Appreciate the feedback, thank you. I rebased and and fixed the shift+tab behavior.

I compared behavior of the FocusScope stories to main and there's some slightly different behavior where tabbing past the end of the innermost dialog blurs and focuses the body. My branch focuses the element following the trigger of the parent FocusScope. This feels more correct, but let me know if the old behavior is intended.

kherock · 2021-10-26T03:18:13Z

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

+      } else if (domRef?.current) {
+        walker.currentNode = domRef.current;
+
+        // Skip over elements within the scope, in case the scope immediately follows the domRef.
+        do {
+          nextElement = (e.shiftKey ? walker.previousNode() : walker.nextNode()) as HTMLElement;
+        } while (isElementInScope(nextElement, scope));


Yep, good catch. Things behave nearly as expected for nested scopes now. However, there's a quirk with the focus scope stories where getScopeRoot resolves to the same element for all nested scopes due to them rendering as siblings:

The causes tabbing between children of nested scopes to behave as if they were all part of the same scope. Should I create a new variant of the story for testing when the focus scopes nest within the DOM?

kherock · 2021-10-26T03:33:22Z

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

+    if (contain) {
+      document.addEventListener('focusin', onFocus, false);
+      document.addEventListener('focusout', onBlur, false);
+    }


I'm confident that those listeners should only be added when contain is true as the previous version of uncontained code only relied on keyboard events:

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

Lines 451 to 458 in bd8c099

if (!contain) {

document.addEventListener('keydown', onKeyDown, true);

}

return () => {

if (!contain) {

document.removeEventListener('keydown', onKeyDown, true);

}

Additionally, the implementations of onFocus and onBlur don't have additional responsibility outside of containing focus.

kherock · 2021-10-26T04:33:04Z

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

+    if (contain) {
+      document.addEventListener('focusin', onFocus, false);
+      document.addEventListener('focusout', onBlur, false);
+    }


The issue you found above is a tough case. However, it's technically behaving as intended, and I think altering this behavior in any way will break unit tests. Though I was missing an isElementInScope check which was causing asymmetric behavior with shift+tab. I'll try to illustrate:

<x> = nodeToRestore >x< = focusedElement /* case: one dialog */ 1 <2> 3 4 >5< /* * Pressing tab will exit focus from the scope and past the end of the document. We intercept * this and move focus to the element immediately following <2>, which is 3. We have a loop * preventing focus from re-entering in this manner, and it will continue iterating until we * focus the element following >5<. This is the end of the document body, so it ultimately wraps * to focus 1. * * Shift+tabbing from >3< moves focus to the element preceding <2>, so focus also moves to 1 as expected * (this was broken during the initial review). */ /* case: two dialogs, nested */ 1 <2> 3 <4> 5 6 7 >8< /* * Pressing tab now will again attempt to move focus past the end of the document. We * focus the element immediately following <4>. So 5 receives focus, and this matches the * behavior seen where the close button is focused after tabbing out of the innermost dialog. */

kherock · 2021-10-26T04:43:36Z

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

-  useAutoFocus(scopeRef, autoFocus);
+  useAutoFocus(scopeRef.current, autoFocus);
+  let nodeToRestoreRef = useRestoreFocus(scopeRef.current, restoreFocus);
+  useTabOrderSplice(scopeRef.current, contain, nodeToRestoreRef);


I can revert, but I do think there's a better name out there for it since the hook now handles some of the tab ordering stuff that useRestoreFocus was doing before.

LFDanLu · 2021-10-26T23:38:47Z

@kherock

I compared behavior of the FocusScope stories to main and there's some slightly different behavior where tabbing past the end of the innermost dialog blurs and focuses the body. My branch focuses the element following the trigger of the parent FocusScope. This feels more correct, but let me know if the old behavior is intended.

From my understanding of how the FocusScopes should work, I believe it should focus the element following the trigger of the parent FocusScope. The behavior of the first FocusScope story on main w/ contain removed from the FocusScope seems wrong to me, lemme bring it up with the team

EDIT: To be more specific, the behavior on main is focusing the body because there isn't an element following the first trigger button that isn't an element within the FocusScope. Adding an input element after the first trigger button rectifies this behavior. Perhaps focusing the body in that specific case (contain=false and no following element after the trigger) is correct since that would allow the user to tab out of the window and into the browser tabs hmm...

kherock · 2021-10-27T21:22:51Z

It didn't occur to me that preventing focus from leaving the window into the browser's chrome is potentially inaccessible. I feel like there should be some special handling for how tabbing past the ends of the a focus scope should work:

when focus is contained, should tabbing past the last element allow focus to move outside of the window?
- I think "no" is an acceptable answer provided that containment is released once Esc is pressed
when focus isn't contained, how do we ensure focus properly leaves the window if the the scope's trigger is the first/final element on the page?

LFDanLu

Thanks for the updates, behavior seems to be correct to me, definitely much more consistent. I've gone ahead and added some stories for testing the original restoreFocus issue as well as the overall FocusScope refactor, hope you don't mind.

To be quite frank I am a bit concerned about the amount of changes happening here but things seem to work from my testing and the logic seems fine. Will see if we can get a second pair of eyes on this

kherock · 2021-11-20T20:16:50Z

Anything further I can do with this PR? I've been especially in need of the autoFocus fix in these changes.

LFDanLu · 2021-11-22T03:06:27Z

I'll bump the team again, may take some time since we are prepping a release + holiday season.

ktabors

the focus scope in /focusscope--keyboard-navigation-no-contain seem wonky. If I tab I stay (cycle) in the most recent dialog and the previous close button. If I "shift+tab" I go back through the previous "dialogs", starting with the "replace focusscope children", but I can tab forward with that again, until I get to the last dialog and then it cycles on that.

Still not a fan of the contain stories and being able to click on a previous dialog's buttons when the keyboard navigation is contained away from them.

ktabors · 2021-12-16T00:27:13Z

packages/@react-aria/focus/test/FocusScope.test.js

+      expect(document.activeElement).toBe(outside);
+    });
+
+    it('should restore focus to the previously focused node when tabbing away from a child with autoFocus', function () {


based on this description I'd expect the final check on active element to be "outside" not "after".

I feel like all three of these tests should check the activeElement before adding the and checking that the focus is within it since they all say "restore focus".

Yes the phrasing here is off, I'll fix the description. In this case focus is "moving after" rather than "restoring".

And just so I understand you (I think you're missing a word), in this case are you suggesting to check if activeElement is outside before rerender(<Test show />)?

ktabors · 2021-12-16T00:28:06Z

packages/@react-aria/focus/test/FocusScope.test.js

+      let input3 = getByTestId('input3');
+      expect(document.activeElement).toBe(input3);
+
+      userEvent.tab();


Do we need a test that looks at shift+tabbing? I ask because there was a lot of discussion about that case.

Yeah there were a couple times we found regressions that weren't caught by tests, I can add some scenarios for those.

This fixes instances where the contained focus is incorrectly tracked while autoFocus=false or when children change.

beamery-tomht · 2022-01-28T09:56:54Z

Waiting eagerly for this fix! 🥺

njsfield · 2022-01-28T10:10:07Z

We're blocked by this too, keen to see it get released 🙏

LFDanLu · 2022-01-29T00:19:56Z

@kherock Apologies for the long delay, @snowystinger and I have spent some time looking at this today and will be discussing the behavior with another team member early next week. Depending on the outcome of that discussion, we may be looking into splitting this PR so the tab order fixes are separated from the fix for the original linked issue. I know we've been quite tardy in getting to this PR so we'd be happy to perform the code splitting if you'd like.

snowystinger · 2022-02-01T20:08:15Z

Thanks @kherock, we've distilled your changes into this PR #2791
We can discuss the oddities of tabbing in/out of un-contained focus scopes elsewhere if it is causing a problem
We can also discuss the refactor now that we won't be changing behavior
Again, thanks for all your hard work on this!

kherock requested review from dannify, devongovett and snowystinger as code owners October 2, 2021 20:22

kherock mentioned this pull request Oct 4, 2021

Support passing a reference to FocusScope for controlling its tab order #2421

Closed

kherock force-pushed the fix-focus-restore branch from 0e31e96 to dcaeaa6 Compare October 5, 2021 03:03

majornista mentioned this pull request Oct 13, 2021

FocusScope fails to restore focus when the element to restore has unmounted #2444

Closed

kherock force-pushed the fix-focus-restore branch 2 times, most recently from d5d1697 to 04221df Compare October 17, 2021 04:33

LFDanLu reviewed Oct 25, 2021

View reviewed changes

kherock force-pushed the fix-focus-restore branch 2 times, most recently from 44cebf1 to a8c0dcc Compare October 26, 2021 04:55

kherock commented Oct 26, 2021

View reviewed changes

kherock force-pushed the fix-focus-restore branch from a8c0dcc to fd235d0 Compare October 28, 2021 00:00

kherock changed the title ~~Fix focus restore~~ Fix some cases where FocusScope focus restoration and tabbing misbehaves Oct 29, 2021

LFDanLu reviewed Nov 2, 2021

View reviewed changes

ktabors reviewed Dec 16, 2021

View reviewed changes

kherock and others added 5 commits December 18, 2021 09:14

Fix restoring focus after a FocusScope child with autoFocus is mounted

b33eb80

Fix focus restoration when the the active element is a dynamic child

5ecd9c9

Ensure the active scope is correctly initialized and maintained

2ca0df9

This fixes instances where the contained focus is incorrectly tracked while autoFocus=false or when children change.

Simplify and fix logic for uncontained nested scopes

275b0e1

Adding additional contain=false FocusScope stories for testing

7dcf0c2

kherock force-pushed the fix-focus-restore branch from 148aff5 to 588c8c3 Compare December 18, 2021 14:23

test: rephrase uncontained tests scenarios, add shift+tab scenario

d98fa6f

kherock force-pushed the fix-focus-restore branch from 588c8c3 to d98fa6f Compare December 18, 2021 14:28

Merge branch 'main' into fix-focus-restore

385d61e

snowystinger mentioned this pull request Jan 31, 2022

Fix focus scope with changing children #2791

Merged

5 tasks

snowystinger closed this Feb 1, 2022

	if (!contain) {
	document.addEventListener('keydown', onKeyDown, true);
	}

	return () => {
	if (!contain) {
	document.removeEventListener('keydown', onKeyDown, true);
	}

Fix some cases where FocusScope focus restoration and tabbing misbehaves #2416

Fix some cases where FocusScope focus restoration and tabbing misbehaves #2416

Uh oh!

Conversation

kherock commented Oct 2, 2021

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

kherock commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kherock commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kherock commented Oct 23, 2021

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kherock Oct 26, 2021 • 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!

kherock Oct 26, 2021 • 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!

LFDanLu Oct 26, 2021 • 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!

kherock left a comment • 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!

kherock Oct 26, 2021 • 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!

kherock Oct 26, 2021 • 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!

LFDanLu commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kherock commented Oct 27, 2021

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

kherock commented Nov 20, 2021

Uh oh!

LFDanLu commented Nov 22, 2021

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

kherock commented Oct 4, 2021 •

edited

Loading

kherock commented Oct 4, 2021 •

edited

Loading

kherock Oct 26, 2021 •

edited

Loading

kherock Oct 26, 2021 •

edited

Loading

LFDanLu Oct 26, 2021 •

edited

Loading

kherock left a comment •

edited

Loading

kherock Oct 26, 2021 •

edited

Loading

kherock Oct 26, 2021 •

edited

Loading

LFDanLu commented Oct 26, 2021 •

edited

Loading