Skip to content

Commit 66acfb1

Browse files
authored
Support overlays in react portals in different document (#12445)
### 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 - Shopify/shopify-app-bridge#316 - Shopify/shopify-app-bridge#264 (comment) - Shopify/shopify-app-bridge#238 (comment) - Shopify/shopify-app-bridge#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 ( <Modal> <AppProvider i18n={{}}> <div style={{padding: '75px 0'}}> <Tooltip content="This order has shipping labels."> <Text fontWeight="bold" as="span"> Order #1001 </Text> </Tooltip> <ButtonGroup variant="segmented" fullWidth> <Tooltip content="Bold" dismissOnMouseOut> <Button>B</Button> </Tooltip> <Tooltip content="Italic" dismissOnMouseOut> <Button>I</Button> </Tooltip> <Tooltip content="Underline" dismissOnMouseOut> <Button>U</Button> </Tooltip> <Tooltip content="Strikethrough" dismissOnMouseOut> <Button>S</Button> </Tooltip> </ButtonGroup> </div> </AppProvider> </Modal> ); } ``` **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~
1 parent e7e5bd6 commit 66acfb1

File tree

26 files changed

+899
-80
lines changed

26 files changed

+899
-80
lines changed

.changeset/four-kids-nail.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/polaris': minor
3+
---
4+
5+
Fixed issues with Popover, Tooltip, Color Picker and Drop Zone not working correctly when used inside Modals for embedded apps.

polaris-react/src/components/ColorPicker/ColorPicker.tsx

+10-4
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ import {clamp} from '../../utilities/clamp';
55
import {classNames} from '../../utilities/css';
66
import {hsbToRgb} from '../../utilities/color-transformers';
77
import type {HSBColor, HSBAColor} from '../../utilities/color-types';
8-
// eslint-disable-next-line import/no-deprecated
9-
import {EventListener} from '../EventListener';
108

119
import {AlphaPicker, HuePicker, Slidable} from './components';
1210
import type {SlidableProps} from './components';
@@ -67,12 +65,21 @@ export class ColorPicker extends PureComponent<ColorPickerProps, State> {
6765
{leading: true, trailing: true, maxWait: RESIZE_DEBOUNCE_TIME_MS},
6866
);
6967

68+
private observer?: ResizeObserver;
69+
70+
componentWillUnmount() {
71+
this.observer?.disconnect();
72+
}
73+
7074
componentDidMount() {
7175
const {colorNode} = this;
72-
if (colorNode == null) {
76+
if (!colorNode) {
7377
return;
7478
}
7579

80+
this.observer = new ResizeObserver(this.handleResize);
81+
this.observer.observe(colorNode);
82+
7683
this.setState({
7784
pickerSize: {
7885
width: colorNode.clientWidth,
@@ -135,7 +142,6 @@ export class ColorPicker extends PureComponent<ColorPickerProps, State> {
135142
</div>
136143
<HuePicker hue={hue} onChange={this.handleHueChange} />
137144
{alphaSliderMarkup}
138-
<EventListener event="resize" handler={this.handleResize} />
139145
</div>
140146
);
141147
}

polaris-react/src/components/ColorPicker/components/AlphaPicker/AlphaPicker.tsx

+30-3
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,24 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
2525
draggerHeight: 0,
2626
};
2727

28+
private node: HTMLElement | null = null;
29+
private observer?: ResizeObserver;
30+
31+
componentWillUnmount() {
32+
this.observer?.disconnect();
33+
}
34+
35+
componentDidMount() {
36+
if (!this.node) {
37+
return;
38+
}
39+
40+
this.observer = new ResizeObserver(this.setSliderHeight);
41+
this.observer.observe(this.node);
42+
43+
this.setSliderHeight();
44+
}
45+
2846
render() {
2947
const {color, alpha} = this.props;
3048
const {sliderHeight, draggerHeight} = this.state;
@@ -33,7 +51,7 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
3351
const background = alphaGradientForColor(color);
3452

3553
return (
36-
<div className={styles.AlphaPicker} ref={this.setSliderHeight}>
54+
<div className={styles.AlphaPicker} ref={this.setNode}>
3755
<div className={styles.ColorLayer} style={{background}} />
3856
<Slidable
3957
draggerY={draggerY}
@@ -45,8 +63,17 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
4563
);
4664
}
4765

48-
private setSliderHeight = (node: HTMLElement | null) => {
49-
if (node == null) {
66+
private setNode = (node: HTMLElement | null) => {
67+
if (!node) {
68+
return;
69+
}
70+
71+
this.node = node;
72+
};
73+
74+
private setSliderHeight = () => {
75+
const {node} = this;
76+
if (!node) {
5077
return;
5178
}
5279

polaris-react/src/components/ColorPicker/components/AlphaPicker/tests/AlphaPicker.test.tsx

+34
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,40 @@ describe('<AlphaPicker />', () => {
7979
});
8080
});
8181
});
82+
83+
describe('Iframe React portal bug fix', () => {
84+
it('observes the resize event for the activator wrapper', () => {
85+
const observe = jest.fn();
86+
87+
// eslint-disable-next-line jest/prefer-spy-on
88+
global.ResizeObserver = jest.fn().mockImplementation(() => ({
89+
observe,
90+
unobserve: jest.fn(),
91+
disconnect: jest.fn(),
92+
}));
93+
94+
const alphaPicker = mountWithApp(<AlphaPicker {...mockProps} />);
95+
96+
expect(observe).toHaveBeenCalledWith(alphaPicker.find('div')?.domNode);
97+
});
98+
99+
it('disconnects the resize observer when component unmounts', () => {
100+
const disconnect = jest.fn();
101+
102+
// eslint-disable-next-line jest/prefer-spy-on
103+
global.ResizeObserver = jest.fn().mockImplementation(() => ({
104+
observe: jest.fn(),
105+
unobserve: jest.fn(),
106+
disconnect,
107+
}));
108+
109+
const alphaPicker = mountWithApp(<AlphaPicker {...mockProps} />);
110+
111+
alphaPicker.unmount();
112+
113+
expect(disconnect).toHaveBeenCalled();
114+
});
115+
});
82116
});
83117

84118
function noop() {}

polaris-react/src/components/ColorPicker/components/HuePicker/HuePicker.tsx

+31-3
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,31 @@ export class HuePicker extends PureComponent<HuePickerProps, State> {
2222
draggerHeight: 0,
2323
};
2424

25+
private node: HTMLElement | null = null;
26+
private observer?: ResizeObserver;
27+
28+
componentWillUnmount() {
29+
this.observer?.disconnect();
30+
}
31+
32+
componentDidMount() {
33+
if (!this.node) {
34+
return;
35+
}
36+
37+
this.observer = new ResizeObserver(this.setSliderHeight);
38+
this.observer.observe(this.node);
39+
40+
this.setSliderHeight();
41+
}
42+
2543
render() {
2644
const {hue} = this.props;
2745
const {sliderHeight, draggerHeight} = this.state;
2846
const draggerY = calculateDraggerY(hue, sliderHeight, draggerHeight);
2947

3048
return (
31-
<div className={styles.HuePicker} ref={this.setSliderHeight}>
49+
<div className={styles.HuePicker} ref={this.setNode}>
3250
<Slidable
3351
draggerY={draggerY}
3452
draggerX={0}
@@ -39,8 +57,18 @@ export class HuePicker extends PureComponent<HuePickerProps, State> {
3957
);
4058
}
4159

42-
private setSliderHeight = (node: HTMLElement | null) => {
43-
if (node == null) {
60+
private setNode = (node: HTMLElement | null) => {
61+
if (!node) {
62+
return;
63+
}
64+
65+
this.node = node;
66+
};
67+
68+
private setSliderHeight = () => {
69+
const {node} = this;
70+
71+
if (!node) {
4472
return;
4573
}
4674

polaris-react/src/components/ColorPicker/components/HuePicker/tests/HuePicker.test.tsx

+34
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,40 @@ describe('<HuePicker />', () => {
6767
});
6868
});
6969
});
70+
71+
describe('Iframe React portal bug fix', () => {
72+
it('observes the resize event for the activator wrapper', () => {
73+
const observe = jest.fn();
74+
75+
// eslint-disable-next-line jest/prefer-spy-on
76+
global.ResizeObserver = jest.fn().mockImplementation(() => ({
77+
observe,
78+
unobserve: jest.fn(),
79+
disconnect: jest.fn(),
80+
}));
81+
82+
const huePicker = mountWithApp(<HuePicker {...mockProps} />);
83+
84+
expect(observe).toHaveBeenCalledWith(huePicker.find('div')?.domNode);
85+
});
86+
87+
it('disconnects the resize observer when component unmounts', () => {
88+
const disconnect = jest.fn();
89+
90+
// eslint-disable-next-line jest/prefer-spy-on
91+
global.ResizeObserver = jest.fn().mockImplementation(() => ({
92+
observe: jest.fn(),
93+
unobserve: jest.fn(),
94+
disconnect,
95+
}));
96+
97+
const huePicker = mountWithApp(<HuePicker {...mockProps} />);
98+
99+
huePicker.unmount();
100+
101+
expect(disconnect).toHaveBeenCalled();
102+
});
103+
});
70104
});
71105

72106
function noop() {}

polaris-react/src/components/ColorPicker/components/Slidable/Slidable.tsx

+63-16
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ interface Position {
1212

1313
interface State {
1414
dragging: boolean;
15+
window?: Window | null;
1516
}
1617

1718
export interface SlidableProps {
@@ -49,29 +50,40 @@ export class Slidable extends PureComponent<SlidableProps, State> {
4950

5051
private node: HTMLElement | null = null;
5152
private draggerNode: HTMLElement | null = null;
53+
private observer?: ResizeObserver;
54+
55+
componentWillUnmount() {
56+
this.observer?.disconnect();
57+
}
5258

5359
componentDidMount() {
54-
const {onDraggerHeight} = this.props;
55-
if (onDraggerHeight == null) {
60+
if (!this.node) {
5661
return;
5762
}
5863

59-
const {draggerNode} = this;
60-
if (draggerNode == null) {
61-
return;
62-
}
64+
this.observer = new ResizeObserver(() => {
65+
/**
66+
* This is a workaround to enable event listeners to be
67+
* re-attached when moving from one document to another
68+
* when using a React portal across iframes.
69+
* Using a resize observer works because when the clientWidth
70+
* will go from 0 to the real width after the node
71+
* gets rendered in its new place.
72+
*/
73+
const {window} = this.state;
74+
if (window !== this.node?.ownerDocument.defaultView) {
75+
this.setState({window: this.node?.ownerDocument.defaultView});
76+
}
77+
this.handleResize();
78+
});
6379

64-
onDraggerHeight(draggerNode.clientWidth);
80+
this.observer.observe(this.node);
6581

66-
if (process.env.NODE_ENV === 'development') {
67-
setTimeout(() => {
68-
onDraggerHeight(draggerNode.clientWidth);
69-
}, 0);
70-
}
82+
this.handleResize();
7183
}
7284

7385
render() {
74-
const {dragging} = this.state;
86+
const {dragging, window} = this.state;
7587
const {draggerX = 0, draggerY = 0} = this.props;
7688

7789
const draggerPositioning = {
@@ -83,6 +95,7 @@ export class Slidable extends PureComponent<SlidableProps, State> {
8395
event="mousemove"
8496
handler={this.handleMove}
8597
passive={false}
98+
window={window}
8699
/>
87100
) : null;
88101

@@ -91,19 +104,32 @@ export class Slidable extends PureComponent<SlidableProps, State> {
91104
event="touchmove"
92105
handler={this.handleMove}
93106
passive={false}
107+
window={window}
94108
/>
95109
) : null;
96110

97111
const endDragListener = dragging ? (
98-
<EventListener event="mouseup" handler={this.handleDragEnd} />
112+
<EventListener
113+
event="mouseup"
114+
handler={this.handleDragEnd}
115+
window={window}
116+
/>
99117
) : null;
100118

101119
const touchEndListener = dragging ? (
102-
<EventListener event="touchend" handler={this.handleDragEnd} />
120+
<EventListener
121+
event="touchend"
122+
handler={this.handleDragEnd}
123+
window={window}
124+
/>
103125
) : null;
104126

105127
const touchCancelListener = dragging ? (
106-
<EventListener event="touchcancel" handler={this.handleDragEnd} />
128+
<EventListener
129+
event="touchcancel"
130+
handler={this.handleDragEnd}
131+
window={window}
132+
/>
107133
) : null;
108134

109135
return (
@@ -127,6 +153,27 @@ export class Slidable extends PureComponent<SlidableProps, State> {
127153
);
128154
}
129155

156+
private handleResize() {
157+
const {onDraggerHeight} = this.props;
158+
if (!onDraggerHeight) {
159+
return;
160+
}
161+
162+
const {draggerNode} = this;
163+
164+
if (!draggerNode) {
165+
return;
166+
}
167+
168+
onDraggerHeight(draggerNode.clientWidth);
169+
170+
if (process.env.NODE_ENV === 'development') {
171+
setTimeout(() => {
172+
onDraggerHeight(draggerNode.clientWidth);
173+
}, 0);
174+
}
175+
}
176+
130177
private setDraggerNode = (node: HTMLElement | null) => {
131178
this.draggerNode = node;
132179
};

0 commit comments

Comments
 (0)