facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.3k stars 46.96k forks source link

Possible bug in ReactDOM.createPortal when used in a new window #12355

Closed Drarok closed 6 years ago

Drarok commented 6 years ago

Do you want to request a feature or report a bug?

A bug.

What is the current behavior?

When a ReactDOM.createPortal is used in conjunction with a container in another window, the components do not respond to user input until after setState or forceUpdate are called on the parent component of the portal.

I've produced a CodePen demonstrating the issue.

  1. Click "Open a Portal" – a new window appears with a pair of buttons.
  2. Click either button in the new window – nothing happens.
  3. Click the "Hack" button in the parent window (which simply calls setState({}) on the parent).
  4. Note that the buttons in the new window now work as expected.

What is the expected behavior?

I expect the components in the new window to be interactive.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.2.0 ReactDOM 16.2.0

I've tested this in Safari and Chrome on Mac.

Drarok commented 6 years ago

Also weird is this form of event handler doesn't work within the portal in a new window:

class MyComponent extends React.Component {
  constructor(props) {
    super(props);
    this.onButtonClicked = this.onButtonClicked.bind(this);
  }

  render() {
    <button onClick={this.onButtonClicked}>Click me</button>;
  }
}

but this does:

class MyComponent extends React.Component {
  constructor(props) {
    super(props);
  }

  render() {
    <button onClick={e => this.onButtonClicked()}>Click me</button>;
  }
}
EthanDRaymond commented 6 years ago

Also appears in Chrome and Firefox on Windows.

jiayihu commented 6 years ago

This happens because if you add the DOM node to the new window in componentDidMount, React won't know it until the next render triggered with setState or forceUpdate. So you can solve by moving the window reference in the state using this.setState({ extWindow })

gaearon commented 6 years ago

The issue is that you're first creating a DOM node in one document, but then moving it to another one. However React has already bound the event handlers to the first document. React doesn't expect that you would move a DOM node between documents while something is rendered into it. I wouldn't say it's a bug — React just can't detect this.

To fix it, you can change your code so that you move the node before rendering something into in React. Like here: https://codepen.io/gaearon/pen/mjGvRV?editors=0010

class Window extends React.Component {
  constructor(props) {
    super(props);
    this.state = { win: null, el: null };
  }

  componentDidMount() {
    let win = window.open('', '', 'width=600,height=400');
    win.document.title = 'A React portal window';
    let el = document.createElement('div');
    win.document.body.appendChild(el);
    this.setState({ win, el });
  }

  componentWillUnmount() {
    this.state.win.close();
  }

  render() {
    const { el } = this.state;
    if (!el) {
      return null;
    }
    return ReactDOM.createPortal(this.props.children, el);
  }
}
nickgcattaneo commented 6 years ago

@gaearon I wish there was a solution to share/move a mounted node between documents and persist/rebind bindings. For example, when creating a popout in a new window...

WindowA boots up, renders content in a container => That container contains a button to open a popout, which when invoked creates a new window (WindowB) and transfers the content node of the container to the new window. The main benefit of this approach is dynamic data/etc that may be invoked on the initial rendering of the content (or transient state edited by the user/etc) is easily preserved as the component does not mount/unmount when moved. This scenario can of course be avoided though with your example above (assuming you do not mind having your component remount when it is moved from location to location).

// in WindowA (root window)
class SomePage extends React.Component {
    constructor(props) {
        super(props)
        this.contentRef = document.createElement('div')
    }

    render() {
        /* elsewhere we have some logic to toggle/invoke the changing of whether the component should or should not be popped out */
        const { isPoppedOut } = this.props
        return (
            <>
                {/* Body appends the contentRef as a child */}
                {!isPoppedOut && <Body contentRef={contentRef} />}
                {/* PopoutWindow creates a new window, appends the contentRef to the new window */}
                {isPoppedOut && <PopoutWindow contentRef={contentRef} />}
                {ReactDOM.createPortal(
                    this.props.children,
                    this.contentRef
                )}
            </>
        )
    }
}
nickgcattaneo commented 6 years ago

~I haven't explored this yet, but I may try seeing if hydrate can handle reattaching events/etc for me: https://reactjs.org/docs/react-dom.html#hydrate (even though it doesn't match this exact use case...).~ Looks like this approach wouldn't work given the nature of ReactDOMServer.

kartik2014 commented 5 years ago

The issue is that you're first creating a DOM node in one document, but then moving it to another one. However React has already bound the event handlers to the first document. React doesn't expect that you would move a DOM node between documents while something is rendered into it. I wouldn't say it's a bug — React just can't detect this.

To fix it, you can change your code so that you move the node before rendering something into in React. Like here: https://codepen.io/gaearon/pen/mjGvRV?editors=0010

class Window extends React.Component {
  constructor(props) {
    super(props);
    this.state = { win: null, el: null };
  }

  componentDidMount() {
    let win = window.open('', '', 'width=600,height=400');
    win.document.title = 'A React portal window';
    let el = document.createElement('div');
    win.document.body.appendChild(el);
    this.setState({ win, el });
  }

  componentWillUnmount() {
    this.state.win.close();
  }

  render() {
    const { el } = this.state;
    if (!el) {
      return null;
    }
    return ReactDOM.createPortal(this.props.children, el);
  }
}

Hi @gaearon

I have implemented something similar, but when I click the browser close button in the new window , "componentWillUnmount" is not being invoked which is why state is not changing even when I close the portal using the browser close button. Is there any solution to this problem?

Hari-aN commented 5 years ago

Also weird is this form of event handler doesn't work within the portal in a new window:

class MyComponent extends React.Component {
  constructor(props) {
    super(props);
    this.onButtonClicked = this.onButtonClicked.bind(this);
  }

  render() {
    <button onClick={this.onButtonClicked}>Click me</button>;
  }
}

but this does:

class MyComponent extends React.Component {
  constructor(props) {
    super(props);
  }

  render() {
    <button onClick={e => this.onButtonClicked()}>Click me</button>;
  }
}

thats because of ES6 I think.

Jip-Hop commented 4 years ago

@gaearon I've used your codepen as a base for my test with "mousedown", "mousemove" and "mouseup" events. But the "mousemove" events are still being triggered by moving over the parent window, and not when moving over the pop out window.

To reproduce: open the example, click on "Open a Portal", arrange both windows side by side, so you can see both and move the mouse between them. Click in the red div in the portal window, then click inside the white background of the parent window and move the mouse. The counter in the red div will update when moving inside the parent window, even though it was supposed to update only when 'dragging' the red div.

How can I ensure that window in the MouseMoveTest component will reference the portal window, and not the parent window? MouseMoveTest is just an example, I'm trying to get react-color to work inside a portal window so I'm looking for a solution without modifying the component to mount inside the portal window.

sunilsp77 commented 4 years ago
class Window extends React.Component {
  constructor(props) {
    super(props);
    this.state = { win: null, el: null };
  }

  componentDidMount() {
    let win = window.open('', '', 'width=600,height=400');
    win.document.title = 'A React portal window';
    let el = document.createElement('div');
    win.document.body.appendChild(el);
    this.setState({ win, el });
  }

  componentWillUnmount() {
    this.state.win.close();
  }

  render() {
    const { el } = this.state;
    if (!el) {
      return null;
    }
    return ReactDOM.createPortal(this.props.children, el);
  }
}

This solution has some problem when we need to copy styles from parent window to new window in ComponentDidMount. The UI gets distorted when new window is opened after adding this solution.

`componentDidMount() { let win = window.open('', '', 'width=600,height=400'); copyStyles(document, win.document); win.document.title = 'A React portal window'; let el = document.createElement('div'); win.document.body.appendChild(el); this.setState({ win, el }); }

function copyStyles (sourceDoc, targetDoc) { Array.from(sourceDoc.querySelectorAll('link[rel="stylesheet"], style')) .forEach(link => { targetDoc.head.appendChild(link.cloneNode(true)); }) } `

nhanpdnguyen commented 4 years ago
class Window extends React.Component {
  constructor(props) {
    super(props);
    this.state = { win: null, el: null };
  }

  componentDidMount() {
    let win = window.open('', '', 'width=600,height=400');
    win.document.title = 'A React portal window';
    let el = document.createElement('div');
    win.document.body.appendChild(el);
    this.setState({ win, el });
  }

  componentWillUnmount() {
    this.state.win.close();
  }

  render() {
    const { el } = this.state;
    if (!el) {
      return null;
    }
    return ReactDOM.createPortal(this.props.children, el);
  }
}

This solution has some problem when we need to copy styles from parent window to new window in ComponentDidMount. The UI gets distorted when new window is opened after adding this solution.

`componentDidMount() { let win = window.open('', '', 'width=600,height=400'); copyStyles(document, win.document); win.document.title = 'A React portal window'; let el = document.createElement('div'); win.document.body.appendChild(el); this.setState({ win, el }); }

function copyStyles (sourceDoc, targetDoc) { Array.from(sourceDoc.querySelectorAll('link[rel="stylesheet"], style')) .forEach(link => { targetDoc.head.appendChild(link.cloneNode(true)); }) } `

Hi @sunilsp77, this is how I managed to copy the styles correctly, I feel like defering the styles-copy process to the next state update.

const WindowPortal = ({ children }) => {
  const [externalWindow, setExternalWindow] = useState(null);
  const [containerRef, setContainerRef] = useState(null);

  useEffect(() => {
    const containerElement = document.createElement('div');
    const externalWindow = window.open(
      '',
      '',
      'width=600,height=400,left=200,top=200'
    );
    // append the element to the external document before setting ref
    // so that React could detect event bindding correctly
    // https://github.com/facebook/react/issues/12355
    externalWindow.document.body.appendChild(containerElement);
    setContainerRef(containerElement);
    setExternalWindow(externalWindow);

    return () => {
      externalWindow?.close();
    };
  }, []);

  useEffect(() => {
    if (externalWindow) {
      copyStyles(document, externalWindow.document);
    }
  }, [externalWindow]);

  return containerRef && ReactDOM.createPortal(children, containerRef);
};