Avoid restoring focus when clicking on focusable element outside Popover

Fix an issue where clicking on a focusable element outside of an open popover,
other than the previously focused element, would cause focus to be reverted back
to the previously focused element.

When native popovers are used, they handle focus restoration for us [^1], so we
don't need to do anything when the popover is closed. Setting
`restoreFocusOnClose` to `false` didn't actually work in this case. When
non-native popovers are used, focus reverts to `document.body` when the popover
is hidden, unless the user clicked on a different focusable element outside the
popover. Only when it has reverted to `document.body` do we want to restore
focus.

Fixes #1906

[^1]: See `focusPreviousElement` references in https://html.spec.whatwg.org/multipage/popover.html#dom-hidepopover

Author: robertknight

src/components/feedback/Popover.tsx

@@ -248,6 +248,9 @@ export type PopoverProps = {
   /**
    * Determines if focus should be restored when the popover is closed.
    * Defaults to true.
+   *
+   * @deprecated - Setting this prop to `false` does not work if native popovers
+   * are used. This prop will be removed entirely in future.
    */
   restoreFocusOnClose?: boolean;
 
@@ -261,6 +264,61 @@ export type PopoverProps = {
   onScroll?: JSX.HTMLAttributes<HTMLElement>['onScroll'];
 };
 
+type RestoreFocusOptions = {
+  /** True if the popover is open. */
+  open: boolean;
+
+  /** Ref for the popover element. */
+  popoverRef: { current: HTMLElement | null };
+};
+
+/**
+ * Restore focus to the previously active element when a popover is closed.
+ */
+function useRestoreFocusOnClose({ popoverRef, open }: RestoreFocusOptions) {
+  useLayoutEffect(() => {
+    const container = popoverRef.current;
+    const restoreFocusTo = open
+      ? (document.activeElement as HTMLElement)
+      : null;
+
+    if (!container || !restoreFocusTo) {
+      return () => {};
+    }
+
+    return () => {
+      // When a popover is opened and then closed, there are several
+      // possibilities for what happens to the focus:
+      //
+      // 1. The focus may be unchanged from before the popover was opened.
+      //
+      // 2. The focus may have moved into the popover when it was opened, and
+      //    then back to either the previously focused element or the body when
+      //    it was closed.
+      //
+      //    When a native popover is closed via `togglePopover` or `hidePopover`,
+      //    focus will revert to the element that was focused at the time the
+      //    popover was shown. See https://html.spec.whatwg.org/multipage/popover.html#dom-hidepopover.
+      //
+      // 3. The user may have clicked an element outside the popover, focusing
+      //    that element and causing the popover to close.
+      //
+      // From the above cases, we only need to restore focus if it is still
+      // inside the popover, or focus reverted to the document body.
+      const currentFocus = document.activeElement;
+      if (
+        currentFocus &&
+        !container.contains(currentFocus) &&
+        currentFocus !== document.body
+      ) {
+        return;
+      }
+
+      restoreFocusTo.focus();
+    };
+  }, [popoverRef, open]);
+}
+
 export default function Popover({
   anchorElementRef,
   children,
@@ -269,7 +327,6 @@ export default function Popover({
   align = 'left',
   classes,
   variant = 'panel',
-  restoreFocusOnClose = true,
   onScroll,
   /* eslint-disable-next-line no-prototype-builtins */
   asNativePopover = HTMLElement.prototype.hasOwnProperty('popover'),
@@ -283,20 +340,11 @@ export default function Popover({
     asNativePopover,
     align === 'right',
   );
-
   useOnClose(popoverRef, anchorElementRef, onClose, open, asNativePopover);
-
-  useLayoutEffect(() => {
-    const restoreFocusTo = open
-      ? (document.activeElement as HTMLElement)
-      : null;
-
-    return () => {
-      if (restoreFocusOnClose && restoreFocusTo) {
-        restoreFocusTo.focus();
-      }
-    };
-  }, [open, restoreFocusOnClose]);
+  useRestoreFocusOnClose({
+    open,
+    popoverRef: popoverRef,
+  });
 
   return (
     <div

src/components/feedback/test/Popover-test.js

@@ -9,6 +9,8 @@ function TestComponent({ children, ...rest }) {
 
   return (
     <div className="relative">
+      {/* Focusable element that is not the popover trigger. */}
+      <input data-testid="test-input" />
       <button
         data-testid="toggle-button"
         className="p-2"
@@ -98,20 +100,14 @@ describe('Popover', () => {
 
   [
     {
-      restoreFocusOnClose: undefined, // Defaults to true
-      expectedFocusAfterClose: wrapper => getToggleButton(wrapper).getDOMNode(),
-    },
-    {
-      restoreFocusOnClose: true,
-      expectedFocusAfterClose: wrapper => getToggleButton(wrapper).getDOMNode(),
+      asNativePopover: true,
     },
     {
-      restoreFocusOnClose: false,
-      expectedFocusAfterClose: () => document.body,
+      asNativePopover: false,
     },
-  ].forEach(({ restoreFocusOnClose, expectedFocusAfterClose }) => {
-    it('restores focus to toggle button after closing popover', () => {
-      const wrapper = createComponent({ restoreFocusOnClose });
+  ].forEach(({ asNativePopover }) => {
+    it('restores focus to previously focused element when popover is closed', () => {
+      const wrapper = createComponent({ asNativePopover });
 
       // Focus toggle button before opening popover
       getToggleButton(wrapper).getDOMNode().focus();
@@ -121,13 +117,43 @@ describe('Popover', () => {
       // Focus something else before closing the popover
       const innerButton = wrapper.find('[data-testid="inner-button"]');
       innerButton.getDOMNode().focus();
+      assert.notEqual(
+        document.activeElement,
+        getToggleButton(wrapper).getDOMNode(),
+      );
+
       // Close the popover with a different button inside the popover itself
       innerButton.simulate('click');
       assert.isFalse(getPopover(wrapper).prop('open'));
 
       // After closing popover, the focus should have returned to the toggle
       // button, which was the last focused element
-      assert.equal(document.activeElement, expectedFocusAfterClose(wrapper));
+      assert.equal(
+        document.activeElement,
+        getToggleButton(wrapper).getDOMNode(),
+      );
+    });
+
+    it("doesn't restore focus if user clicked on focusable element outside popover", () => {
+      const wrapper = createComponent({ asNativePopover });
+
+      // Focus toggle button before opening popover
+      getToggleButton(wrapper).getDOMNode().focus();
+      togglePopover(wrapper);
+      assert.isTrue(getPopover(wrapper).prop('open'));
+
+      // Focus a control outside the popover
+      const input = wrapper.find('[data-testid="test-input"]').getDOMNode();
+      input.focus();
+      assert.equal(document.activeElement, input);
+
+      // Close popover
+      const innerButton = wrapper.find('[data-testid="inner-button"]');
+      innerButton.simulate('click');
+      assert.isFalse(getPopover(wrapper).prop('open'));
+
+      // Check that input remains focused
+      assert.equal(document.activeElement, input);
     });
   });