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

Strange onScroll behaviour #15723

Closed
O4epegb opened this issue May 23, 2019 · 23 comments
Closed

Strange onScroll behaviour #15723

O4epegb opened this issue May 23, 2019 · 23 comments

Comments

@O4epegb
Copy link

O4epegb commented May 23, 2019

Do you want to request a feature or report a bug?

Possible bug

What is the current behavior?

onScroll callback on parent element fires when children element is scrolled.
Native listener is working as expected, though.

Example with reproduction
https://codesandbox.io/s/kk3th

Just try to scroll little box with items.

What is the expected behavior?

I am not sure if this behaviour is correct, but it was unexpected for me, so it might be a bug.
I was not expecting onScroll to fire at all.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

"react": "16.8.6"
macOs Mojave 10.14.5

Did not tried any other versions

@kunukn
Copy link
Contributor

kunukn commented May 24, 2019

The syntax for adding event listener is this

something.addEventListener(event, function, useCapture)

The native event don't fire for the code you provide

this.wrapperNode.current.addEventListener('scroll', () =>
      console.log('native scroll')
    );

But it will for this

this.wrapperNode.current.addEventListener('scroll', () =>
      console.log('native scroll'), true
    );

I suspect behind the scenes, the React onScroll event does the addEventListener with useCapture set to true

@O4epegb
Copy link
Author

O4epegb commented May 24, 2019

Sorry, you probably misunderstood me!

The native event don't fire for the code you provide

This is actually intended, this is correct behaviour I am expecting. It should not fire because there is no capturing.

React onScroll event does the addEventListener with useCapture set to true

So, if this is true then it is very unexpected! Why would it do this?

@kunukn
Copy link
Contributor

kunukn commented May 24, 2019

Regarding my comment that I suspected the useCapture was set to true by React, I was speculating.

The React scroll event was not added to the wrapper or child element.

From the docs, it says we can read the nativeEvent https://reactjs.org/docs/events.html
I applied console.log(e.nativeEvent.currentTarget); I got #document

When I inspect what event listeners there are attached to the document from Chrome devtools in the console getEventListeners(document)

I got this

{scroll: Array(1)}
scroll: Array(1)
0: {listener: ƒ, useCapture: true, passive: false, once: false, type: "scroll"}
length: 1

Maybe somebody else could give some more insight of how the scroll events works in a React?
I probably need to look more into the source code before I can provide more accurate answers.

EDIT:
I investigated and the following code gets executed when adding an onScroll property on any component.

https://github.com/facebook/react/blob/master/packages/react-dom/src/events/EventListener.js#L18

with arguments
element: #document eventType: scroll

@O4epegb
Copy link
Author

O4epegb commented May 25, 2019

So you were right it actually use capturing!

But for me it seems like a bug, or at least very unintuitive behavior. Isn't it?

@kunukn
Copy link
Contributor

kunukn commented May 27, 2019

React is also about abstracting the DOM operations away from the developer.

React will take care of the DOM operations in the fastest, safest, best performance, cross browser friendly, support most devices, etc.

Disclaimer: I am speculating.

I believe in this case, there is a strategy to use one scroll event-listener to rule them all. This strategy could have been applied to make the DOM as fast as possible or to mitigate some other DOM concerns that could be relevant.

@O4epegb
Copy link
Author

O4epegb commented May 27, 2019

It is fine to use one event, I guess!

But I don't understand how this event is useful when you need to make some checks by yourself, and that check can only be made by creating ref (for example you could compare target element with your container element). So if you creating ref anyway why would you use onScroll over native event? Why this check is not internal to React?

I also have suspicion that it was't always like that, because I found out about this after I saw some strange behavior in production code which was't changed for several months! Going to check earlier versions of React later

@O4epegb
Copy link
Author

O4epegb commented Jun 6, 2019

So can anybody confirm is this a bug or correct behaviour?

@chocolatl
Copy link

I also encountered this problem, it makes me confused. Why parent elements can capture scroll event of children?

@O4epegb
Copy link
Author

O4epegb commented Aug 1, 2019

Exactly!

@chocolatl
Copy link

For native event, scroll event don't bubble unless target element is document, it's different from other events.
I think React provide uniform behavior for each event, but it's not explained in the documentation.

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@O4epegb
Copy link
Author

O4epegb commented Jan 11, 2020

This behavior is still reproducible with current react 16.12.0

I am still not sure if this is expected or not

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 11, 2020
@stale
Copy link

stale bot commented Apr 11, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2020
@O4epegb
Copy link
Author

O4epegb commented Apr 11, 2020

Bump, I guess. Need to check it with recent versions though

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2020
@O4epegb
Copy link
Author

O4epegb commented Jul 7, 2020

Still happens with React 16.13.1

@gaearon
Copy link
Collaborator

gaearon commented Jul 7, 2020

Maybe @sophiebits remembers some context around this? I think early on the decision was to consistently bubble all React events — even the ones that the browser doesn't bubble by default.

@sophiebits
Copy link
Collaborator

Correct. This is “working as intended”, similar to onSubmit (but I wouldn’t make the same decision if I was designing React DOM today).

As a workaround, you can check e.target === e.currentTarget if you want to know if the event originated from the element that the listener is attached to.

@O4epegb
Copy link
Author

O4epegb commented Jul 7, 2020

Hm, ok, thank you for the answer!

Although it is very weird thing, basically it means that in every onScroll handler you need to make this check? Because otherwise some new scrollable component could be introduced down the tree and then you handler will be "bugged"? I mean it will also be invoked even when no scroll actually happened. Do I understand it correctly? No offense or anything, just want to understand what is the best way to handle this situation :)

@gaearon
Copy link
Collaborator

gaearon commented Jul 8, 2020

Yeah. It's a reasonable concern and the same reason bubbling media events in React are weird. It's not an easy change to make in terms of breakages so I don't know if we would ever do it, but feel free to file a proposal here: https://github.com/reactjs/rfcs.

@gaearon gaearon closed this as completed Jul 8, 2020
@O4epegb
Copy link
Author

O4epegb commented Jul 8, 2020

Ok, got it! Thank you for help.

@gaearon
Copy link
Collaborator

gaearon commented Aug 7, 2020

We're fixing this in 17.

@O4epegb
Copy link
Author

O4epegb commented Aug 9, 2020

Wow, that's great to hear, thank you!

@webdo6263
Copy link

webdo6263 commented Jan 27, 2023

FYI I still see this but I am on 16.*, so I assume once I move to 17 it will stop working.
I agree the behavior is wierd that onscroll should get fired when a child/descendent is scrolled. However, instead of completely removing it, is there a option to have onScroll fired only for direct scrolling but another event for example
onScrollWithin fired when any of its child/descendent is scrolled?
May be this behavior could be replicated for other events as well similar to onFocus and onFocusWithin etc

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

No branches or pull requests

7 participants