facebook / react

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

Bug: window as new portal will break event delegation #18616

Open hanq08 opened 4 years ago

hanq08 commented 4 years ago

React version: any?

Steps To Reproduce

  1. Button attach to a window portal with window.open
  2. Event not triggering

Link to code example:

const { useState, useEffect } = React;

function WindowPortal({ children }) {
  const [container, setContainer] = useState(document.createElement('div'));
  useEffect(() => { 
    const newWindow = window.open('', '', 'width=600,height=400,left=200,top=200');
    newWindow.document.body.appendChild(container);
  });
  return ReactDOM.createPortal(children, container);
}
function App() {
  const [value, setValue] = useState('unclicked');
  const handleClick = () => setValue('clicked'); 
  return (
    <div>
      <div>Portal Test</div>
      <WindowPortal>
        <button onClick={handleClick}>{value}</button>
      </WindowPortal>
    </div>
  );
}

ReactDOM.render(
  React.createElement(App),
  document.getElementById('root')
);

Any event in the new window will not be triggered since all events are bind to the original window. I think react can support a new mode for using native event binding rather than event delegation if it makes sense. Preact actually uses native browser event and don't use react event delegation system.

The current behavior

Event not trigger for components in new window

The expected behavior

Event will trigger

tombrowndev commented 4 years ago

@hanq08

You could achieve it by using an effect and attaching an event listener to the button:

function WindowPortal({ children }) {
  const [container, setContainer] = useState(document.createElement("div"));

  useEffect(() => {
    const newWindow = window.open(
      "",
      "",
      "width=600,height=400,left=200,top=200"
    );
    newWindow.document.body.appendChild(container);
  }, [container]);

  return ReactDOM.createPortal(children, container);
}

export default function App() {
  const [value, setValue] = useState("unclicked");
  const buttonRef = useRef(null);
  const handleClick = () => setValue("clicked");

  useEffect(() => {
    if (buttonRef.current) {
      buttonRef.current.addEventListener("click", handleClick);

      return () => buttonRef.current.removeEventListener("click", handleClick);
    }
  });

  return (
    <div>
      <div>Portal Test</div>
      <WindowPortal>
        <button ref={buttonRef}>{value}</button>
      </WindowPortal>
    </div>
  );
}

Looks like something to be made into a nice custom hook?

tombrowndev commented 4 years ago

Maybe for official support we could introduce new event properties such as nativeOnClick?

hanq08 commented 4 years ago

@tombrowndev yep that should work. But I don't know if there are any potential issues by mixing event delegation system and events binding to ref arbitrarily. I feel like the whole app should either use native event binding or event delegation rather than having some event delegated to the root and some bind to the host dom.

rachelnabors commented 4 years ago

We've had a couple of discussions around event propagation and portals in the past (see also https://github.com/facebook/react/issues/11387). Seems like this is a part of that discussion