Skip to content

Commit

Permalink
Fix Popover, Tooltip, Color Picker and Drop Zone when rendering insid…
Browse files Browse the repository at this point in the history
…e iframes

- ensure references to document and window are updated to the component's ownerDocument so
when elements are moved inside of an iframes the position can be calculated correctly
- ensure 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 across iframes since elements will be cloned as objects

Fix Color Picker and Slidable

Fix popover auto-focus and click outside handler
  • Loading branch information
vividviolet committed Jul 30, 2024
1 parent 2f98294 commit b5b89b1
Show file tree
Hide file tree
Showing 26 changed files with 891 additions and 80 deletions.
5 changes: 5 additions & 0 deletions .changeset/four-kids-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Enable overlays to work inside React portals
14 changes: 10 additions & 4 deletions polaris-react/src/components/ColorPicker/ColorPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -67,12 +65,21 @@ export class ColorPicker extends PureComponent<ColorPickerProps, State> {
{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,
Expand Down Expand Up @@ -135,7 +142,6 @@ export class ColorPicker extends PureComponent<ColorPickerProps, State> {
</div>
<HuePicker hue={hue} onChange={this.handleHueChange} />
{alphaSliderMarkup}
<EventListener event="resize" handler={this.handleResize} />
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,24 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
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;
Expand All @@ -33,7 +51,7 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
const background = alphaGradientForColor(color);

return (
<div className={styles.AlphaPicker} ref={this.setSliderHeight}>
<div className={styles.AlphaPicker} ref={this.setNode}>
<div className={styles.ColorLayer} style={{background}} />
<Slidable
draggerY={draggerY}
Expand All @@ -45,8 +63,17 @@ export class AlphaPicker extends PureComponent<AlphaPickerProps, State> {
);
}

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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,40 @@ describe('<AlphaPicker />', () => {
});
});
});

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(<AlphaPicker {...mockProps} />);

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 {...mockProps} />);

alphaPicker.unmount();

expect(disconnect).toHaveBeenCalled();
});
});
});

function noop() {}
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,31 @@ export class HuePicker extends PureComponent<HuePickerProps, State> {
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 (
<div className={styles.HuePicker} ref={this.setSliderHeight}>
<div className={styles.HuePicker} ref={this.setNode}>
<Slidable
draggerY={draggerY}
draggerX={0}
Expand All @@ -39,8 +57,18 @@ export class HuePicker extends PureComponent<HuePickerProps, State> {
);
}

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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,40 @@ describe('<HuePicker />', () => {
});
});
});

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(<HuePicker {...mockProps} />);

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 {...mockProps} />);

huePicker.unmount();

expect(disconnect).toHaveBeenCalled();
});
});
});

function noop() {}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ interface Position {

interface State {
dragging: boolean;
window?: Window | null;
}

export interface SlidableProps {
Expand Down Expand Up @@ -49,29 +50,40 @@ export class Slidable extends PureComponent<SlidableProps, State> {

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 = {
Expand All @@ -83,6 +95,7 @@ export class Slidable extends PureComponent<SlidableProps, State> {
event="mousemove"
handler={this.handleMove}
passive={false}
window={window}
/>
) : null;

Expand All @@ -91,19 +104,32 @@ export class Slidable extends PureComponent<SlidableProps, State> {
event="touchmove"
handler={this.handleMove}
passive={false}
window={window}
/>
) : null;

const endDragListener = dragging ? (
<EventListener event="mouseup" handler={this.handleDragEnd} />
<EventListener
event="mouseup"
handler={this.handleDragEnd}
window={window}
/>
) : null;

const touchEndListener = dragging ? (
<EventListener event="touchend" handler={this.handleDragEnd} />
<EventListener
event="touchend"
handler={this.handleDragEnd}
window={window}
/>
) : null;

const touchCancelListener = dragging ? (
<EventListener event="touchcancel" handler={this.handleDragEnd} />
<EventListener
event="touchcancel"
handler={this.handleDragEnd}
window={window}
/>
) : null;

return (
Expand All @@ -127,6 +153,27 @@ export class Slidable extends PureComponent<SlidableProps, State> {
);
}

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;
};
Expand Down
Loading

0 comments on commit b5b89b1

Please sign in to comment.