Repository: mapbox/mr-ui (ts)

Issue:
Investigate observing size updates of Popover child
# Problem

Right now, if the child of a `Popover` changes it's width/height, the calculations for the Popover's alignment stay the same. There is `Popover.repositionPopovers` which is helpful for scenarios you know this may occur, but tracking these scenarios individually may be bug-prone.

# Potential solution

A pattern I found that might be more generally useful is observing the child of a `Popover` with [ResizeObserver](https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver), so when the size updates the popover can reposition.

Maybe there could be an optional `Popover` field such as `observeSize`?

Pull request:
Use ResizeObserver if available in <PopoverPositioner>
Closes #130 

This ticket adds an `observeSize` prop to `Popover` and `PopoverPositioner` (true by default) that observes the popover content using `ResizeObserver` and recalculates size dynamically.

This is a progressive enhancement so if `ResizeObserver` isn't available in a browser this is not used. It has extremely high browser support though, minus IE11. https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

---

This lets us also remove Studio logic that it really shouldn't need to worry about:

https://github.com/mapbox/studio/blob/main/src/shared_components/resize_observer_component.tsx
https://github.com/mapbox/studio/blob/main/src/notification_components/notification_display.js

Patch:
diff --git a/src/components/popover-trigger/popover-trigger.js b/src/components/popover-trigger/popover-trigger.js
index fb5d259..ad81095 100644
--- a/src/components/popover-trigger/popover-trigger.js
+++ b/src/components/popover-trigger/popover-trigger.js
@@ -128,7 +128,10 @@ export default class PopoverTrigger extends React.Component {
       (!this.popoverElement || !this.popoverElement.contains(event.target))
     ) {
       this.hide();
-    } else {
+    } else if (
+      !this.popoverContentElement ||
+      !this.popoverContentElement.contains(event.target)
+    ) {
       this.showBecauseClick();
     }
   };
@@ -319,9 +322,16 @@ export default class PopoverTrigger extends React.Component {
   };
 
   setPopoverElement = (element) => {
+    if (this.props.popoverProps.ref) {
+      this.props.popoverProps.ref(element);
+    }
     this.popoverElement = element;
   };
 
+  setPopoverContentElement = (element) => {
+    this.popoverContentElement = element;
+  };
+
   getPopoverContent = () => {
     const { content } = this.props;
     if (typeof content === 'function') {
@@ -351,6 +361,10 @@ export default class PopoverTrigger extends React.Component {
           receiveFocus={receiveFocus}
           trapFocus={trapFocus}
           onElement={this.setPopoverElement}
+          passthroughProps={{
+            ...(props.popoverProps.passthroughProps || {}),
+            ref: this.setPopoverContentElement
+          }}
         >
           {this.getPopoverContent()}
         </Popover>
diff --git a/src/components/popover/examples/popover-e.js b/src/components/popover/examples/popover-e.js
new file mode 100644
index 0000000..09a4176
--- /dev/null
+++ b/src/components/popover/examples/popover-e.js
@@ -0,0 +1,106 @@
+/*
+Popover with non-static height.
+*/
+import React from 'react';
+import Popover from '../popover';
+import Button from '../../button';
+
+class ExampleInner extends React.Component {
+  constructor(props) {
+    super(props);
+    this.state = { items: [0] };
+  }
+
+  add = () => {
+    this.setState({ items: [...this.state.items, 0] });
+  };
+
+  remove = () => {
+    this.setState({ items: this.state.items.slice(1) });
+  };
+
+  render() {
+    return (
+      <div>
+        <ul>
+          {this.state.items &&
+            this.state.items.map((item, index) => (
+              <li key={index}>This is a popover!</li>
+            ))}
+        </ul>
+        <div className="flex flex--row mt6">
+          <Button onClick={this.add} size="medium">
+            Add
+          </Button>
+          <span className="ml6">
+            <Button onClick={this.remove} size="medium">
+              Remove
+            </Button>
+          </span>
+        </div>
+      </div>
+    );
+  }
+}
+
+export default class Example extends React.Component {
+  constructor(props) {
+    super(props);
+    this.state = { popoverOpen: false };
+    this.togglePopover = this.togglePopover.bind(this);
+    this.setAnchor = this.setAnchor.bind(this);
+    this.getAnchor = this.getAnchor.bind(this);
+    this.ignore = this.ignore.bind(this);
+  }
+
+  togglePopover() {
+    this.setState((state) => ({ popoverOpen: !state.popoverOpen }));
+  }
+
+  setAnchor(el) {
+    this.anchor = el;
+  }
+
+  getAnchor() {
+    return this.anchor;
+  }
+
+  ignore(el) {
+    return el === this.getAnchor();
+  }
+
+  renderPopover() {
+    if (!this.state.popoverOpen) {
+      return null;
+    }
+    return (
+      <Popover
+        getAnchorElement={this.getAnchor}
+        onExit={this.togglePopover}
+        ignoreClickWithinElement={this.ignore}
+        placement="top"
+        alignment="left"
+      >
+        <ExampleInner />
+      </Popover>
+    );
+  }
+
+  render() {
+    return (
+      <div>
+        <Button
+          onClick={this.togglePopover}
+          size="medium"
+          passthroughProps={{
+            'aria-label': 'Toggle popover',
+            ref: this.setAnchor
+          }}
+        >
+          Toggle popover
+        </Button>
+        {this.renderPopover()}
+      </div>
+    );
+  }
+}
diff --git a/src/components/popover/popover-positioner.js b/src/components/popover/popover-positioner.js
index 1ba465b..ba637cc 100644
--- a/src/components/popover/popover-positioner.js
+++ b/src/components/popover/popover-positioner.js
@@ -45,6 +45,7 @@ class PopoverPositioner extends React.PureComponent {
     getContainingElement: PropTypes.func,
     onElement: PropTypes.func,
     zIndex: PropTypes.number,
+    observeSize: PropTypes.bool,
     // For mockery
     calculatePosition: PropTypes.func,
     getScrollableParents: PropTypes.func,
@@ -75,6 +76,17 @@ class PopoverPositioner extends React.PureComponent {
       return scrollListener;
     }, this);
     this.setPosition();
+
+    const popoverEl = this.getPopoverElement();
+    if (this.props.observeSize && 'ResizeObserver' in window) {
+      this.resizeObserver = new ResizeObserver(this.setPosition);
+      this.resizeObserver.observe(popoverEl);
+    } else {
+      if (this.resizeObserver) {
+        this.resizeObserver.unobserve(popoverEl);
+        this.resizeObserver = null;
+      }
+    }
   }
 
   componentDidUpdate() {
@@ -82,6 +94,10 @@ class PopoverPositioner extends React.PureComponent {
   }
 
   componentWillUnmount() {
+    if (this.resizeObserver) {
+      this.resizeObserver.unobserve(this.getPopoverElement());
+      this.resizeObserver = null;
+    }
     this.props.getWindow().removeEventListener('resize', this.handleResize);
     this.scrollListeners.forEach((scrollListener) => {
       scrollListener.stop();
diff --git a/src/components/popover/popover.js b/src/components/popover/popover.js
index f39c62d..c614718 100644
--- a/src/components/popover/popover.js
+++ b/src/components/popover/popover.js
@@ -25,6 +25,10 @@ let popoverCounter = 0; // Incremented on creation
  * useful if some state change other than a scroll or resize may have caused
  * the popover's anchor to move or the space available to the
  * popover to change.
+ *
+ * If `ResizeObserver` is available in the user's browser, the popover will
+ * automatically reposition itself when the content is resized. This can be disabled
+ * in the `observeSize` prop.
  */
 export default class Popover extends React.Component {
   constructor(props) {
@@ -210,6 +214,7 @@ export default class Popover extends React.Component {
         offsetFromAnchor={props.offsetFromAnchor}
         onElement={this.setPopoverElement}
         zIndex={props.zIndex}
+        observeSize={props.observeSize}
       >
         <div
           key="body"
@@ -339,7 +344,12 @@ Popover.propTypes = {
   /**
    * CSS z-index number to order popover over content.
    */
-  zIndex: PropTypes.number
+  zIndex: PropTypes.number,
+  /**
+   * If `true` and `ResizeObserver` is available in the user's browser,
+   * the popover will automatically reposition itself when the content is resized.
+   */
+  observeSize: PropTypes.bool
 };
 
 Popover.defaultProps = {
@@ -355,7 +365,8 @@ Popover.defaultProps = {
   escapeCloses: true,
   receiveFocus: true,
   trapFocus: false,
-  zIndex: 1
+  zIndex: 1,
+  observeSize: true
 };
 
 Popover.repositionPopovers = PopoverPositioner.recalculatePositions;


Test patch:
diff --git a/src/components/copiable/__tests__/__snapshots__/copiable.test.js.snap b/src/components/copiable/__tests__/__snapshots__/copiable.test.js.snap
index b3d6e6f..88e22b1 100644
--- a/src/components/copiable/__tests__/__snapshots__/copiable.test.js.snap
+++ b/src/components/copiable/__tests__/__snapshots__/copiable.test.js.snap
@@ -78,6 +78,7 @@ exports[`basic shows copy hint when focused 1`] = `
     getAnchorElement={[Function]}
     hasPointer={true}
     hideWhenAnchorIsOffscreen={true}
+    observeSize={true}
     padding="small"
     placement="top"
     receiveFocus={true}
diff --git a/src/components/copy-button/__tests__/__snapshots__/copy-button.test.js.snap b/src/components/copy-button/__tests__/__snapshots__/copy-button.test.js.snap
index 1294631..86ae6fe 100644
--- a/src/components/copy-button/__tests__/__snapshots__/copy-button.test.js.snap
+++ b/src/components/copy-button/__tests__/__snapshots__/copy-button.test.js.snap
@@ -37,6 +37,7 @@ exports[`CopyButton Defaults changes state on click 1`] = `
       getAnchorElement={[Function]}
       hasPointer={true}
       hideWhenAnchorIsOffscreen={true}
+      observeSize={true}
       padding="small"
       placement="top"
       receiveFocus={true}
@@ -151,6 +152,7 @@ exports[`CopyButton all props changes state on click 1`] = `
       getAnchorElement={[Function]}
       hasPointer={true}
       hideWhenAnchorIsOffscreen={true}
+      observeSize={true}
       padding="small"
       placement="top"
       receiveFocus={true}
diff --git a/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js b/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
index c065520..39e6314 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger-test-cases.js
@@ -73,4 +73,35 @@ testCases.callbacks = {
   }
 };
 
+let popoverTriggerRef = null;
+function setPopoverTriggerRef(ref) {
+  popoverTriggerRef = ref;
+}
+
+testCases.clickInsideContent = {
+  description: 'click inside content',
+  element: (
+    <PopoverTrigger
+      ref={setPopoverTriggerRef}
+      content={() => {
+        return (
+          <button
+            aria-label="Close"
+            onClick={() => {
+              if (popoverTriggerRef) popoverTriggerRef.hide();
+            }}
+          >
+            Close
+          </button>
+        );
+      }}
+      respondsToClick={true}
+    >
+      <button aria-label="Trigger" className="btn">
+        Trigger
+      </button>
+    </PopoverTrigger>
+  )
+};
+
 export { testCases };
diff --git a/src/components/popover-trigger/__tests__/popover-trigger.test.js b/src/components/popover-trigger/__tests__/popover-trigger.test.js
index 3ed1611..600179d 100644
--- a/src/components/popover-trigger/__tests__/popover-trigger.test.js
+++ b/src/components/popover-trigger/__tests__/popover-trigger.test.js
@@ -1,5 +1,5 @@
 import React from 'react';
-import { shallow } from 'enzyme';
+import { mount, shallow } from 'enzyme';
 import { testCases } from './popover-trigger-test-cases';
 
 describe('PopoverTrigger', () => {
@@ -116,4 +116,24 @@ describe('PopoverTrigger', () => {
       expect(testCase.props.onPopoverClose).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe(testCases.clickInsideContent.description, () => {
+    beforeEach(() => {
+      testCase = testCases.clickInsideContent;
+      wrapper = mount(testCase.element);
+    });
+
+    test('when click inside content, popover closes', () => {
+      expect(wrapper.find('Popover').length).toBe(0);
+      wrapper.simulate('click');
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(1);
+
+      const closeButtons = wrapper.find('button[aria-label="Close"]');
+      expect(closeButtons.length).toBe(1);
+      closeButtons.at(0).simulate('click');
+      wrapper.update();
+      expect(wrapper.find('Popover').length).toBe(0);
+    });
+  });
 });
diff --git a/src/components/popover/__tests__/__snapshots__/popover.test.js.snap b/src/components/popover/__tests__/__snapshots__/popover.test.js.snap
index 0da3c3d..85a61b8 100644
--- a/src/components/popover/__tests__/__snapshots__/popover.test.js.snap
+++ b/src/components/popover/__tests__/__snapshots__/popover.test.js.snap
@@ -10,6 +10,7 @@ exports[`Popover all props renders 1`] = `
   hasPointer={false}
   hideWhenAnchorIsOffscreen={true}
   mounted={true}
+  observeSize={true}
   onElement={[Function]}
   placement="left"
   pointerColor="#fff"
@@ -48,6 +49,7 @@ exports[`Popover basic renders 1`] = `
   hasPointer={true}
   hideWhenAnchorIsOffscreen={false}
   mounted={true}
+  observeSize={true}
   onElement={[Function]}
   placement="right"
   pointerColor="#fff"
@@ -85,6 +87,7 @@ exports[`Popover dark renders 1`] = `
   hasPointer={true}
   hideWhenAnchorIsOffscreen={false}
   mounted={true}
+  observeSize={true}
   onElement={[Function]}
   placement="right"
   pointerColor="#0E2127"
@@ -122,6 +125,7 @@ exports[`Popover no pointer renders 1`] = `
   hasPointer={false}
   hideWhenAnchorIsOffscreen={false}
   mounted={true}
+  observeSize={true}
   onElement={[Function]}
   placement="right"
   pointerColor="#fff"
@@ -159,6 +163,7 @@ exports[`Popover warning renders 1`] = `
   hasPointer={true}
   hideWhenAnchorIsOffscreen={false}
   mounted={true}
+  observeSize={true}
   onElement={[Function]}
   placement="right"
   pointerColor="#dd7a08"
diff --git a/src/components/popover/__tests__/popover-positioner.test.js b/src/components/popover/__tests__/popover-positioner.test.js
index 8bf5dba..14893f2 100644
--- a/src/components/popover/__tests__/popover-positioner.test.js
+++ b/src/components/popover/__tests__/popover-positioner.test.js
@@ -50,7 +50,8 @@ describe('PopoverPositioner', () => {
       getScrollableParents: mockGetScrollableParents,
       createScrollListener: mockCreateScrollListener,
       zIndex: 3,
-      getWindow: mockGetWindow
+      getWindow: mockGetWindow,
+      observeSize: true
     };
   });
 
@@ -214,4 +215,20 @@ describe('PopoverPositioner', () => {
     PopoverPositioner.recalculatePositions();
     expect(mockCalculatePosition).toHaveBeenCalledTimes(2);
   });
+
+  test('resize observer does not exist with observeSize=false', () => {
+    const wrapper = mount(
+      <PopoverPositioner.WrappedComponent {...props} observeSize={false} />
+    );
+    expect(mockCalculatePosition).toHaveBeenCalledTimes(1);
+    expect(wrapper.instance().resizeObserver).toBeFalsy();
+  });
+
+  test('resize observer forces position recalc', () => {
+    const wrapper = mount(<PopoverPositioner.WrappedComponent {...props} />);
+    expect(mockCalculatePosition).toHaveBeenCalledTimes(1);
+    expect(wrapper.instance().resizeObserver).not.toBeFalsy();
+    wrapper.instance().resizeObserver.resizeFn();
+    expect(mockCalculatePosition).toHaveBeenCalledTimes(2);
+  });
 });
diff --git a/src/test-utils/jest-setup.js b/src/test-utils/jest-setup.js
index 4891a26..d184882 100644
--- a/src/test-utils/jest-setup.js
+++ b/src/test-utils/jest-setup.js
@@ -1,4 +1,5 @@
 import './raf';
+import './resize-observer';
 import Enzyme from 'enzyme';
 import Adapter from 'enzyme-adapter-react-16';
 
diff --git a/src/test-utils/resize-observer.js b/src/test-utils/resize-observer.js
new file mode 100644
index 0000000..e6da25c
--- /dev/null
+++ b/src/test-utils/resize-observer.js
@@ -0,0 +1,14 @@
+/* globals global */
+global.ResizeObserver = class ResizeObserver {
+  constructor(resizeFn) {
+    this.resizeFn = resizeFn;
+  }
+
+  observe() {
+    // noop
+  }
+
+  unobserve() {
+    // noop
+  }
+};