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

Re-implement onWheel to fix inverted scrolling #1241

Closed
wants to merge 4 commits into from
Closed

Re-implement onWheel to fix inverted scrolling #1241

wants to merge 4 commits into from

Conversation

danalloway
Copy link
Contributor

@danalloway danalloway commented Feb 5, 2019

Beginning with this issue #995 and then this pull request #1137 with it's review, this is my attempt at a fix for inverted scrolling direction.

The original pull request #1137 did a few things differently and did not work properly on Firefox.

This PR:

  • Addresses an issue with Firefox using deltaMode of 1 which gives you the number of lines, not pixels, in the deltaY value of the onWheel event. This results in the inverted scroll only scrolling a few pixels at a time.
  • Uses the onWheel prop that is passed to the underlying ScrollView to listen to the wheel event instead of attaching an additional one. This eliminates the need to attach and detach listeners.
  • Dispatches the onWheel event to any child lists as well as forwards the onWheel prop on just like how it does in _onScroll (do we want to do this, is this right?)

How I'm figuring out the number of pixels in a line is interesting, it only applies to events that have their deltaMode set to 1 though. I question if we even need this? is just setting it do the default enough for a proper scroll experience?

@danalloway
Copy link
Contributor Author

testing this a bit more and it appears to have issues on Windows and Android, will update if I find a solution

@danalloway
Copy link
Contributor Author

there are some cases where the corrected inverted scrolling jumps backwards as you scroll (can easily reproduce in the Brave browser)
wrapping the scrollTop assignment in a setTimeout(() => {}, 0) resolves the issue
can anyone shed some light on why that would happen?


scrollNode[direction] += delta

e.preventDefault();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need this preventDefault here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it looks like we do, as it breaks the scrolling in Chrome without it

@danalloway
Copy link
Contributor Author

ok, everything works properly on Windows across Brave, Firefox, Edge and Chrome now.
just need to confirm that scrolling in Chrome on Android works properly

Brave on Windows has a jump scroll, delaying the scroll adjustment my a tick fixes the issue without introducing anything noticeable across other browsers / platforms
prefer delta over wheelDelta
the values they report are not the same (my mistake) so correct my previous commit
@danalloway
Copy link
Contributor Author

broken again on latest version of chrome
fixing the scrolling on the scaleY -1 hack with another hack just doesn't smell right

going to close this PR for now, this isn't the way to solve it

@danalloway danalloway closed this Feb 28, 2019
@necolas
Copy link
Owner

necolas commented Feb 28, 2019

Thanks for looking into this. I'll give this some thought as it might be something we can solve at a lower level in the future

@danalloway
Copy link
Contributor Author

danalloway commented Feb 28, 2019

you're welcome, and when that time comes let me know how I can help, even if it's just testing or vetting an idea

@annie-elequin
Copy link

Hey y'all, attempting to build a chat web app and running into this same problem with the inverted FlatList. is there a recommended way to workaround the scrollwheel for now, or was it not really solved?

necolas pushed a commit that referenced this pull request Feb 18, 2022
rnike pushed a commit to VeryBuy/react-native-web that referenced this pull request Sep 13, 2022
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 this pull request may close these issues.

None yet

3 participants