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

Trap click events for portal root #11927

Merged
merged 3 commits into from Aug 18, 2018

Conversation

@aweary
Copy link
Contributor

@aweary aweary commented Dec 27, 2017

Should resolve #11918

Still need to run the build in iOS Safari to verify, but I'm 90% sure it will since an empty click handler fixed it in the JSFiddle provided for the issue.

@@ -1125,6 +1126,7 @@ function createPortal(
isValidContainer(container),
'Target container is not a DOM element.',
);
trapClickOnNonInteractiveElement(((container: any): HTMLElement));
Copy link
Member

@gaearon gaearon Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure we want to introduce side effects there.

Loading

Copy link
Contributor Author

@aweary aweary Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another place to perform DOM-specific effects when a portal is mounted?

Loading

Copy link
Member

@gaearon gaearon Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like appendToContainer in the renderer?

Loading

Copy link
Contributor Author

@aweary aweary Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do it in appendChildToContainer then we'll also be trapping click events on other containers, I guess that's fine?

Loading

Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe? Idk :-(

Loading

Copy link
Member

@gaearon gaearon left a comment

createPortal shouldn't have side effects

Loading

@@ -760,7 +761,11 @@ const DOMRenderer = ReactFiberReconciler({
): void {
if (container.nodeType === COMMENT_NODE) {
(container.parentNode: any).insertBefore(child, container);
trapClickOnNonInteractiveElement(
((container.parentNode: any): HTMLElement),
);
Copy link
Contributor Author

@aweary aweary Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon is using a comment node as a mount point supported for portals as well?

Loading

Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth supporting for consistency I guess

Loading

@aweary aweary dismissed gaearon’s stale review Jan 5, 2018

addressed

@aweary aweary force-pushed the empty-click-portal-container branch from 879cc35 to 7e8a4dc Jan 5, 2018
} else {
trapClickOnNonInteractiveElement(((container: any): HTMLElement));
Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it already has onclick? Is it expected that rendering into it overrides onclick?

Loading

Copy link
Contributor Author

@aweary aweary Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if it already has onclick we don't need to trap click events, so I'll wrap this in a null check.

Loading

Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do that in trapClickOnNonInteractiveElement itself then?

Loading

Copy link
Contributor Author

@aweary aweary Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I was doing :) my only hesitation is that trapClickOnNonInteractiveElement is also called on elements that React controls (which shouldn't have onclick making the check redundant in most cases).

Loading

Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess you're right

Loading

Copy link
Member

@gaearon gaearon Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way do we do this regardless of whether we're mobile safari? Is that good? Should we check the browser somehow?

Loading

Copy link
Contributor Author

@aweary aweary Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we do it regardless of the browser. There's an existing TODO for adding browser detection. It might be a good time to address that, assuming browser detection is fast/reliable enough.

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jul 15, 2018

Hey Brandon! Are there any next steps I can help figure out to move this along? The TODO you referenced for only applying this behavior in Safari also has an issue tracking it (#12989). Should we focus on that first?

Loading

@gaearon gaearon force-pushed the empty-click-portal-container branch from 7e8a4dc to 11687a3 Aug 17, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2018

I don't understand this fix.

In the existing code we set onclick handlers on the target nodes. That is, on the nodes that would be clicked.

Here, we set onclick on the container node of the portal. Why is this helpful?

Loading

@aweary
Copy link
Contributor Author

@aweary aweary commented Aug 17, 2018

It's been awhile, but see this JSFiddle from the issue @gaearon. Attaching an event listener to the container allows the event to bubble 🤷‍♂️

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2018

We need to have a rational explanation first. 😄

Loading

@aweary
Copy link
Contributor Author

@aweary aweary commented Aug 17, 2018

Is "because iOS Safari" rational enough? 😅

I'll have to dig into this again to make any sense of it.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2018

Is "because iOS Safari" rational enough

Scare tactics.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2018

It does work though. WHY

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2018

Loading

@gaearon gaearon merged commit b3a4cfe into facebook:master Aug 18, 2018
1 check was pending
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Aug 18, 2018

Seems fine. I added a comment for why this works.

Loading

@pull-bot
Copy link

@pull-bot pull-bot commented Aug 18, 2018

ReactDOM: size: 0.0%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: 0da5102...4893ae2

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 649.27 KB 649.95 KB 152.41 KB 152.57 KB UMD_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 95.6 KB 95.64 KB 31.24 KB 31.28 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 645.41 KB 646.09 KB 151.28 KB 151.45 KB NODE_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 95.59 KB 95.64 KB 30.98 KB 31 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.1% 652.7 KB 653.38 KB 149.44 KB 149.63 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 288.52 KB 288.67 KB 53.42 KB 53.45 KB FB_WWW_PROD
react-dom.profiling.min.js 0.0% +0.1% 96.79 KB 96.83 KB 31.37 KB 31.4 KB NODE_PROFILING
ReactDOM-profiling.js +0.1% +0.1% 290.87 KB 291.01 KB 54.02 KB 54.05 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

Loading

philipp-spiess added a commit to philipp-spiess/react that referenced this issue Oct 4, 2018
Fixes facebook#13777

As part of facebook#11927 we introduced a regression by adding onclick handler
to the React root as well. This causes the whole React tree to flash
when tapped on iOS devices (for reasons I outlined in facebook#12989 (comment)).

To fix this, we should only apply onclick listeners to portal roots. I
verified that this fix indeed worked by checkout out our DOM fixtures
and added regression tests as well.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites being
bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative fix would be to add a third parameter to
`appendChildToContainer` based on the tag of the parent fiber. Although
I think relying on the `_reactRootContainer` property that we set on the
element is less intrusive.
philipp-spiess added a commit to philipp-spiess/react that referenced this issue Oct 4, 2018
Fixes facebook#13777

As part of facebook#11927 we introduced a regression by adding onclick handler
to the React root. This causes the whole React tree to flash
when tapped on iOS devices (for reasons I outlined in facebook#12989 (comment)).

To fix this, we should only apply onclick listeners to portal roots. I
verified that this fix indeed worked by checkout out our DOM fixtures
and added regression tests as well.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites being
bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative fix would be to add a third parameter to
`appendChildToContainer` based on the tag of the parent fiber. Although
I think relying on the `_reactRootContainer` property that we set on the
element is less intrusive.
philipp-spiess added a commit that referenced this issue Oct 9, 2018
Fixes #13777

As part of #11927 we introduced a regression by adding onclick handler
to the React root. This causes the whole React tree to flash when tapped
on iOS devices (for reasons I outlined in
#12989 (comment)).

To fix this, we should only apply onclick listeners to portal roots. I
verified that my proposed fix indeed works by checking out our DOM
fixtures and adding regression tests.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites (and 
thus their React root) being bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative approach to finding out if we're appending to a React
root would be to add a third parameter to `appendChildToContainer` based
on the tag of the parent fiber.
linjiajian999 pushed a commit to linjiajian999/react that referenced this issue Oct 22, 2018
Fixes facebook#13777

As part of facebook#11927 we introduced a regression by adding onclick handler
to the React root. This causes the whole React tree to flash when tapped
on iOS devices (for reasons I outlined in
facebook#12989 (comment)).

To fix this, we should only apply onclick listeners to portal roots. I
verified that my proposed fix indeed works by checking out our DOM
fixtures and adding regression tests.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites (and 
thus their React root) being bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative approach to finding out if we're appending to a React
root would be to add a third parameter to `appendChildToContainer` based
on the tag of the parent fiber.
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
Fixes facebook#13777

As part of facebook#11927 we introduced a regression by adding onclick handler
to the React root. This causes the whole React tree to flash when tapped
on iOS devices (for reasons I outlined in
facebook#12989 (comment)).

To fix this, we should only apply onclick listeners to portal roots. I
verified that my proposed fix indeed works by checking out our DOM
fixtures and adding regression tests.

Strangely, I had to make changes to the DOM fixtures to see the behavior
in the first place. This seems to be caused by our normal sites (and 
thus their React root) being bigger than the viewport:

![](http://cl.ly/3f18f8b85e91/Screen%20Recording%202018-10-05%20at%2001.32%20AM.gif)

An alternative approach to finding out if we're appending to a React
root would be to add a third parameter to `appendChildToContainer` based
on the tag of the parent fiber.
jasco pushed a commit to jasco/react-shade that referenced this issue Feb 4, 2020
This event retarget was originally added in order to work around a
problem with react event propagation across the shadow root boundary.
This appears to be no longer needed.  It is currently resulting in
double dispatching to the on click handler.

While not yet confirmed, it is possible that the underlying issue was
fixed with facebook/react#11927 which was
released in react-dom v16.5.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants