Repository: mapbox/mr-ui
Pull request #92 (2019-12-05 21:40:50)

focusTrapPaused in copyButton to work for popover use case
ref: https://github.com/mapbox/studio/issues/12578

ref: https://github.com/mapbox/mr-ui/issues/49

☝️ I decided not to close the above issue because while this fixes the specific issue we were having in Studio, I'm seeing that it may not fix other infinite scroll issues which may require more investigation.

Copy button was optimized for modals by trapping focus. This was causing breakage where it could lead to an infinite scroll. The copy button no longer defaults to be optimized for modals.

Also, to note: For this fix to actually fix https://github.com/mapbox/studio/issues/12578 we need to switch out [`@mapbox/react-copy-button`](https://github.com/mapbox/studio/blob/master/src/shared_components/copiable.js#L5) for `@mapbox/mr-ui/copy-button` and add the `focusTrapPaused={true}` to the `CopyButton` component.

### Question

~~The `trapFocus` prop makes the most sense to me, but it will be a breaking change since it needs to be set to `true` for modals. Should we change this prop to `pauseFocusTrap` and set that to `true` for instances that are **not** modals? That would not be breaking, but I feel like it's optimizing a lesser user case?~~

Switched to `focusTrapPaused`.

## ~~TODO~~

~~Currently working on replicating the original issue in test cases app to demonstrate this fix works.~~

Per conversation, we're going to see if this works and not drive ourselves insane to get a replicating test case here. Logically this makes sense.

cc @elifitch 

aparlato: @elifitch I made an edit to the original comment about not closing the original `mr-ui` issue. I'm seeing odd scrolling behavior still present with a `CopyButton` in a `CodeSnippet` that doesn't totally replicate the original issue, but isn't changed by this fix. Due to difficulty replicating the original issue in a test environment and the fact that this generally seems like a good fix and solves our problem in Studio, I still think we should move forward. I suspect there might be wider spanning issues/patterns in `mr-ui` around focus that are causing various scrolling issues.

Patch:
diff --git a/src/components/copiable/copiable.js b/src/components/copiable/copiable.js
index ca2ee66..8df3749 100644
--- a/src/components/copiable/copiable.js
+++ b/src/components/copiable/copiable.js
@@ -97,7 +97,11 @@ export default class Copiable extends React.Component {
 
     return (
       <div className="absolute top right px6 py6">
-        <CopyButton text={props.value} block={true} />
+        <CopyButton
+          text={props.value}
+          block={true}
+          focusTrapPaused={props.focusTrapPaused}
+        />
       </div>
     );
   }
@@ -153,6 +157,13 @@ Copiable.propTypes = {
    * The text that will be displayed and copied.
    */
   value: PropTypes.string.isRequired,
+  /**
+   * If `true`, this will allow interaction with elements outside of the
+   * modal container. You normally don't want to set this, but it can be
+   * useful for nesting different components that are displaced to other
+   * parts of the DOM.
+   */
+  focusTrapPaused: PropTypes.bool,
   /**
    * If `false` (default), the text will be overflow to multiple lines,
    * and words longer than a single line (e.g. long access tokens or URLs)
diff --git a/src/components/copy-button/copy-button.js b/src/components/copy-button/copy-button.js
index e415d60..f9e1954 100644
--- a/src/components/copy-button/copy-button.js
+++ b/src/components/copy-button/copy-button.js
@@ -74,10 +74,11 @@ export default class CopyButton extends React.PureComponent {
   };
 
   setClipboard(element) {
+    const { focusTrapPaused } = this.props;
     this.clipboard = new Clipboard(element, {
       // Setting the container is necessary for Clipboard to function within
       // focus traps, like in a Modal.
-      container: element
+      ...(!focusTrapPaused && { container: element })
     });
   }
 
@@ -165,6 +166,13 @@ CopyButton.propTypes = {
    * width (in a layout that uses flexbox, absolute positioning, or floats).
    */
   block: PropTypes.bool,
+  /**
+   * If `true`, this will allow interaction with elements outside of the
+   * modal container. You normally don't want to set this, but it can be
+   * useful for nesting different components that are displaced to other
+   * parts of the DOM.
+   */
+  focusTrapPaused: PropTypes.bool,
   /**
    * The `className` prop of the `<button>`.
    */


Test patch:
diff --git a/src/components/copiable/__tests__/copiable-test-cases.js b/src/components/copiable/__tests__/copiable-test-cases.js
index 40ba794..3082cbe 100644
--- a/src/components/copiable/__tests__/copiable-test-cases.js
+++ b/src/components/copiable/__tests__/copiable-test-cases.js
@@ -11,6 +11,16 @@ testCases.basic = {
   }
 };
 
+testCases.focusTrapPaused = {
+  description: 'focusTrapPaused',
+  component: Copiable,
+  props: {
+    focusTrapPaused: true,
+    value:
+      'thetextyoucopythetextyoucopythetextyoucopythetext you copy the text you copy '
+  }
+};
+
 testCases.truncated = {
   description: 'truncated',
   component: Copiable,
diff --git a/src/components/copiable/__tests__/copiable.test.js b/src/components/copiable/__tests__/copiable.test.js
index c80e24d..7f7b3dc 100644
--- a/src/components/copiable/__tests__/copiable.test.js
+++ b/src/components/copiable/__tests__/copiable.test.js
@@ -73,3 +73,15 @@ describe(testCases.basic.description, () => {
     });
   });
 });
+
+describe(testCases.focusTrapPaused.description, () => {
+  beforeEach(() => {
+    testCase = testCases.focusTrapPaused;
+    wrapper = shallow(React.createElement(testCase.component, testCase.props));
+  });
+
+  test('passed focusTrapPaused to CopyButton', () => {
+    const focusTrapPaused = wrapper.find('CopyButton').props().focusTrapPaused;
+    expect(focusTrapPaused).toBe(true);
+  });
+});
diff --git a/src/components/copy-button/__tests__/copy-button-test-cases.js b/src/components/copy-button/__tests__/copy-button-test-cases.js
index c84f000..6fb19c9 100644
--- a/src/components/copy-button/__tests__/copy-button-test-cases.js
+++ b/src/components/copy-button/__tests__/copy-button-test-cases.js
@@ -20,6 +20,7 @@ testCases.allProps = {
     text: 'more copiable text',
     onCopy: safeSpy(),
     className: 'px6 py6 btn btn--purple btn--stroke',
+    focusTrapPaused: true,
     passthroughProps: {
       'data-test': 'copy-button'
     }
diff --git a/src/components/copy-button/__tests__/copy-button.test.js b/src/components/copy-button/__tests__/copy-button.test.js
index 154c8a6..0dd1c7c 100644
--- a/src/components/copy-button/__tests__/copy-button.test.js
+++ b/src/components/copy-button/__tests__/copy-button.test.js
@@ -3,15 +3,15 @@ import { shallow } from 'enzyme';
 import delay from 'delay';
 import { testCases } from './copy-button-test-cases';
 import getWindow from '../../utils/get-window';
+import Clipboard from 'clipboard/dist/clipboard.min.js';
 
 const mockUserAgent =
   'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36';
 
-jest.mock('clipboard/dist/clipboard.min.js', () => {
-  class Clipboard {}
-  Clipboard.isSupported = () => true;
-  return Clipboard;
-});
+jest.mock('clipboard/dist/clipboard.min.js');
+
+Clipboard.isSupported = () => true;
+
 jest.mock('../../utils/get-window', () => {
   return jest.fn();
 });
@@ -49,6 +49,14 @@ describe('CopyButton', () => {
         expect(wrapper).toMatchSnapshot();
       });
     });
+
+    test('focusTrapPaused false/undefined updates container element when setting clipboard', () => {
+      wrapper.instance().setClipboard('cool container');
+
+      expect(Clipboard).toHaveBeenCalledWith('cool container', {
+        container: 'cool container'
+      });
+    });
   });
 
   describe(testCases.allProps.description, () => {
@@ -79,5 +87,11 @@ describe('CopyButton', () => {
       expect(testCase.props.onCopy).toHaveBeenCalledTimes(1);
       expect(testCase.props.onCopy).toHaveBeenCalledWith(testCase.props.text);
     });
+
+    test('focusTrapPaused true does not update container element when setting clipboard', () => {
+      wrapper.instance().setClipboard('cool container');
+
+      expect(Clipboard).toHaveBeenCalledWith('cool container', {});
+    });
   });
 });