(RSP-1596 and RSP-1592) Update onFocus and onBlur behavior of FocusScope containment #281

LFDanLu · 2020-03-13T22:06:39Z

Closes https://jira.corp.adobe.com/browse/RSP-1596 and https://jira.corp.adobe.com/browse/RSP-1592

✅ Pull Request Checklist:

Included link to corresponding 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:

Go to DialogTrigger nested modal/dialog story and open the nested modal. Note that you can hit escape to close the top most modal/dialog
Go to the DialogTrigger modal story and try opening the modal, clicking the underlay, and hitting escape key. Note that the modal properly closes

packages/@react-spectrum/dialog/test/DialogTrigger.test.js

snowystinger · 2020-03-14T00:46:24Z

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

  restoreFocus?: boolean,
-  autoFocus?: boolean
+  autoFocus?: boolean,
+  setActiveScope?: boolean


API change?

also, isActiveScope?

replaceScope?

suggesting other names because setActiveScope collides with hook usage in my head, sounds like setState conventions

yep, added this api in since the issue is that the nested Dialog's FocusScope isn't set as the active scope on mount. Then when it attempts to focus the nested dialog, the first FocusScope detects that the element is not in its scope and resets focus back to the button that triggered the nested dialog.

I like isActiveScope, I'll go and use that

I believe the reason autofocus made this work is because it updated the activeScope to be equal to the nested dialog's scopeRef so that when useDialog tries to focus the nested dialog on mount, the focus isn't perceived as a event outside of the active scope, thus it doesn't reset the focus to the previously focused element (i.e. the dialog trigger button)

can we find out why the active scope isn't set properly? we do auto focus the dialog itself, which is inside the scope, so it should handle that.

So the activeScope is only updated in two cases, the first being if autoFocus is set on one of the FocusScope (inapplicable in this case) and the second being from onFocus when a focused element is found in the current scope and that scope is not the active scope. The problematic flow is as follows:

The first dialog is opened and focus is called on it via useDialog useEffect.

Its FocusScope's onFocus is called. Since the dialog is within scope and there isn't an activeScope set, the first dialog's FocusScope is set as the activeScope.

User clicks on the action button to open the nested dialog. onFocus for the first FocusScope is called and it saves the action button as the focusedNode. activeScope doesn't change.

The nested dialog renders and focus is called on it via useDialog useEffect. onFocus for the first FocusScope triggers. It determines that the focus target (the nested dialog) is not in it's scope and since it is the current activeScope it resets focus back to the last focusedNode aka the action button

If the onFocus for the nested Dialog's FocusScope triggered first then the activeScope would get updated appropriately to be the nested Dialog's FocusScope. Is there a way to make it trigger first? Dunno if that was possible hence why I opted for the current solution

Ah I see. I think we need to make child focus scopes take precedence over parent focus scopes. If focus occurs in a child focus scope, that scope should always become the active scope rather than the parent taking back focus.

Mmmm, ok. I'll see if I can check the focus context and set the focus scope based on whether or not there is anything returned (indicating if it is a child focus scope)

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

adobe-bot · 2020-03-16T18:59:36Z

Build successful! 🎉

LFDanLu · 2020-03-16T21:59:27Z

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

+      function ChildComponent(props) {
+        return ReactDOM.createPortal(props.children, document.body);
+      }


Rendered the child FocusScope in a portal to test the case where the child is not contained within the parent FocusScope in the DOM but is still a child of the parent FocusScope (nested Dialog test case basically)

adobe-bot · 2020-03-16T22:03:11Z

Build successful! 🎉

devongovett · 2020-03-18T03:57:56Z

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

  }, [children]);

+  if (prevContext) {
+    activeScope = scopeRef;


Not sure this will work correctly in all cases. The FocusScope might be re-rendered while a child scope is active, and take focus away. I think this needs to be done in the focus handler. The active scope needs to not take focus back if the element being focused is in a different scope.

re your earlier comment about order of events, if we attached the focusin/focusout events at the scope level rather than the document level, we could rely on bubbling to handle the order. Inner scopes would receive events first and we could stopPropagation() to prevent them being handled in outer scopes. Also, if events occurred completely outside the scope (e.g. portals), non-active scopes wouldn't even receive events.

We may also need to track not only the active scope but a set of all currently mounted scopes. Then, in the blur event, we could determine if the target was within another scope and ignore it in that case. If the event occurred outside any scope at all (e.g. the body), only then would we move focus back to the active scope.

In this dialog scenario:

The child dialog would mount and focus itself.

The previous scope would handle the focusout event, and check if the related target is outside any other scope. In this case it's not, so it would do nothing and allow the focus to move to the child scope. Otherwise, it would restore focus to itself.

The new focus scope would handle the focusin event on its node, make itself the active focus scope, and stop propagation to any parent scopes.

Let me know if this makes sense. We can chat about it over vc if needed.

adobe-bot · 2020-03-18T21:47:59Z

Build successful! 🎉

adobe-bot · 2020-03-18T22:09:41Z

Build successful! 🎉

LFDanLu · 2020-03-18T22:11:50Z

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

  let focusedNode = useRef<HTMLElement>();

  useEffect(() => {
+    let scope = scopeRef.current;


Did this cuz lint warning regarding hooks

devongovett · 2020-03-19T01:39:15Z

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


 let activeScope: RefObject<HTMLElement[]> = null;
+let counter = 0;
+let scopesMap: Map<number, RefObject<HTMLElement[]>> = new Map();


You could use a Set for this instead of a Map since it doesn't look like you're using the keys anywhere. Then you could get rid of the counter too.

devongovett · 2020-03-19T01:43:56Z

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

-        }
+      if (!isInAnyScope) {
+        activeScope = scopeRef;
+        focusedNode.current = e.target;


Looks like you could get rid of focusedNode entirely. It's not used anywhere else.

using it now for the document level focusin events, thanks for the catch

devongovett · 2020-03-19T01:58:27Z

One more issue I noticed while testing: if you open a dialog, move focus to the browser location bar, and then tab back into the page (past all the chrome toolbar items), it will focus the input behind the dialog. It used to restore focus to the previously focused node in the active scope. We may still need a document level focusin handler to detect that. If a focus event occurs outside the active scope, then move it back into the active scope.

majornista · 2020-03-19T15:28:30Z

One more issue I noticed while testing: if you open a dialog, move focus to the browser location bar, and then tab back into the page (past all the chrome toolbar items), it will focus the input behind the dialog. It used to restore focus to the previously focused node in the active scope. We may still need a document level focusin handler to detect that. If a focus event occurs outside the active scope, then move it back into the active scope.

In v2 this was one of the rare cases where the Dialog used tabindex={1} so that it would receive focus first when tabbing into the document from the browser chrome. But I agree, a more robust handling of document level focusin to marshall focus to the dialog may be better.

…ctrum-v3 into RSP-1596

adobe-bot · 2020-03-19T17:16:14Z

Build successful! 🎉

snowystinger · 2020-03-19T18:25:45Z

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

    return () => {
      document.removeEventListener('keydown', onKeyDown, false);
      document.removeEventListener('focusin', onFocus, false);
+      scope.forEach(element => element.removeEventListener('focusin', onFocus, false));


just to make sure, but the element's we are looping over, they can't change between effects right? I ask because scope is from a ref, which can change without causing useEffect

A lint error previously mentioned this possibility when I had scopeRef.current here previously, and it suggested that I do let scope = scopeRef.current; on line 158 so I think we are good here

o, yep, that'll do the trick.
hmm... though is there any chance that elements might be null now? so maybe we should element && element.remove...?

Think we are ok, tested by adding a div and a button that removes said div inside the Dialog's FocusScope. Removing the div and then closing the dialog didn't trigger any console errors, scope preserved all 3 element (div, button, and Dialog section) while scopeRef.current updated correctly to show button and section only

adobe-bot · 2020-03-24T01:45:28Z

Build successful! 🎉

update active scope to newly rendered dialogs

ef41409

LFDanLu requested review from dannify, devongovett and snowystinger as code owners March 13, 2020 22:06

LFDanLu commented Mar 13, 2020

View reviewed changes

packages/@react-spectrum/dialog/test/DialogTrigger.test.js Outdated Show resolved Hide resolved

snowystinger reviewed Mar 14, 2020

View reviewed changes

LFDanLu added 2 commits March 16, 2020 11:00

Adding test for isActiveScope

051cf50

removing extra newline

419c72f

LFDanLu commented Mar 16, 2020

View reviewed changes

packages/@react-aria/focus/test/FocusScope.test.js Outdated Show resolved Hide resolved

Merge branch 'master' into RSP-1596

ad0110d

Making child focus scopes active over parent focus scopes

e4c16fa

LFDanLu changed the title ~~(WIP) (RSP-1596) Preserve focus on nested dialog on mount~~ (RSP-1596) Preserve focus on nested dialog on mount Mar 16, 2020

LFDanLu commented Mar 16, 2020

View reviewed changes

LFDanLu changed the title ~~(RSP-1596) Preserve focus on nested dialog on mount~~ (RSP-1596) Make child focus scopes take "activeScope" precedent over parent focus scopes Mar 16, 2020

devongovett reviewed Mar 18, 2020

View reviewed changes

LFDanLu mentioned this pull request Mar 18, 2020

(RSP-1592) Reset focus to the Modal Dialog onblur #271

Closed

5 tasks

updating FocusScope onFocus and onBlur so they are attached to scope

71e3785

LFDanLu changed the title ~~(RSP-1596) Make child focus scopes take "activeScope" precedent over parent focus scopes~~ (RSP-1596 and RSP-1592) Update onFocus and onBlur behavior of FocusScope containment Mar 18, 2020

Merge branch 'master' into RSP-1596

e8e7ef3

LFDanLu commented Mar 18, 2020

View reviewed changes

devongovett requested changes Mar 19, 2020

View reviewed changes

LFDanLu added 2 commits March 19, 2020 10:01

Adding focusin listener on document to catch tabbing from browser bar

d27b044

Merge branch 'RSP-1596' of https://github.com/adobe-private/react-spe…

1adff4a

…ctrum-v3 into RSP-1596

snowystinger reviewed Mar 19, 2020

View reviewed changes

snowystinger previously approved these changes Mar 19, 2020

View reviewed changes

devongovett previously approved these changes Mar 24, 2020

View reviewed changes

Merge branch 'master' into RSP-1596

28c7c6a

devongovett dismissed stale reviews from snowystinger and themself via 28c7c6a March 24, 2020 01:35

devongovett approved these changes Mar 24, 2020

View reviewed changes

devongovett merged commit 4091406 into master Mar 24, 2020

devongovett deleted the RSP-1596 branch March 24, 2020 01:46

(RSP-1596 and RSP-1592) Update onFocus and onBlur behavior of FocusScope containment #281

(RSP-1596 and RSP-1592) Update onFocus and onBlur behavior of FocusScope containment #281

Uh oh!

Conversation

LFDanLu commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

Uh oh!

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!

Uh oh!

adobe-bot commented Mar 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Mar 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Mar 18, 2020

Uh oh!

adobe-bot commented Mar 18, 2020

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!

devongovett commented Mar 19, 2020

Uh oh!

majornista commented Mar 19, 2020

Uh oh!

adobe-bot commented Mar 19, 2020

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!

adobe-bot commented Mar 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LFDanLu commented Mar 13, 2020 •

edited

Loading