From 66acfb1c9bf10ccfbdc12d1b71b2c709632ed98b Mon Sep 17 00:00:00 2001 From: Trish Ta Date: Thu, 1 Aug 2024 10:15:31 -0400 Subject: [PATCH] Support overlays in react portals in different document (#12445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### WHY are these changes introduced? Fix issues related to overlays not working correct in App Bridge modals as outlined in this [doc](https://docs.google.com/document/d/1eocCl07pk8xt7rvUaGDWZzfJxxeJ2yRi1ul0x0ezkDM/edit). These specific issues will be fixed - https://github.com/Shopify/shopify-app-bridge/issues/316 - https://github.com/Shopify/shopify-app-bridge/issues/264#issuecomment-2129535852 - https://github.com/Shopify/shopify-app-bridge/issues/238#issuecomment-1980064764 - https://github.com/Shopify/shopify-app-bridge/issues/301 ### WHAT is this pull request doing? Currently Overlays are broken when rendered using React portals. This PR fixes the issue by doing the following: - ensuring all references to `document` are updated to be `node.ownerDocument` instead so the positioning can be calculated correctly inside of iframes - ensuring wherever we do `instanceOf HTMLElement` we instead just check to see if the node is present or try to call the related methods (`getBoundingClientRect`) with a fallback as the `instanceof` check will fail when using React portals for iframe content since iframes have their own globals This PR only touches the related utils for Overlays but there are other references to `document` and `instanceOf HTMLElement` we should also update. Expected usage with App Bridge v4, note that you have to wrap the modal's content with the AppProvider from Polaris so that the portals are created in the correct place. For example, here's a modal with tooltips inside: ```jsx import {AppProvider} from '@shopify/polaris'; import {Modal} from '@shopify/app-bridge-react'; function App() { return (
Order #1001
); } ``` **Before** https://github.com/user-attachments/assets/f7b7a7df-f25f-422c-a5bc-d1a8d7f12167 **After** https://github.com/user-attachments/assets/6c1bf97a-e8f4-4715-9960-9f5709222f75 https://github.com/user-attachments/assets/ebc4755e-cc3b-413a-aaf5-9f39cc7b7ad0 ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) 1. Open the App Bridge Playground: https://admin.shopify.com/store/trish-dev/apps/app-bridge-next-playground-4/react/index.html 2. Open all the custom content modals and do a smoke test to verify overlay components are rendered on top with correct sizing and click events are working ### 🎩 checklist - [X] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [X] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [X] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) ~- [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)~ ~- [ ] Updated the component's `README.md` with documentation changes~ ~- [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide~ --- .changeset/four-kids-nail.md | 5 + .../components/ColorPicker/ColorPicker.tsx | 14 +- .../components/AlphaPicker/AlphaPicker.tsx | 33 +++- .../AlphaPicker/tests/AlphaPicker.test.tsx | 34 ++++ .../components/HuePicker/HuePicker.tsx | 34 +++- .../HuePicker/tests/HuePicker.test.tsx | 34 ++++ .../components/Slidable/Slidable.tsx | 79 ++++++-- .../Slidable/tests/Slidable.test.tsx | 130 ++++++++++++ .../ColorPicker/tests/ColorPicker.test.tsx | 40 ++++ .../src/components/DropZone/DropZone.tsx | 20 +- .../DropZone/tests/DropZone.test.tsx | 41 ++++ .../EventListener/EventListener.tsx | 12 +- .../tests/EventListener.test.tsx | 21 ++ .../KeypressListener/KeypressListener.tsx | 12 +- .../tests/KeypressListener.test.tsx | 21 ++ .../src/components/Popover/Popover.tsx | 74 +++++-- .../PopoverOverlay/PopoverOverlay.tsx | 44 ++++- .../tests/PopoverOverlay.test.tsx | 185 +++++++++++++++++- .../components/Popover/tests/Popover.test.tsx | 57 ++++++ .../PositionedOverlay/PositionedOverlay.tsx | 36 ++-- .../PositionedOverlay/utilities/math.ts | 5 +- .../src/components/Tooltip/Tooltip.tsx | 5 +- polaris-react/src/utilities/geometry.ts | 26 ++- .../src/utilities/is-element-in-viewport.ts | 1 + polaris-react/tests/setup/environment.ts | 9 + polaris-react/tests/setup/tests.ts | 7 + 26 files changed, 899 insertions(+), 80 deletions(-) create mode 100644 .changeset/four-kids-nail.md diff --git a/.changeset/four-kids-nail.md b/.changeset/four-kids-nail.md new file mode 100644 index 00000000000..0653c576d19 --- /dev/null +++ b/.changeset/four-kids-nail.md @@ -0,0 +1,5 @@ +--- +'@shopify/polaris': minor +--- + +Fixed issues with Popover, Tooltip, Color Picker and Drop Zone not working correctly when used inside Modals for embedded apps. diff --git a/polaris-react/src/components/ColorPicker/ColorPicker.tsx b/polaris-react/src/components/ColorPicker/ColorPicker.tsx index a768ed83625..5323bfbfaac 100644 --- a/polaris-react/src/components/ColorPicker/ColorPicker.tsx +++ b/polaris-react/src/components/ColorPicker/ColorPicker.tsx @@ -5,8 +5,6 @@ import {clamp} from '../../utilities/clamp'; import {classNames} from '../../utilities/css'; import {hsbToRgb} from '../../utilities/color-transformers'; import type {HSBColor, HSBAColor} from '../../utilities/color-types'; -// eslint-disable-next-line import/no-deprecated -import {EventListener} from '../EventListener'; import {AlphaPicker, HuePicker, Slidable} from './components'; import type {SlidableProps} from './components'; @@ -67,12 +65,21 @@ export class ColorPicker extends PureComponent { {leading: true, trailing: true, maxWait: RESIZE_DEBOUNCE_TIME_MS}, ); + private observer?: ResizeObserver; + + componentWillUnmount() { + this.observer?.disconnect(); + } + componentDidMount() { const {colorNode} = this; - if (colorNode == null) { + if (!colorNode) { return; } + this.observer = new ResizeObserver(this.handleResize); + this.observer.observe(colorNode); + this.setState({ pickerSize: { width: colorNode.clientWidth, @@ -135,7 +142,6 @@ export class ColorPicker extends PureComponent { {alphaSliderMarkup} - ); } diff --git a/polaris-react/src/components/ColorPicker/components/AlphaPicker/AlphaPicker.tsx b/polaris-react/src/components/ColorPicker/components/AlphaPicker/AlphaPicker.tsx index 2beadfb8e20..541e84b1725 100644 --- a/polaris-react/src/components/ColorPicker/components/AlphaPicker/AlphaPicker.tsx +++ b/polaris-react/src/components/ColorPicker/components/AlphaPicker/AlphaPicker.tsx @@ -25,6 +25,24 @@ export class AlphaPicker extends PureComponent { draggerHeight: 0, }; + private node: HTMLElement | null = null; + private observer?: ResizeObserver; + + componentWillUnmount() { + this.observer?.disconnect(); + } + + componentDidMount() { + if (!this.node) { + return; + } + + this.observer = new ResizeObserver(this.setSliderHeight); + this.observer.observe(this.node); + + this.setSliderHeight(); + } + render() { const {color, alpha} = this.props; const {sliderHeight, draggerHeight} = this.state; @@ -33,7 +51,7 @@ export class AlphaPicker extends PureComponent { const background = alphaGradientForColor(color); return ( -
+
{ ); } - private setSliderHeight = (node: HTMLElement | null) => { - if (node == null) { + private setNode = (node: HTMLElement | null) => { + if (!node) { + return; + } + + this.node = node; + }; + + private setSliderHeight = () => { + const {node} = this; + if (!node) { return; } diff --git a/polaris-react/src/components/ColorPicker/components/AlphaPicker/tests/AlphaPicker.test.tsx b/polaris-react/src/components/ColorPicker/components/AlphaPicker/tests/AlphaPicker.test.tsx index b1ae4399efb..5d959da9bdd 100644 --- a/polaris-react/src/components/ColorPicker/components/AlphaPicker/tests/AlphaPicker.test.tsx +++ b/polaris-react/src/components/ColorPicker/components/AlphaPicker/tests/AlphaPicker.test.tsx @@ -79,6 +79,40 @@ describe('', () => { }); }); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator wrapper', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const alphaPicker = mountWithApp(); + + expect(observe).toHaveBeenCalledWith(alphaPicker.find('div')?.domNode); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const alphaPicker = mountWithApp(); + + alphaPicker.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + }); }); function noop() {} diff --git a/polaris-react/src/components/ColorPicker/components/HuePicker/HuePicker.tsx b/polaris-react/src/components/ColorPicker/components/HuePicker/HuePicker.tsx index c6c9327d35c..89f29ddc8b1 100644 --- a/polaris-react/src/components/ColorPicker/components/HuePicker/HuePicker.tsx +++ b/polaris-react/src/components/ColorPicker/components/HuePicker/HuePicker.tsx @@ -22,13 +22,31 @@ export class HuePicker extends PureComponent { draggerHeight: 0, }; + private node: HTMLElement | null = null; + private observer?: ResizeObserver; + + componentWillUnmount() { + this.observer?.disconnect(); + } + + componentDidMount() { + if (!this.node) { + return; + } + + this.observer = new ResizeObserver(this.setSliderHeight); + this.observer.observe(this.node); + + this.setSliderHeight(); + } + render() { const {hue} = this.props; const {sliderHeight, draggerHeight} = this.state; const draggerY = calculateDraggerY(hue, sliderHeight, draggerHeight); return ( -
+
{ ); } - private setSliderHeight = (node: HTMLElement | null) => { - if (node == null) { + private setNode = (node: HTMLElement | null) => { + if (!node) { + return; + } + + this.node = node; + }; + + private setSliderHeight = () => { + const {node} = this; + + if (!node) { return; } diff --git a/polaris-react/src/components/ColorPicker/components/HuePicker/tests/HuePicker.test.tsx b/polaris-react/src/components/ColorPicker/components/HuePicker/tests/HuePicker.test.tsx index 029c02de628..22fa6e7b3bf 100644 --- a/polaris-react/src/components/ColorPicker/components/HuePicker/tests/HuePicker.test.tsx +++ b/polaris-react/src/components/ColorPicker/components/HuePicker/tests/HuePicker.test.tsx @@ -67,6 +67,40 @@ describe('', () => { }); }); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator wrapper', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const huePicker = mountWithApp(); + + expect(observe).toHaveBeenCalledWith(huePicker.find('div')?.domNode); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const huePicker = mountWithApp(); + + huePicker.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + }); }); function noop() {} diff --git a/polaris-react/src/components/ColorPicker/components/Slidable/Slidable.tsx b/polaris-react/src/components/ColorPicker/components/Slidable/Slidable.tsx index acd969a86bc..e16038dfdd8 100644 --- a/polaris-react/src/components/ColorPicker/components/Slidable/Slidable.tsx +++ b/polaris-react/src/components/ColorPicker/components/Slidable/Slidable.tsx @@ -12,6 +12,7 @@ interface Position { interface State { dragging: boolean; + window?: Window | null; } export interface SlidableProps { @@ -49,29 +50,40 @@ export class Slidable extends PureComponent { private node: HTMLElement | null = null; private draggerNode: HTMLElement | null = null; + private observer?: ResizeObserver; + + componentWillUnmount() { + this.observer?.disconnect(); + } componentDidMount() { - const {onDraggerHeight} = this.props; - if (onDraggerHeight == null) { + if (!this.node) { return; } - const {draggerNode} = this; - if (draggerNode == null) { - return; - } + this.observer = new ResizeObserver(() => { + /** + * This is a workaround to enable event listeners to be + * re-attached when moving from one document to another + * when using a React portal across iframes. + * Using a resize observer works because when the clientWidth + * will go from 0 to the real width after the node + * gets rendered in its new place. + */ + const {window} = this.state; + if (window !== this.node?.ownerDocument.defaultView) { + this.setState({window: this.node?.ownerDocument.defaultView}); + } + this.handleResize(); + }); - onDraggerHeight(draggerNode.clientWidth); + this.observer.observe(this.node); - if (process.env.NODE_ENV === 'development') { - setTimeout(() => { - onDraggerHeight(draggerNode.clientWidth); - }, 0); - } + this.handleResize(); } render() { - const {dragging} = this.state; + const {dragging, window} = this.state; const {draggerX = 0, draggerY = 0} = this.props; const draggerPositioning = { @@ -83,6 +95,7 @@ export class Slidable extends PureComponent { event="mousemove" handler={this.handleMove} passive={false} + window={window} /> ) : null; @@ -91,19 +104,32 @@ export class Slidable extends PureComponent { event="touchmove" handler={this.handleMove} passive={false} + window={window} /> ) : null; const endDragListener = dragging ? ( - + ) : null; const touchEndListener = dragging ? ( - + ) : null; const touchCancelListener = dragging ? ( - + ) : null; return ( @@ -127,6 +153,27 @@ export class Slidable extends PureComponent { ); } + private handleResize() { + const {onDraggerHeight} = this.props; + if (!onDraggerHeight) { + return; + } + + const {draggerNode} = this; + + if (!draggerNode) { + return; + } + + onDraggerHeight(draggerNode.clientWidth); + + if (process.env.NODE_ENV === 'development') { + setTimeout(() => { + onDraggerHeight(draggerNode.clientWidth); + }, 0); + } + } + private setDraggerNode = (node: HTMLElement | null) => { this.draggerNode = node; }; diff --git a/polaris-react/src/components/ColorPicker/components/Slidable/tests/Slidable.test.tsx b/polaris-react/src/components/ColorPicker/components/Slidable/tests/Slidable.test.tsx index 190772355cb..c53215302de 100644 --- a/polaris-react/src/components/ColorPicker/components/Slidable/tests/Slidable.test.tsx +++ b/polaris-react/src/components/ColorPicker/components/Slidable/tests/Slidable.test.tsx @@ -1,6 +1,9 @@ +/* eslint-disable import/no-deprecated */ import React from 'react'; import {mountWithApp} from 'tests/utilities'; +import {EventListener} from '../../../../EventListener'; +import styles from '../../../ColorPicker.module.css'; import {Slidable} from '../Slidable'; describe('', () => { @@ -10,4 +13,131 @@ describe('', () => { slidable.find('div')!.trigger('onMouseDown', {}); expect(spy).not.toHaveBeenCalled(); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const slidable = mountWithApp(); + + expect(observe).toHaveBeenCalledWith(slidable.find('div')?.domNode); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const slidable = mountWithApp(); + + slidable.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + + it('passes the node.ownerDocument.defaultView as the window for all event listeners after resize observer fires', () => { + let resizeCallback: () => void; + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation((callback) => { + resizeCallback = callback; + return { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + }; + }); + + const slidable = mountWithApp(); + const node = slidable.find('div')?.domNode; + + slidable.act(() => resizeCallback()); + + slidable.forceUpdate(); + + slidable.find('div')!.trigger('onMouseDown', { + type: 'mousedown', + clientX: 1, + clientY: 1, + }); + + // We can't assert using `toContainReactComponent` because the matcher blows up trying to assert on window as an argument + expect( + slidable + .find(EventListener, { + event: 'mousemove', + })! + .prop('window'), + ).toStrictEqual(node!.ownerDocument.defaultView); + + expect( + slidable + .find(EventListener, { + event: 'touchmove', + })! + .prop('window'), + ).toStrictEqual(node!.ownerDocument.defaultView); + + expect( + slidable + .find(EventListener, { + event: 'mouseup', + })! + .prop('window'), + ).toStrictEqual(node!.ownerDocument.defaultView); + + expect( + slidable + .find(EventListener, { + event: 'touchend', + })! + .prop('window'), + ).toStrictEqual(node!.ownerDocument.defaultView); + + expect( + slidable + .find(EventListener, { + event: 'touchcancel', + })! + .prop('window'), + ).toStrictEqual(node!.ownerDocument.defaultView); + }); + + it('calls provided onDraggerHeight after resize observer fires', () => { + let resizeCallback: () => void; + const onDraggerHeight = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation((callback) => { + resizeCallback = callback; + return { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + }; + }); + + const slidable = mountWithApp( + , + ); + + slidable.act(() => resizeCallback()); + + expect(onDraggerHeight).toHaveBeenCalledWith( + slidable.find('div', {className: styles.Dragger})?.domNode?.clientWidth, + ); + }); + }); }); diff --git a/polaris-react/src/components/ColorPicker/tests/ColorPicker.test.tsx b/polaris-react/src/components/ColorPicker/tests/ColorPicker.test.tsx index 5040d2847ad..c9996c50ebb 100644 --- a/polaris-react/src/components/ColorPicker/tests/ColorPicker.test.tsx +++ b/polaris-react/src/components/ColorPicker/tests/ColorPicker.test.tsx @@ -5,6 +5,7 @@ import {mountWithApp} from 'tests/utilities'; import {EventListener} from '../../EventListener'; import {Slidable, AlphaPicker} from '../components'; import {ColorPicker} from '../ColorPicker'; +import styles from '../ColorPicker.module.css'; const red = { hue: 0, @@ -179,6 +180,45 @@ describe('', () => { expect(event.preventDefault).not.toHaveBeenCalled(); }); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator wrapper', () => { + const observe = jest.fn(); + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const colorPicker = mountWithApp( + , + ); + + expect(observe).toHaveBeenCalledWith( + colorPicker.find('div', {className: styles.MainColor})?.domNode, + ); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const colorPicker = mountWithApp( + , + ); + + colorPicker.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + }); }); function noop() {} diff --git a/polaris-react/src/components/DropZone/DropZone.tsx b/polaris-react/src/components/DropZone/DropZone.tsx index 25ed2d3ac32..f20c46a86e6 100755 --- a/polaris-react/src/components/DropZone/DropZone.tsx +++ b/polaris-react/src/components/DropZone/DropZone.tsx @@ -266,6 +266,10 @@ export const DropZone: React.FunctionComponent & { [disabled, onDragOver], ); + const document = isServer + ? null + : node.current?.ownerDocument || globalThis.document; + const handleDragLeave = useCallback( (event: DropZoneEvent) => { event.preventDefault(); @@ -285,7 +289,7 @@ export const DropZone: React.FunctionComponent & { onDragLeave && onDragLeave(); }, - [dropOnPage, disabled, onDragLeave], + [disabled, onDragLeave, dropOnPage, document], ); const dropNode = dropOnPage && !isServer ? document : node.current; @@ -294,12 +298,24 @@ export const DropZone: React.FunctionComponent & { useEventListener('dragover', handleDragOver, dropNode); useEventListener('dragenter', handleDragEnter, dropNode); useEventListener('dragleave', handleDragLeave, dropNode); - useEventListener('resize', adjustSize, isServer ? null : window); useComponentDidMount(() => { adjustSize(); }); + useEffect(() => { + if (!node.current) { + return; + } + + const observer = new ResizeObserver(adjustSize); + observer.observe(node.current); + + return () => { + observer.disconnect(); + }; + }, [adjustSize]); + const uniqId = useId(); const id = idProp ?? uniqId; diff --git a/polaris-react/src/components/DropZone/tests/DropZone.test.tsx b/polaris-react/src/components/DropZone/tests/DropZone.test.tsx index 6a25e998107..c631fe12302 100755 --- a/polaris-react/src/components/DropZone/tests/DropZone.test.tsx +++ b/polaris-react/src/components/DropZone/tests/DropZone.test.tsx @@ -10,6 +10,7 @@ import {Labelled} from '../../Labelled'; import {DropZone} from '../DropZone'; import type {DropZoneFileType} from '../DropZone'; import {DropZoneContext} from '../context'; +import styles from '../DropZone.module.css'; const files = [ { @@ -532,6 +533,46 @@ describe('', () => { }).not.toThrow(); }); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator wrapper', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const dropZone = mountWithApp(); + + expect(observe).toHaveBeenCalledWith( + dropZone + .findAll('div') + .filter((node) => + node.domNode?.getAttribute('class')?.includes(styles.DropZone), + )[0]?.domNode, + ); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const dropZone = mountWithApp(); + + dropZone.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + }); }); function createEvent(name: string, files: any) { diff --git a/polaris-react/src/components/EventListener/EventListener.tsx b/polaris-react/src/components/EventListener/EventListener.tsx index 00a2a2f91be..ecc3b372fcb 100644 --- a/polaris-react/src/components/EventListener/EventListener.tsx +++ b/polaris-react/src/components/EventListener/EventListener.tsx @@ -4,6 +4,7 @@ interface BaseEventProps { event: string; capture?: boolean; handler(event: Event): void; + window?: Window | null; } export interface EventListenerProps extends BaseEventProps { @@ -30,12 +31,19 @@ export class EventListener extends PureComponent { } private attachListener() { - const {event, handler, capture, passive} = this.props; + const {event, handler, capture, passive, window: customWindow} = this.props; + const window = customWindow || globalThis.window; window.addEventListener(event, handler, {capture, passive}); } private detachListener(prevProps?: BaseEventProps) { - const {event, handler, capture} = prevProps || this.props; + const { + event, + handler, + capture, + window: customWindow, + } = prevProps || this.props; + const window = customWindow || globalThis.window; window.removeEventListener(event, handler, capture); } } diff --git a/polaris-react/src/components/EventListener/tests/EventListener.test.tsx b/polaris-react/src/components/EventListener/tests/EventListener.test.tsx index 88d4248f2d4..760e31844aa 100644 --- a/polaris-react/src/components/EventListener/tests/EventListener.test.tsx +++ b/polaris-react/src/components/EventListener/tests/EventListener.test.tsx @@ -35,6 +35,27 @@ describe('', () => { window.dispatchEvent(new Event('resize')); expect(spy).not.toHaveBeenCalled(); }); + + it('uses the custom window if provided', () => { + const myWindow = { + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + }; + + const wrapper = mountWithApp( + , + ); + + expect(myWindow.addEventListener).toHaveBeenCalled(); + + wrapper.unmount(); + + expect(myWindow.removeEventListener).toHaveBeenCalled(); + }); }); function noop() {} diff --git a/polaris-react/src/components/KeypressListener/KeypressListener.tsx b/polaris-react/src/components/KeypressListener/KeypressListener.tsx index d9a24e965a3..9e99156383d 100644 --- a/polaris-react/src/components/KeypressListener/KeypressListener.tsx +++ b/polaris-react/src/components/KeypressListener/KeypressListener.tsx @@ -7,6 +7,7 @@ export interface NonMutuallyExclusiveProps { keyCode: Key; handler(event: KeyboardEvent): void; keyEvent?: KeyEvent; + document?: Document; } export type KeypressListenerProps = NonMutuallyExclusiveProps & @@ -23,6 +24,7 @@ export function KeypressListener({ keyEvent = 'keyup', options, useCapture, + document: ownerDocument = globalThis.document, }: KeypressListenerProps) { const tracked = useRef({handler, keyCode}); @@ -38,15 +40,19 @@ export function KeypressListener({ }, []); useEffect(() => { - document.addEventListener(keyEvent, handleKeyEvent, useCapture || options); + ownerDocument.addEventListener( + keyEvent, + handleKeyEvent, + useCapture || options, + ); return () => { - document.removeEventListener( + ownerDocument.removeEventListener( keyEvent, handleKeyEvent, useCapture || options, ); }; - }, [keyEvent, handleKeyEvent, useCapture, options]); + }, [keyEvent, handleKeyEvent, useCapture, options, ownerDocument]); return null; } diff --git a/polaris-react/src/components/KeypressListener/tests/KeypressListener.test.tsx b/polaris-react/src/components/KeypressListener/tests/KeypressListener.test.tsx index d759e60f0e4..5574fd1269f 100644 --- a/polaris-react/src/components/KeypressListener/tests/KeypressListener.test.tsx +++ b/polaris-react/src/components/KeypressListener/tests/KeypressListener.test.tsx @@ -83,6 +83,27 @@ describe('', () => { expect(listenerOptionsMap.keyup).toStrictEqual(eventOptions); }); + + it('uses the custom document if provided', () => { + const myDocument = { + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + }; + + const wrapper = mountWithApp( + , + ); + + expect(myDocument.addEventListener).toHaveBeenCalled(); + + wrapper.unmount(); + + expect(myDocument.removeEventListener).toHaveBeenCalled(); + }); }); function noop() {} diff --git a/polaris-react/src/components/Popover/Popover.tsx b/polaris-react/src/components/Popover/Popover.tsx index 58aeb3c68d2..9c8d8761cf8 100644 --- a/polaris-react/src/components/Popover/Popover.tsx +++ b/polaris-react/src/components/Popover/Popover.tsx @@ -110,8 +110,8 @@ const PopoverComponent = forwardRef( }, ref, ) { + const [isDisplayed, setIsDisplay] = useState(false); const [activatorNode, setActivatorNode] = useState(); - const overlayRef = useRef(null); const activatorContainer = useRef(null); @@ -127,7 +127,6 @@ const PopoverComponent = forwardRef( if (activatorContainer.current == null || preventFocusOnClose) { return; } - if (source === PopoverCloseSource.FocusOut && activatorNode) { const focusableActivator = findFirstFocusableNodeIncludingDisabled(activatorNode) || @@ -191,6 +190,42 @@ const PopoverComponent = forwardRef( }); }, [id, active, ariaHaspopup]); + useEffect(() => { + function setDisplayState() { + /** + * This is a workaround to prevent rendering the Popover when the content is moved into + * a React portal that hasn't been rendered. We don't want to render the Popover in this + * case because the auto-focus logic will break. We wait until the activatorContainer is + * displayed, which is when it has an offsetParent, or if the activatorContainer is the + * body, if it has a clientWidth bigger than 0. + * See: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent + */ + + setIsDisplay( + Boolean( + activatorContainer.current && + (activatorContainer.current.offsetParent !== null || + (activatorContainer.current === + activatorContainer.current.ownerDocument.body && + activatorContainer.current.clientWidth > 0)), + ), + ); + } + + if (!activatorContainer.current) { + return; + } + + const observer = new ResizeObserver(setDisplayState); + observer.observe(activatorContainer.current); + + setDisplayState(); + + return () => { + observer.disconnect(); + }; + }, []); + useEffect(() => { if (!activatorNode && activatorContainer.current) { setActivatorNode( @@ -217,23 +252,24 @@ const PopoverComponent = forwardRef( setAccessibilityAttributes(); }, [activatorNode, setAccessibilityAttributes]); - const portal = activatorNode ? ( - - - {children} - - - ) : null; + const portal = + activatorNode && isDisplayed ? ( + + + {children} + + + ) : null; return ( diff --git a/polaris-react/src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx b/polaris-react/src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx index 88971157e7f..6007fd7562b 100644 --- a/polaris-react/src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx +++ b/polaris-react/src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx @@ -59,6 +59,7 @@ export interface PopoverOverlayProps { interface State { transitionStatus: TransitionStatus; + window?: Window | null; } export class PopoverOverlay extends PureComponent { @@ -74,6 +75,7 @@ export class PopoverOverlay extends PureComponent { private contentNode = createRef(); private enteringTimer?: number; private overlayRef: React.RefObject; + private observer?: ResizeObserver; constructor(props: PopoverOverlayProps) { super(props); @@ -97,6 +99,22 @@ export class PopoverOverlay extends PureComponent { this.changeTransitionStatus(TransitionStatus.Entered); } + + this.observer = new ResizeObserver(() => { + this.setState({ + /** + * This is a workaround to enable event listeners to be + * re-attached when moving from one document to another + * when using a React portal across iframes. + * Using a resize observer works because when the clientWidth + * will go from 0 to the real width after the activator + * gets rendered in its new place. + */ + window: this.props.activator.ownerDocument.defaultView, + }); + }); + + this.observer.observe(this.props.activator); } componentDidUpdate(oldProps: PopoverOverlayProps) { @@ -116,10 +134,16 @@ export class PopoverOverlay extends PureComponent { this.clearTransitionTimeout(); this.setState({transitionStatus: TransitionStatus.Exited}); } + + if (this.props.activator !== oldProps.activator) { + this.observer?.unobserve(oldProps.activator); + this.observer?.observe(this.props.activator); + } } componentWillUnmount() { this.clearTransitionTimeout(); + this.observer?.disconnect(); } render() { @@ -232,11 +256,25 @@ export class PopoverOverlay extends PureComponent { fluidContent && styles['Content-fluidContent'], ); + const {window} = this.state; + return (
- - - + + +
', () => { let rafSpy: jest.SpyInstance; @@ -596,6 +598,187 @@ describe('', () => { expect(overlayRef).toHaveProperty('current.forceUpdatePosition'); }); }); + + describe('Iframe React portal bug fix', () => { + it('observes the resize event for the activator', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + mountWithApp( + + {children} + , + ); + + expect(observe).toHaveBeenCalledWith(activator); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const popoverOverlay = mountWithApp( + + {children} + , + ); + + popoverOverlay.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + + it('updates the observer when activator updates', () => { + const observe = jest.fn(); + const unobserve = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve, + disconnect: jest.fn(), + })); + + const popoverOverlay = mountWithApp( + + {children} + , + ); + + const newActivator = document.createElement('button'); + + popoverOverlay.act(() => { + popoverOverlay.setProps({activator: newActivator}); + }); + + expect(unobserve).toHaveBeenCalledWith(activator); + expect(observe).toHaveBeenCalledWith(newActivator); + }); + + it('passes the activator.ownerDocument.defaultView as the window for the click and touchstart event listeners after resize observer fires', () => { + let resizeCallback: () => void; + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation((callback) => { + resizeCallback = callback; + return { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + }; + }); + + const popoverOverlay = mountWithApp( + + {children} + , + ); + + popoverOverlay.act(() => resizeCallback()); + + popoverOverlay.forceUpdate(); + + // We can't assert using `toContainReactComponent` because the matcher blows up trying to assert on window as an argument + expect( + popoverOverlay + .find(EventListener, { + event: 'click', + })! + .prop('window'), + ).toStrictEqual(activator.ownerDocument.defaultView); + + expect( + popoverOverlay + .find(EventListener, { + event: 'touchstart', + })! + .prop('window'), + ).toStrictEqual(activator.ownerDocument.defaultView); + }); + + it('passes the activator.ownerDocument as the document for the escape key press listener after ResizeObserver fires', () => { + let resizeCallback: () => void; + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation((callback) => { + resizeCallback = callback; + return { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + }; + }); + + const popoverOverlay = mountWithApp( + + {children} + , + ); + + popoverOverlay.act(() => resizeCallback()); + + popoverOverlay.forceUpdate(); + + // We can't assert using `toContainReactComponent` because the matcher blows up trying to assert on window as an argument + expect( + popoverOverlay + .find(KeypressListener, { + keyCode: Key.Escape, + })! + .prop('document'), + ).toStrictEqual(activator.ownerDocument); + + expect( + popoverOverlay + .find(EventListener, { + event: 'touchstart', + })! + .prop('window'), + ).toStrictEqual(activator.ownerDocument.defaultView); + }); + }); }); function noop() {} diff --git a/polaris-react/src/components/Popover/tests/Popover.test.tsx b/polaris-react/src/components/Popover/tests/Popover.test.tsx index 8c0ed16402c..d57db7b674e 100644 --- a/polaris-react/src/components/Popover/tests/Popover.test.tsx +++ b/polaris-react/src/components/Popover/tests/Popover.test.tsx @@ -523,6 +523,63 @@ describe('', () => { }); }); }); + + describe('Iframe React portal bug fix', () => { + it('does not render a positionedOverlay when activator node does not have an offsetParent even when active', () => { + // Replace patch for offsetParent for jsdom + Object.defineProperty(HTMLElement.prototype, 'offsetParent', { + get() { + return null; + }, + }); + + const popover = mountWithApp( + Activator
} onClose={spy} />, + ); + expect(popover).not.toContainReactComponent(PositionedOverlay); + }); + + it('observes the resize event for the activator wrapper', () => { + const observe = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe, + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + + const popover = mountWithApp( + Activator
} + activatorWrapper="span" + onClose={spy} + />, + ); + + expect(observe).toHaveBeenCalledWith(popover.find('span')?.domNode); + }); + + it('disconnects the resize observer when component unmounts', () => { + const disconnect = jest.fn(); + + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect, + })); + + const popover = mountWithApp( + Activator
} onClose={spy} />, + ); + + popover.unmount(); + + expect(disconnect).toHaveBeenCalled(); + }); + }); }); function noop() {} diff --git a/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx b/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx index 0c8c0569fdf..3c212784ca6 100644 --- a/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx +++ b/polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx @@ -154,7 +154,11 @@ export class PositionedOverlay extends PureComponent< return (
- + {render(this.overlayDetails())}
); @@ -258,6 +262,8 @@ export class PositionedOverlay extends PureComponent< preferInputActivator = true, } = this.props; + const document = activator.ownerDocument; + const preferredActivator = preferInputActivator ? activator.querySelector('input') || activator : activator; @@ -288,13 +294,16 @@ export class PositionedOverlay extends PureComponent< topBarOffset = topBarElement.clientHeight; } - const overlayMargins = - this.overlay.firstElementChild && - this.overlay.firstChild instanceof HTMLElement - ? getMarginsForNode(this.overlay.firstElementChild as HTMLElement) - : {activator: 0, container: 0, horizontal: 0}; + let overlayMargins = {activator: 0, container: 0, horizontal: 0}; + + if (this.overlay.firstElementChild) { + const nodeMargins = getMarginsForNode( + this.overlay.firstElementChild as HTMLElement, + ); + overlayMargins = nodeMargins; + } - const containerRect = windowRect(); + const containerRect = windowRect(activator); const zIndexForLayer = getZIndexForLayerFromNode(activator); const zIndex = zIndexForLayer == null ? zIndexForLayer : zIndexForLayer + 1; @@ -341,7 +350,10 @@ export class PositionedOverlay extends PureComponent< onScrollOut != null && rectIsOutsideOfRect( activatorRect, - intersectionWithViewport(scrollableContainerRect), + intersectionWithViewport( + scrollableContainerRect, + containerRect, + ), ), zIndex, chevronOffset, @@ -358,6 +370,8 @@ export class PositionedOverlay extends PureComponent< } function getMarginsForNode(node: HTMLElement) { + // Accounts for when the node is moved between documents + const window = node.ownerDocument.defaultView || globalThis.window; const nodeStyles = window.getComputedStyle(node); return { activator: parseFloat(nodeStyles.marginTop || '0'), @@ -367,14 +381,14 @@ function getMarginsForNode(node: HTMLElement) { } function getZIndexForLayerFromNode(node: HTMLElement) { - const layerNode = node.closest(layer.selector) || document.body; + const layerNode = node.closest(layer.selector) || node.ownerDocument.body; const zIndex = - layerNode === document.body + layerNode === node.ownerDocument.body ? 'auto' : parseInt(window.getComputedStyle(layerNode).zIndex || '0', 10); return zIndex === 'auto' || isNaN(zIndex) ? null : zIndex; } function isDocument(node: HTMLElement | Document): node is Document { - return node === document; + return node.ownerDocument === null; } diff --git a/polaris-react/src/components/PositionedOverlay/utilities/math.ts b/polaris-react/src/components/PositionedOverlay/utilities/math.ts index cdce1f82830..e23f9c3812b 100644 --- a/polaris-react/src/components/PositionedOverlay/utilities/math.ts +++ b/polaris-react/src/components/PositionedOverlay/utilities/math.ts @@ -172,7 +172,10 @@ export function intersectionWithViewport( }); } -export function windowRect() { +export function windowRect(node?: HTMLElement) { + const document = node?.ownerDocument || globalThis.document; + const window = document.defaultView || globalThis.window; + return new Rect({ top: window.scrollY, left: window.scrollX, diff --git a/polaris-react/src/components/Tooltip/Tooltip.tsx b/polaris-react/src/components/Tooltip/Tooltip.tsx index ab64dae2618..d46a29bea40 100644 --- a/polaris-react/src/components/Tooltip/Tooltip.tsx +++ b/polaris-react/src/components/Tooltip/Tooltip.tsx @@ -236,8 +236,9 @@ export function Tooltip({ return; } - node.firstElementChild instanceof HTMLElement && - setActivatorNode(node.firstElementChild); + if (node.firstElementChild) { + setActivatorNode(node.firstElementChild as HTMLElement); + } activatorContainerRef.current = node; } diff --git a/polaris-react/src/utilities/geometry.ts b/polaris-react/src/utilities/geometry.ts index 9dc77ce17d9..9653d268909 100644 --- a/polaris-react/src/utilities/geometry.ts +++ b/polaris-react/src/utilities/geometry.ts @@ -38,19 +38,25 @@ export class Rect { export function getRectForNode( node: Element | React.ReactNode | Window | Document, ): Rect { - if (!(node instanceof Element)) { + /** + * NOTE: We cannot do node instanceof Element because it will fail when inside of an iframe. + * Technically we can do `node instanceof node.ownerDocument.defaultView.Element`but this will + * fail when node isn't an Element. We might as well try to run `getBoundingClientRect` and then + * have a fallback for when that breaks. + */ + try { + const rect = (node as Element).getBoundingClientRect(); + + return new Rect({ + top: rect.top, + left: rect.left, + width: rect.width, + height: rect.height, + }); + } catch (_) { return new Rect({ width: window.innerWidth, height: window.innerHeight, }); } - - const rect = node.getBoundingClientRect(); - - return new Rect({ - top: rect.top, - left: rect.left, - width: rect.width, - height: rect.height, - }); } diff --git a/polaris-react/src/utilities/is-element-in-viewport.ts b/polaris-react/src/utilities/is-element-in-viewport.ts index b54ae112086..5b6527cc7ff 100644 --- a/polaris-react/src/utilities/is-element-in-viewport.ts +++ b/polaris-react/src/utilities/is-element-in-viewport.ts @@ -1,5 +1,6 @@ export function isElementInViewport(element: Element) { const {top, left, bottom, right} = element.getBoundingClientRect(); + const window = element.ownerDocument.defaultView || globalThis.window; return ( top >= 0 && diff --git a/polaris-react/tests/setup/environment.ts b/polaris-react/tests/setup/environment.ts index 4a3c0030ae0..62c9cc8c671 100644 --- a/polaris-react/tests/setup/environment.ts +++ b/polaris-react/tests/setup/environment.ts @@ -47,3 +47,12 @@ console.warn = (...args: any[]) => { originalConsoleWarn(...args); }; + +if (typeof HTMLElement !== 'undefined') { + // Patch for offsetParent for jsdom, needed for Popover + Object.defineProperty(HTMLElement.prototype, 'offsetParent', { + get() { + return this.parentNode; + }, + }); +} diff --git a/polaris-react/tests/setup/tests.ts b/polaris-react/tests/setup/tests.ts index 96328e9fd29..eba7784a977 100644 --- a/polaris-react/tests/setup/tests.ts +++ b/polaris-react/tests/setup/tests.ts @@ -11,6 +11,13 @@ if (typeof window !== 'undefined' && !matchMedia.isMocked()) { // eslint-disable-next-line jest/require-top-level-describe beforeEach(() => { + // eslint-disable-next-line jest/prefer-spy-on + global.ResizeObserver = jest.fn().mockImplementation(() => ({ + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + })); + if (typeof window !== 'undefined' && !matchMedia.isMocked()) { matchMedia.mock(); }