adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
13.11k stars 1.14k forks source link

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

Open kherock opened 3 years ago

kherock commented 3 years ago

πŸ™‹ Feature Request

I could also consider this a bug report, but its fix might require a breaking change, so I'm reporting this as a feature request instead.

I'd like for a better way to control how focus is restored when tabbing in an out of a FocusScope. Due to the nature of how Portals work, it's impossible to accomplish this effectively without updating its API.

πŸ€” Expected Behavior

When a FocusScope is created, its primary responsibility is to ensure that attempts to move focus in and out of the scope are handled correctly. When focus is contained, I expect that Tab will wrap focus to the beginning and Shift+Tab will wrap to the end. However, when focus isn't contained, I expect that focus will return to a logical position in the virtual DOM. Since the FocusScope has no way to know where in the DOM this is without additional context, it should rely on the native tab ordering.

In the case where the FocusScope's location in the virtual DOM is vastly different from the actual DOM (such as with Portals), I expect to pass an additional hint to indicate where focus should go when tabbing in and out of the scope. In 99% of cases, this will just be the trigger element responsible for opening the portal, so a sensible default value for this hint is to just use the nodeToRestore (which is the existing behavior when restoreFocus is true).

😯 Current Behavior

The existing FocusScope implementation has special handling to make moving focus in and out of portals more seamless. When containment is disabled, it uses the following approach:

[^1]: The existing behavior also questionably makes the nodeToRestore inaccessible via Shift+Tab. If I have a non-modal popup rendered below a button, I'd expect that Shift+Tab would move focus out of the popup and back onto the button. Instead, the existing behavior attempts to move focus to an often non-existent element situated before the button in the DOM

πŸ’ Possible Solution

I drafted up an initial idea here: https://github.com/adobe/react-spectrum/pull/2416#issuecomment-933681029

πŸ”¦ Context

I have a popup component that supports rendering itself in a portal. It implements a non-modal disclosure pattern where focus is restored to the trigger when closed, but it is allowed to stay open when focusing or interacting outside of the popup. Users should be able to freely tab in or out of the popup, and when they do so, focus is moved to a position that corresponds to its position on the page rather than the element that opened it.

πŸ’» Examples

🧒 Your Company/Team

🎁 Tracking Ticket (optional)

LFDanLu commented 3 years ago

Just to make sure I understand the scenario here: you have a popup in a portal and tabbing out of the popup moves focus to the popup's trigger element, but you want to be able to modify that behavior so that tabbing out of the popup would move to a element that is "closer" (tab order wise) to its position?

kherock commented 3 years ago

Correct, and it's mainly useful if I want to keep the trigger as part of the tab order. Currently, the behavior is to sort of replace the trigger in the tab order - like here

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

Shift+Tabing backwards out of the scope moves focus to before, and Tabing forwards moves focus to after.

Say I have a popup overlaid to the left of the trigger, and I want Tab to move focus "forward" and return to the trigger, then I could use something like this to get the behavior I want:

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

I'd have to think about it more, but then it could make sense to have the FocusScope monitor the ref for focus events so that it can restore focus to the focusScope when it becomes active.

LFDanLu commented 3 years ago

I talked with the team regarding this behavior and we think the current FocusScope tab behavior when restoreFocus=true is correct. We regard the trigger element and the popover as a single tab stop and thus tabbing forwards out of the popover's FocusScope should move focus to the element immediately after the trigger element and that tabbing backwards should move focus to the element immediately before the trigger element. If the user wants to move focus back to the trigger element, it feels natural to us that they would simply close the popover via the Escape key and let FocusScope restore focus to the nodeToRestore.

That being said, we think that your idea still has merit but it feels a little dangerous to allow for such full control. Would you mind clarifying a bit more on what your specific use case is? From what you've mentioned in the Context section, it sounds like you are trying to implement a tab order that takes into account the spatial position of the popup (a draggable panel comes to mind). Is the code example in your comment above an example of your actual use case or just an illustrative example of the desired behavior?

kherock commented 3 years ago

I actually don't have a super concrete use case as I'm sort of in the middle of drafting out a design system myself, I just have some ideas of what I'll need. Having the scope replace the trigger in the tab order is definitely causing problems though since it sometimes causes focus to move very far away or to <body> when the trigger is the first element in its section.

The one thing I keep thinking where this feature would be useful come in handy are these popups that overlay content docked below a toolbar. The code example is an illustrative example, as it's missing the extra logic needed to move focus back into the "panel". I've opened a PR that covers the other use case I had in mind with the 'after-trigger' strategy. Let me know what you think of the API I've proposed.

LFDanLu commented 3 years ago

Thanks for making the draft PR! We'll talk to our accessibility team for feedback on this behavior and get back to you.