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

Events propagate to nested components after stopPropagation() #1691

Closed
lrowe opened this issue Jun 14, 2014 · 17 comments
Closed

Events propagate to nested components after stopPropagation() #1691

lrowe opened this issue Jun 14, 2014 · 17 comments

Comments

@lrowe
Copy link
Contributor

lrowe commented Jun 14, 2014

We're using react-bootstrap modals in a site with full page rendering, so we have nested React rendering. As of d853c85 events are propagated to ancestors, but calls to event.stopPropagation() on the inner component hierarchy are not respected by the outer component hierarchy.

Complicating matters, the SyntheticEvent is created afresh for dispatch to each component hierarchy and there is no stoppedPropagation equivalent to nativeEvent.defaultPrevented to be called in the SyntheticEvent constructor. So even if forEachEventDispatch (EventPluginUtils.js) checked event.isPropagationStopped() in the single callback case, it would still return false.

Tracking the propagationStopped status through a return value of ReactEventEmitter.handleTopLevel looks tricky too given EventPluginHub.extractEvents may map a single nativeEvent to multiple synthetic events.

The easiest fix may just be to set a custom property on the nativeEvent object in SyntheticEvent.stopPropagation to check in the constructor, then check event.isPropagationStopped() in the single callback case of forEachEventDispatch.

@sompylasar
Copy link
Contributor

@lrowe Prop_o_gation should be Prop_a_gation

@lrowe lrowe changed the title Events propagate to nested components after stopPropogation() Events propagate to nested components after stopPropagation() Jun 14, 2014
@lrowe
Copy link
Contributor Author

lrowe commented Jun 14, 2014

Thanks @sompylasar, spelling fixed.

lrowe added a commit to lrowe/react that referenced this issue Jun 16, 2014
When nesting top-level components (e.g., calling React.renderComponent within
componentDidMount), events bubble to the parent component.

This change ensures that when `event.stopPropagation()` is called in an event
handler of the inner top-level component, event handlers of outer top-level
components are not called. Closes facebook#1691.

As of the current W3C DOM4 draft, there is no standardised way to determine if
`stopPropagation()` has been called on an event. This change sets the
deprecated `cancelBubble` property existing in most current browsers and
should continue to work if the property is removed in the future.
@mooncakelmn
Copy link

Hi there, it's been around 1.5 years passed, any plan on this issue? There are several cases in our team that we need to prevent the event bubbling of inner components. The workaround for us is painful: to drill down the target element and adding addEventListener to stop propagation there, then we have to care about the clean up, remove the listener when unmount.

@sophiebits
Copy link
Collaborator

Not really, unfortunately. We don't have a good model for how events for nested trees should interact. Synthetic events and native events are two different concepts – for example, a native keyup event could result in a synthetic onChange event – but if you call preventDefault on the onChange event, should that prevent the keyup? We dispatch the native event to each render tree and form the synthetic events separately in each tree right now, which we'd probably need to change to fix this properly. I'm afraid that any short-term solution here would be a patch that solves only a few cases and makes the code harder to reason about; we probably need a holistic rethinking of this problem to make a good solution here, and it's not something that we encounter very much because we're much more focused on React in a single render tree.

@jugimaster
Copy link

jugimaster commented Jul 22, 2016

For the record, I ran into this problem without doing anything exotic.

I've got a CRUD-style entity list table where each row has an onClick handler to "choose" the entity the row represents. But in one of the row's cells, there are control buttons with their own onClick handlers.

Now when I click on one of the buttons under the table row, the table row's onClick gets triggered before the button's!

I tried circumventing the problem by setting up a native browser event for the button component, but now any click anywhere seems to trigger just about everything on the page, even though I'm calling all the stop propagation methods I could find.

How can this still be a problem after two years? Why does React hijack event handling to begin with?

@vhmth
Copy link

vhmth commented Dec 13, 2016

Getting the same problem. I'm in a somewhat exotic environment though (overloading Gmail via a Chrome Extension), but the event propagation should stop all the same. I'm sure there was a good reason for hijacking the event system and creating SyntheticEvents, but this kinda takes away from something I really like about React (the fact that it always has a "top-hatch" to let you plug-n-play it in any environment).

@ehartford
Copy link

Hi, I have a parent div and a child div, both handle onClick. If the mouse is on the child, I am calling event.stopPropogation but it is not working... This works fine in regular html. but not in react.

@LogicMonsters
Copy link

LogicMonsters commented Feb 11, 2017

I have same problem. And I used a third-party component, Which means binding vanila event and then stop propagation is not feasible.

@farynaio
Copy link

I have the same problem.

@winnchen
Copy link

winnchen commented Mar 7, 2017

Sad

@MartinHaeusler
Copy link

... same problem here... this is really annoying.

@cbengtson85
Copy link

Is there a plan to fix this issue in the next version of React?

@randohinn
Copy link

I stumbled accross this aswell. really annoying. @spicyj, any news?

@okonet
Copy link
Contributor

okonet commented Jun 21, 2017

It seems react-dropzone is also suffering from this one.

@gaearon
Copy link
Collaborator

gaearon commented Oct 1, 2017

Portals introduced in React 16 let you unite several DOM trees into a single conceptual React tree. If I understand correctly, this should help address the original problem described in the post, as event bubbling works respects the React hierarchy in portals.

As @sophiebits noted in #1691 (comment), it is not clear what a generic fix would be like. Comments like “+1” and “Sad” don’t bring us closer to solving that problem.

I am closing this issue as I believe a reasonable workaround (portals) exists in React 16. Libraries providing modals should migrate to using portals.

If you are experiencing this problem but have a different use case, please file a new issue and describe it in more detail, preferably with an example. Unfortunately most other comments in this thread are too brief or lacking details.

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

We're fixing a subset of these problems in React 17 — by moving native React event listeners from document to the root. Portals are still recommended if you use one copy of React though.

@gaearon
Copy link
Collaborator

gaearon commented Aug 11, 2020

If you want to try it:
https://reactjs.org/blog/2020/08/10/react-v17-rc.html

josephsavona pushed a commit that referenced this issue May 15, 2024
…ferenced

identifiers 

--- 

A few fixes for finding context identifiers: 

Previously, we counted every babel identifier as a reference. This is 
problematic because babel counts every string symbol as an identifier. 

```js 

print(x);  // x is an identifier as expected 

obj.x      // x is.. also an identifier here 

{x: 2}     // x is also an identifier here 

``` 

This PR adds a check for `isReferencedIdentifier`. Note that only non-lval 
references pass this check 

```js 

print(x);  // isReferencedIdentifier(x) -> true 

obj.x      // isReferencedIdentifier(x) -> false 

{x: 2}     // isReferencedIdentifier(x) -> false 

x = 2      // isReferencedIdentifier(x) -> false 

``` 

Which brings us to change #2. 

Previously, we counted assignments as references due to the identifier visiting 
+ checking logic. The logic was roughly the following (from #1691) 

```js 

contextVars = intersection(reassigned, referencedByInnerFn); 

``` 

Now that assignments (lvals) and references (rvals) are tracked separately, the 
equivalent logic is this. Note that assignment to a context variable does not 
need to be modeled as a read (`console.log(x = 5)` always will evaluates and 
prints 5, regardless of the previous value of x). 

``` 

contextVars = union(reassignedByInnerFn, intersection(reassigned, 
referencedByInnerFn)) 

``` 

--- 

Note that variables that are never read do not need to be modeled as context 
variables, but this is unlikely to be a common pattern. 

```js 

function fn() { 

let x = 2; 

const inner = () => { 

x = 3; 

} 

} 

```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.