From 4b255bba9bea97ceaf4006cab7dddad16de1ff18 Mon Sep 17 00:00:00 2001
From: Andrew Musgrave <andrew.musgrave@shopify.com>
Date: Mon, 7 Oct 2019 17:27:05 -0400
Subject: [PATCH] Fixed Popover focusing incorrect element when closed

---
 src/components/Popover/Popover.tsx            |  33 ++++-
 src/components/Popover/tests/Popover.test.tsx |  43 ++++++
 src/components/Portal/Portal.tsx              |   3 +-
 src/components/Portal/tests/Portal.test.tsx   |   5 +-
 src/components/shared.ts                      |   5 +
 src/utilities/focus.ts                        |  37 +++++
 src/utilities/is-element-in-viewport.ts       |  10 ++
 src/utilities/tests/focus.test.ts             | 134 ++++++++++++++++--
 .../tests/is-element-in-viewport.test.ts      |  73 ++++++++++
 9 files changed, 323 insertions(+), 20 deletions(-)
 create mode 100644 src/utilities/is-element-in-viewport.ts
 create mode 100644 src/utilities/tests/is-element-in-viewport.test.ts

diff --git a/src/components/Popover/Popover.tsx b/src/components/Popover/Popover.tsx
index 2d06a55dcb4..b2b452506bb 100644
--- a/src/components/Popover/Popover.tsx
+++ b/src/components/Popover/Popover.tsx
@@ -1,12 +1,11 @@
 import React from 'react';
 import {createUniqueIDFactory} from '@shopify/javascript-utilities/other';
-import {
-  focusFirstFocusableNode,
-  findFirstFocusableNode,
-} from '@shopify/javascript-utilities/focus';
+import {findFirstFocusableNode} from '@shopify/javascript-utilities/focus';
+import {focusNextFocusableNode} from '../../utilities/focus';
 
 import {PreferredPosition, PreferredAlignment} from '../PositionedOverlay';
 import {Portal} from '../Portal';
+import {portal} from '../shared';
 import {CloseSource, Pane, PopoverOverlay, Section} from './components';
 
 export {CloseSource};
@@ -126,16 +125,25 @@ export class Popover extends React.PureComponent<PopoverProps, State> {
   }
 
   private handleClose = (source: CloseSource) => {
+    const {activatorNode} = this.state;
     this.props.onClose(source);
 
     if (this.activatorContainer == null) {
       return;
     }
+
     if (
-      source === CloseSource.FocusOut ||
-      source === CloseSource.EscapeKeypress
+      (source === CloseSource.FocusOut ||
+        source === CloseSource.EscapeKeypress) &&
+      activatorNode
     ) {
-      focusFirstFocusableNode(this.activatorContainer, false);
+      const focusableActivator =
+        findFirstFocusableNode(activatorNode) ||
+        findFirstFocusableNode(this.activatorContainer) ||
+        this.activatorContainer;
+      if (!focusNextFocusableNode(focusableActivator, isInPortal)) {
+        focusableActivator.focus();
+      }
     }
   };
 
@@ -150,3 +158,14 @@ export class Popover extends React.PureComponent<PopoverProps, State> {
     this.activatorContainer = node;
   };
 }
+
+function isInPortal(element: Element) {
+  let parentElement = element.parentElement;
+
+  while (parentElement) {
+    if (parentElement.matches(portal.selector)) return false;
+    parentElement = parentElement.parentElement;
+  }
+
+  return true;
+}
diff --git a/src/components/Popover/tests/Popover.test.tsx b/src/components/Popover/tests/Popover.test.tsx
index 5f930bbeef8..13daf3e5862 100644
--- a/src/components/Popover/tests/Popover.test.tsx
+++ b/src/components/Popover/tests/Popover.test.tsx
@@ -1,6 +1,8 @@
 import React, {useState, useCallback} from 'react';
 import {mountWithAppProvider, findByTestID} from 'test-utilities/legacy';
+import {mountWithApp} from 'test-utilities';
 import {Popover} from '../Popover';
+import {PopoverOverlay} from '../components';
 
 describe('<Popover />', () => {
   const spy = jest.fn();
@@ -173,4 +175,45 @@ describe('<Popover />', () => {
 
     expect(onCloseSpy).not.toHaveBeenCalled();
   });
+
+  it('focuses the next available element when the popover is closed', () => {
+    const id = 'focus-target';
+    function PopoverTest() {
+      return (
+        <React.Fragment>
+          <div>
+            <Popover active activator={<div />} onClose={noop} />
+          </div>
+          <button id={id} />
+        </React.Fragment>
+      );
+    }
+
+    const popover = mountWithApp(<PopoverTest />);
+
+    popover.find(PopoverOverlay)!.trigger('onClose', 1);
+    const focusTarget = popover.find('button', {id})!.domNode;
+
+    expect(document.activeElement).toBe(focusTarget);
+  });
+
+  it('focuses the activator when another focusable element is not available when the popover is closed', () => {
+    const id = 'activator';
+    function PopoverTest() {
+      return (
+        <React.Fragment>
+          <Popover active activator={<button id={id} />} onClose={noop} />
+        </React.Fragment>
+      );
+    }
+
+    const popover = mountWithApp(<PopoverTest />);
+
+    popover.find(PopoverOverlay)!.trigger('onClose', 1);
+    const focusTarget = popover.find('button', {id})!.domNode;
+
+    expect(document.activeElement).toBe(focusTarget);
+  });
 });
+
+function noop() {}
diff --git a/src/components/Portal/Portal.tsx b/src/components/Portal/Portal.tsx
index 4ea2716ec61..d94c89fec87 100644
--- a/src/components/Portal/Portal.tsx
+++ b/src/components/Portal/Portal.tsx
@@ -2,6 +2,7 @@ import React from 'react';
 import {createPortal} from 'react-dom';
 import {createUniqueIDFactory} from '@shopify/javascript-utilities/other';
 import {ThemeContext} from '../../utilities/theme';
+import {portal} from '../shared';
 
 export interface PortalProps {
   children?: React.ReactNode;
@@ -31,7 +32,7 @@ export class Portal extends React.PureComponent<PortalProps, State> {
 
   componentDidMount() {
     this.portalNode = document.createElement('div');
-    this.portalNode.setAttribute('data-portal-id', this.portalId);
+    this.portalNode.setAttribute(portal.props[0], this.portalId);
 
     if (this.context != null) {
       /* eslint-disable babel/camelcase */
diff --git a/src/components/Portal/tests/Portal.test.tsx b/src/components/Portal/tests/Portal.test.tsx
index 36fc42f64e8..3ca08f698c3 100644
--- a/src/components/Portal/tests/Portal.test.tsx
+++ b/src/components/Portal/tests/Portal.test.tsx
@@ -2,6 +2,7 @@ import React from 'react';
 import {mount} from 'enzyme';
 import {mountWithAppProvider} from 'test-utilities/legacy';
 import {Portal} from '../Portal';
+import {portal} from '../../shared';
 
 jest.mock('react-dom', () => ({
   ...require.requireActual('react-dom'),
@@ -34,7 +35,7 @@ describe('<Portal />', () => {
       const idPrefix = 'test';
       mountWithAppProvider(<Portal idPrefix={idPrefix} />);
       const [, portalNode] = lastSpyCall(createPortalSpy);
-      expect(portalNode.getAttribute('data-portal-id')).toMatch(
+      expect(portalNode.getAttribute(portal.props[0])).toMatch(
         new RegExp(`^${idPrefix}-portal`),
       );
     });
@@ -42,7 +43,7 @@ describe('<Portal />', () => {
     it('is ignored when not defined', () => {
       mountWithAppProvider(<Portal />);
       const [, portalNode] = lastSpyCall(createPortalSpy);
-      expect(portalNode.getAttribute('data-portal-id')).toMatch(/^portal/);
+      expect(portalNode.getAttribute(portal.props[0])).toMatch(/^portal/);
     });
   });
 
diff --git a/src/components/shared.ts b/src/components/shared.ts
index 5888a46b90c..debcdfb7b06 100644
--- a/src/components/shared.ts
+++ b/src/components/shared.ts
@@ -28,6 +28,11 @@ export const headerCell = {
   selector: '[data-polaris-header-cell]',
 };
 
+export const portal = {
+  props: ['data-portal-id'],
+  selector: '[data-portal-id]',
+};
+
 export const DATA_ATTRIBUTE = {
   overlay,
   layer,
diff --git a/src/utilities/focus.ts b/src/utilities/focus.ts
index 1c4b9b2e0ca..2aa39c31ed8 100644
--- a/src/utilities/focus.ts
+++ b/src/utilities/focus.ts
@@ -1,5 +1,42 @@
+import {FOCUSABLE_SELECTOR} from '@shopify/javascript-utilities/focus';
+import {isElementInViewport} from './is-element-in-viewport';
+
+type Filter = (element: Element) => void;
+
 export function handleMouseUpByBlurring({
   currentTarget,
 }: React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>) {
   currentTarget.blur();
 }
+
+export function nextFocusableNode(
+  node: HTMLElement,
+  filter?: Filter,
+): HTMLElement | Element | null {
+  const allFocusableElements = [
+    ...document.querySelectorAll(FOCUSABLE_SELECTOR),
+  ];
+  const sliceLocation = allFocusableElements.indexOf(node) + 1;
+  const focusableElementsAfterNode = allFocusableElements.slice(sliceLocation);
+
+  for (const focusableElement of focusableElementsAfterNode) {
+    if (
+      isElementInViewport(focusableElement) &&
+      (!filter || (filter && filter(focusableElement)))
+    ) {
+      return focusableElement;
+    }
+  }
+
+  return null;
+}
+
+export function focusNextFocusableNode(node: HTMLElement, filter?: Filter) {
+  const nextFocusable = nextFocusableNode(node, filter);
+  if (nextFocusable && nextFocusable instanceof HTMLElement) {
+    nextFocusable.focus();
+    return true;
+  }
+
+  return false;
+}
diff --git a/src/utilities/is-element-in-viewport.ts b/src/utilities/is-element-in-viewport.ts
new file mode 100644
index 00000000000..b54ae112086
--- /dev/null
+++ b/src/utilities/is-element-in-viewport.ts
@@ -0,0 +1,10 @@
+export function isElementInViewport(element: Element) {
+  const {top, left, bottom, right} = element.getBoundingClientRect();
+
+  return (
+    top >= 0 &&
+    right <= window.innerWidth &&
+    bottom <= window.innerHeight &&
+    left >= 0
+  );
+}
diff --git a/src/utilities/tests/focus.test.ts b/src/utilities/tests/focus.test.ts
index d71cbeedc69..92ed2d283d7 100644
--- a/src/utilities/tests/focus.test.ts
+++ b/src/utilities/tests/focus.test.ts
@@ -1,14 +1,128 @@
 import {MouseEvent} from 'react';
-import {handleMouseUpByBlurring} from '../focus';
-
-describe('focus', () => {
-  describe('handleMouseUpByBlurring()', () => {
-    it('calls blur on the currentTarget', () => {
-      const currentTarget = document.createElement('button');
-      jest.spyOn(currentTarget, 'blur');
-      const mouseEvent = {currentTarget};
-      handleMouseUpByBlurring(mouseEvent as MouseEvent<HTMLButtonElement>);
-      expect(currentTarget.blur).toHaveBeenCalled();
+import {
+  handleMouseUpByBlurring,
+  focusNextFocusableNode,
+  nextFocusableNode,
+} from '../focus';
+
+describe('handleMouseUpByBlurring()', () => {
+  it('calls blur on the currentTarget', () => {
+    const currentTarget = document.createElement('button');
+    jest.spyOn(currentTarget, 'blur');
+    const mouseEvent = {currentTarget};
+    handleMouseUpByBlurring(mouseEvent as MouseEvent<HTMLButtonElement>);
+    expect(currentTarget.blur).toHaveBeenCalled();
+  });
+});
+
+describe('nextFocusableNode', () => {
+  it('does not return the initial element as the focusable node', () => {
+    const {activator, otherNode} = domSetup();
+
+    expect(nextFocusableNode(activator)).toBe(otherNode);
+  });
+
+  it('returns null when a focusable element is not found', () => {
+    const {activator} = domSetup({
+      otherNodeTag: 'div',
+    });
+
+    expect(nextFocusableNode(activator)).toBeNull();
+  });
+
+  it('filters out elements', () => {
+    const {activator} = domSetup();
+
+    expect(nextFocusableNode(activator, () => false)).toBeNull();
+  });
+
+  it("returns the parent of an adjacent element when it's focusable", () => {
+    const {activator, otherNode} = domSetup();
+
+    expect(nextFocusableNode(activator)).toBe(otherNode);
+  });
+
+  it('searches adjacent elements for focusable children', () => {
+    const {activator, otherNodeNested} = domSetup({
+      otherNodeTag: 'div',
+      nested: true,
+    });
+
+    expect(nextFocusableNode(activator)).toBe(otherNodeNested);
+  });
+
+  it('searches parent elements for focusable children', () => {
+    const {activator, parentsFocusableNode} = domSetup({
+      otherNodeTag: 'div',
+      parents: true,
     });
+
+    expect(nextFocusableNode(activator)).toBe(parentsFocusableNode);
   });
 });
+
+describe('focusNextFocusableNode', () => {
+  it('returns true when the node was focused', () => {
+    const {activator} = domSetup();
+
+    expect(focusNextFocusableNode(activator)).toBe(true);
+  });
+
+  it('returns false when the node was not focused', () => {
+    const {activator} = domSetup({otherNodeTag: 'div'});
+
+    expect(focusNextFocusableNode(activator)).toBe(false);
+  });
+
+  it('focused the node', () => {
+    const {activator, otherNode} = domSetup();
+
+    focusNextFocusableNode(activator);
+
+    expect(document.activeElement).toBe(otherNode);
+  });
+});
+
+function domSetup(
+  options: {
+    wrapperTag?: string;
+    activatorTag?: string;
+    otherNodeTag?: string;
+    otherNodeNestedTag?: string;
+    nested?: boolean;
+    parents?: true;
+  } = {},
+) {
+  const div = 'div';
+  const button = 'button';
+  const {
+    wrapperTag = div,
+    activatorTag = button,
+    otherNodeTag = button,
+    otherNodeNestedTag = button,
+    nested,
+    parents,
+  } = options;
+  const wrapper = document.createElement(wrapperTag);
+  const activator = document.createElement(activatorTag);
+  const otherNode = document.createElement(otherNodeTag);
+  let otherNodeNested = null;
+  let parentNode = null;
+  let parentsFocusableNode = null;
+
+  if (nested) {
+    otherNodeNested = document.createElement(otherNodeNestedTag);
+    otherNode.appendChild(otherNodeNested);
+  }
+
+  wrapper.append(activator, otherNode);
+
+  if (parents) {
+    parentNode = document.createElement(div);
+    parentsFocusableNode = document.createElement(button);
+    parentNode.append(wrapper, parentsFocusableNode);
+  }
+
+  document.body.appendChild(parentNode || wrapper);
+  return {wrapper, activator, otherNode, otherNodeNested, parentsFocusableNode};
+}
diff --git a/src/utilities/tests/is-element-in-viewport.test.ts b/src/utilities/tests/is-element-in-viewport.test.ts
new file mode 100644
index 00000000000..a710c0f0b54
--- /dev/null
+++ b/src/utilities/tests/is-element-in-viewport.test.ts
@@ -0,0 +1,73 @@
+import {isElementInViewport} from '../is-element-in-viewport';
+
+describe('isElementInViewport', () => {
+  const originalGetBoundingClientRect = Element.prototype.getBoundingClientRect;
+
+  afterEach(() => {
+    Element.prototype.getBoundingClientRect = originalGetBoundingClientRect;
+  });
+
+  it('returns true when in viewport', () => {
+    mockGetBoundingClientRect({
+      top: 200,
+      right: window.innerWidth - 200,
+      bottom: window.innerHeight - 200,
+      left: 200,
+    });
+    expect(isElementInViewport(createDiv())).toBe(true);
+  });
+
+  it('returns true when on the viewport edges', () => {
+    mockGetBoundingClientRect({
+      top: 0,
+      right: window.innerWidth,
+      bottom: window.innerHeight,
+      left: 0,
+    });
+    expect(isElementInViewport(createDiv())).toBe(true);
+  });
+
+  describe('not in viewport', () => {
+    it('returns false when top is not in viewport', () => {
+      mockGetBoundingClientRect({top: -1});
+      expect(isElementInViewport(createDiv())).toBe(false);
+    });
+
+    it('returns false when right is not in viewport', () => {
+      mockGetBoundingClientRect({right: window.innerWidth + 1});
+      expect(isElementInViewport(createDiv())).toBe(false);
+    });
+
+    it('returns false when bottom is not in viewport', () => {
+      mockGetBoundingClientRect({bottom: window.innerHeight + 1});
+      expect(isElementInViewport(createDiv())).toBe(false);
+    });
+
+    it('returns false when left is not in viewport', () => {
+      mockGetBoundingClientRect({left: -1});
+      expect(isElementInViewport(createDiv())).toBe(false);
+    });
+  });
+});
+
+function createDiv() {
+  return document.createElement('div');
+}
+
+function mockGetBoundingClientRect({
+  top = 0,
+  left = 0,
+  bottom = 0,
+  right = 0,
+  height = 0,
+  width = 0,
+}: Partial<DOMRect | ClientRect>) {
+  Element.prototype.getBoundingClientRect = () => ({
+    top,
+    left,
+    bottom,
+    right,
+    height,
+    width,
+  });
+}