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

Align root container onclick behavior with legacy mode #17330

Closed
wants to merge 1 commit into from

Conversation

gaearon
Copy link
Member

@gaearon gaearon commented Nov 10, 2019

So... this isn't really important but we're inconsistent with how we set onclick on container nodes.

In Legacy Mode, we only set them on Portal Containers (but not Root Containers).
In modern Modes, we set them both on Portal Containers and Root Containers.

There's no particular good reason to set them on Root Containers (Portal Containers need it for onclick event to bubble on Safari). So I'm changing them to behave the same way as in Legacy Mode.

I guess it would also be ok to always set it. Another way to think of it is this is removing some usage of _reactRootContainer where the code implies "root" because it's really only "legacy root" and this isn't obvious. For example someone may change this logic later and assume all roots have _reactRootContainer.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 10, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8bd2b88:

Sandbox Source
competent-fire-rtbqb Configuration

@sizebot
Copy link

sizebot commented Nov 10, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 8bd2b88

@sizebot
Copy link

sizebot commented Nov 10, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 8bd2b88

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 10, 2019

Can you explain why we need it for only Portals?

@gaearon
Copy link
Member Author

gaearon commented Nov 10, 2019

If something has a React click listener we put onclick on that node directly. That’s sufficient to catch events from it. Or originating from its children.

But portals break out of DOM hierarchy. So if you click on a modal’s div, there might not be anything in the DOM tree above with an onclick handler. So safari will ignore it. However, the logical React hierarchy might have a click handler above the portal. We don’t want to miss it.

We don’t need these on roots because if there was no click handler by the time we reached the root, we know there’s nothing that needs to handle it in the React tree.

But we need them in portal containers because there might still be React listeners in the logical tree even if the physical tree has no onclick above.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 18, 2019

So in concurrent mode there's two more places we rely on bubbling for delegation:

  1. Selective hydration. Emitting onclick="" attributes in the SSR should cover this use case sufficiently.
  2. To force flush discrete work when the event hasn't committed yet. I believe this extra listener does help in that case.

Imagine that there isn't an onclick listener and then one is added in another discrete state update: c ? <div /> : <div onClick={...} />

If this change hasn't flushed yet, by the time the second click comes in, then Safari doesn't know that something might need the click and doesn't dispatch any event to us. If we always attach a click then Safari does dispatch the event and we can flush the previous work which attaches the event listeners in time to be invoked.

Ofc, this doesn't happen in legacy mode since work isn't batched across events so it's unnecessary there.

@andi1984
Copy link

andi1984 commented Nov 30, 2019

But we need them in portal containers because there might still be React listeners in the logical tree even if the physical tree has no onclick above.

I'm not knowing the internals of React but for me, this seems to be the main situation where React maintains something which differs completely with the actual DOM. From a component wrapping level, it makes total sense to me within the React ecosystem, but are you sure that you want to maintain all those edge cases the users of React might run into because of that? Or is it actually no big deal?

I would personally have no problem if it would be clear that every DOM-based interaction between Portal (and its children) and outer components is not supported, but I agree it's nice to have this kind of interaction.

@necolas necolas added the React Core Team label Jan 8, 2020
@gaearon
Copy link
Member Author

gaearon commented Jan 23, 2020

meh.

@gaearon gaearon closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants