Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: (17.0.0-rc.0) Event propagation through portals is inconsistent #19608

Closed
nstepien opened this issue Aug 13, 2020 · 9 comments · Fixed by #19659
Closed

Bug: (17.0.0-rc.0) Event propagation through portals is inconsistent #19608

nstepien opened this issue Aug 13, 2020 · 9 comments · Fixed by #19659

Comments

@nstepien
Copy link

React version: 17.0.0-rc.0

Steps To Reproduce

  1. Open the codesandbox demo link below.
  2. Click on the root and portal divs, check the logs.
  3. Uncomment the portal div's onClickCapture noop handler, check the logs again.

Link to code example: https://codesandbox.io/s/determined-montalcini-vjrgc?file=/src/App.js

The current behavior

Clicking on the portal div logs "portal click" only.
Adding an onClickCapture noop handler on the portal div "fixes" the root's onClickCapture handler.
You might have to refresh the page between edits, otherwise the root's onClickCapture handler might keep working even after removing the portal's onClickCapture handler.

The expected behavior

Clicking on the portal div should trigger the root's onClickCapture handler, whether the portal div has an onClickCapture handler or not.

@nstepien nstepien added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Aug 13, 2020
@gaearon gaearon added Component: DOM Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Aug 13, 2020
@gaearon
Copy link
Collaborator

gaearon commented Aug 13, 2020

Thanks for reporting!

@koba04
Copy link
Contributor

koba04 commented Aug 14, 2020

I've investigated the issue and figured out why this happens.
React registers event handlers to a root container, with Portal, the root container is the container of Portal. React listens to events, which are only registered event handlers on components within Portal, at the container of Portal. It means that events that are not registered in Portal container don't propagate outside the Portal.
To fix this, React should listen to events, which are registered event handlers on all parent root containers, at the Portal container to propagate them.
But I'm not sure that is the right way to fix this issue.

The following is a minimum case for this issue.

container = document.createElement('div');
const onClick= jest.fn();

const ref = React.createRef();
ReactDOM.render(
  <div onClick={onClick}>
    {ReactDOM.createPortal(
      <button ref={ref}>click</button>,
      document.body
    )}
  </div>,
  container
);
const event = new MouseEvent('click', {
  bubbles: true,
});
ref.current.dispatchEvent(event);

expect(onClick).toHaveBeenCalledTimes(1); // 0

@diegohaz
Copy link

The funny thing is that this bug actually fixes #11387

And I would consider this the desired behavior 99% of the time.

@gaearon
Copy link
Collaborator

gaearon commented Aug 17, 2020

I wouldn't say it "fixes" it, it's just an inconsistent behavior. I understand the desire for a different behavior there but let's not spill it off in this thread. (My latest reply is #16721 (comment).)

gaearon added a commit to gaearon/react that referenced this issue Aug 19, 2020
gaearon pushed a commit to gaearon/react that referenced this issue Aug 21, 2020
gaearon pushed a commit to gaearon/react that referenced this issue Aug 21, 2020
gaearon pushed a commit to gaearon/react that referenced this issue Aug 21, 2020
gaearon added a commit that referenced this issue Aug 24, 2020
* Failing test for #19608

* Attach Listeners Eagerly to Roots and Portal Containers

* Forbid createEventHandle with custom events

We can't support this without adding more complexity. It's not clear that this is even desirable, as none of our existing use cases need custom events. This API primarily exists as a deprecation strategy for Flare, so I don't think it is important to expand its support beyond what Flare replacement code currently needs. We can later revisit it with a better understanding of the eager/lazy tradeoff but for now let's remove the inconsistency.

* Reduce risk by changing condition only under the flag

Co-authored-by: koba04 <koba0004@gmail.com>
@nstepien
Copy link
Author

@gaearon Thanks! Can we expect a rc.1 with the fix?

@gaearon
Copy link
Collaborator

gaearon commented Aug 24, 2020

Yes, but it will take a bit of time for us to test it internally. It's a pretty significant internal change.

@gaearon
Copy link
Collaborator

gaearon commented Aug 28, 2020

Fixed in 17.0.0-rc.1.
https://codesandbox.io/s/agitated-wiles-t27jm?file=/src/App.js

@targos
Copy link

targos commented Aug 31, 2020

Is there a way to know what commits have been added between 17.0.0-rc.0 and 17.0.0-rc.1. There seems to be no tags on GitHub for those versions.

@gaearon
Copy link
Collaborator

gaearon commented Aug 31, 2020

reactjs/react.dev#3231

koto pushed a commit to koto/react that referenced this issue Jun 15, 2021
* Failing test for facebook#19608

* Attach Listeners Eagerly to Roots and Portal Containers

* Forbid createEventHandle with custom events

We can't support this without adding more complexity. It's not clear that this is even desirable, as none of our existing use cases need custom events. This API primarily exists as a deprecation strategy for Flare, so I don't think it is important to expand its support beyond what Flare replacement code currently needs. We can later revisit it with a better understanding of the eager/lazy tradeoff but for now let's remove the inconsistency.

* Reduce risk by changing condition only under the flag

Co-authored-by: koba04 <koba0004@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants