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.04k stars 1.13k forks source link

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

Closed majornista closed 2 years ago

majornista commented 3 years ago

๐Ÿ› Bug Report

In the DialogContainer: Dialog triggered by a menu item example, a Dialog opens from a menu item, and then the menu containing the menu item closes, removing the element to which focus should be restored when the Dialog closes from the DOM. When the Dialog closes, focus is lost to the document.body.

๐Ÿค” Expected Behavior

When a scope containing a nodeToRestore has been unmounted, we should restore focus to a previous `nodeToRestore, so in the Dialog triggered by a menu item example, focus should return to the ActionButton within the MenuTrigger.

๐Ÿ˜ฏ Current Behavior

When the Dialog closes, focus is lost to the document.body, because we do not keep track of previous nodeToRestores.

๐Ÿ’ Possible Solution

Manage a stack of nodeToRestore elements so that when unmounting a scope, if that scopes nodeToRestore is no longer in the DOM, we can restore focus to previous nodeToRestore that remains in the DOM.

๐Ÿ”ฆ Context

This is very important for keyboard and screen reader accessibility, especially with Portals in React, because when a FocusScope within a Portal unmounts with nowhere to return focus, focus is lost to the document.body at the end of the DOM, and a keyboard or screen reader user would have to navigate through the document to get back to where they left off.

A user can explicitly manage focus within their application, when dialogs close, but that puts the burden on the developer, when it should probably be the framework's responsibility.

๐Ÿ’ป Code Sample

Within FocusScope:

// An array to create a stack of nodeToRestore elements.
let nodeToRestoreArray: HTMLElement[] = [];

function addToNodeToRestoreArray(node: HTMLElement) {
  if (nodeToRestoreArray.indexOf(node) === -1) {
    nodeToRestoreArray.push(node);
  }
}

function removeFromNodeToRestoreArray(node: HTMLElement) {
  let index = nodeToRestoreArray.indexOf(node);
  if (index > -1) {
    nodeToRestoreArray.splice(index, 1);
  }
}

function useRestoreFocus(scopeRef: RefObject<HTMLElement[]>, restoreFocus: boolean, contain: boolean) {
  // useLayoutEffect instead of useEffect so the active element is saved synchronously instead of asynchronously.
  useLayoutEffect(() => {
    if (!restoreFocus) {
      return;
    }

    let scope = scopeRef.current;
    let nodeToRestore = document.activeElement as HTMLElement;
    addToNodeToRestoreArray(nodeToRestore);

    // Handle the Tab key so that tabbing out of the scope goes to the next element
    // after the node that had focus when the scope mounted. This is important when
    // using portals for overlays, so that focus goes to the expected element when
    // tabbing out of the overlay.
    let onKeyDown = (e: KeyboardEvent) => {
      if (e.key !== 'Tab' || e.altKey || e.ctrlKey || e.metaKey) {
        return;
      }

      let focusedElement = document.activeElement as HTMLElement;
      if (!isElementInScope(focusedElement, scope)) {
        return;
      }

      // Create a DOM tree walker that matches all tabbable elements
      let walker = getFocusableTreeWalker(document.body, {tabbable: true});

      // Find the next tabbable element after the currently focused element
      walker.currentNode = focusedElement;
      let nextElement = (e.shiftKey ? walker.previousNode() : walker.nextNode()) as HTMLElement;

      if (!document.body.contains(nodeToRestore) || nodeToRestore === document.body) {
        removeFromNodeToRestoreArray(nodeToRestore);
        nodeToRestore = null;
      }

      // If there is no next element, or it is outside the current scope, move focus to the
      // next element after the node to restore to instead.
      if ((!nextElement || !isElementInScope(nextElement, scope)) && nodeToRestore) {
        walker.currentNode = nodeToRestore;

        // Skip over elements within the scope, in case the scope immediately follows the node to restore.
        do {
          nextElement = (e.shiftKey ? walker.previousNode() : walker.nextNode()) as HTMLElement;
        } while (isElementInScope(nextElement, scope));

        e.preventDefault();
        e.stopPropagation();
        if (nextElement) {
          focusElement(nextElement, true);
        } else {
           // If there is no next element and the nodeToRestore isn't within a FocusScope (i.e. we are leaving the top level focus scope)
           // then move focus to the body.
           // Otherwise restore focus to the nodeToRestore (e.g menu within a popover -> tabbing to close the menu should move focus to menu trigger)
          if (!isElementInAnyScope(nodeToRestore)) {
            focusedElement.blur();
          } else {
            focusElement(nodeToRestore, true);
          }
        }
      }
    };

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

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

      if (nodeToRestore) {
        let index = nodeToRestoreArray.indexOf(nodeToRestore);

        if (restoreFocus && isElementInScope(document.activeElement, scope)) {
          requestAnimationFrame(() => {
            if (document.body.contains(nodeToRestore) && nodeToRestore !== document.body) {
              focusElement(nodeToRestore);
            } else {
              while (index > 0) {
                index--;
                let node = nodeToRestoreArray[index];
                if (!document.body.contains(node) || node === document.body) {
                  removeFromNodeToRestoreArray(node);
                } else {
                  focusElement(node);
                  break;
                }
              }
            }
          });
        } else if (!document.body.contains(nodeToRestore)) {
          removeFromNodeToRestoreArray(nodeToRestore);
        }
      }
    };
  }, [scopeRef, restoreFocus, contain]);
}

๐ŸŒ Your Environment

Software Version(s)
react-spectrum @react-aria/focus v3.5.0
Browser All
Operating System All

๐Ÿงข Your Company/Team

Adobe/Accessibility

๐Ÿ•ท Tracking Issue (optional)

Related issues:

snowystinger commented 2 years ago

We'd like to fix this with a refactor to FocusScope that takes our current Map and implements a doubly linked tree for our FocusScopes with null as the root. The reason for it being doubly-linked is that we will need to update and remove nodes in the tree, and we'll need to handle reparenting. We also need to know if something is in an ancestor or child. Having it doubly-linked should make these queries easier and these movements easier.

Right now we have a Map that vaguely implements a single direction tree, however, it can become out of date with nodes pointing to non-existent items.

There are several useful tests added in the PR that did exploration around this. They should be used to vet any future work.