Skip to content

Fix scrolling issues - #4484

Closed
vadimka123 wants to merge 2 commits into
JedWatson:masterfrom
vadimka123:fix-scrolling-issue
Closed

vadimka123 wants to merge 2 commits into
JedWatson:masterfrom
vadimka123:fix-scrolling-issue

Conversation

@vadimka123

@vadimka123 vadimka123 commented Mar 10, 2021 •

Copy link
Copy Markdown

@changeset-bot

changeset-bot Bot commented Mar 10, 2021 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 4e10d6c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci

codesandbox-ci Bot commented Mar 10, 2021 •

Copy link
Copy Markdown

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 4e10d6c:

Sandbox Source
react-codesandboxer-example Configuration

@ebonow

ebonow commented Mar 10, 2021

Copy link
Copy Markdown
Collaborator

Greetings @vadimka123 ,

Do you know or can you test if this makes the the touch Handlers obsolete? My understanding is that the mouseDown and mouseUp are fired after the touch events so perhaps we can replace the touch handlers with these instead of simply adding two more event listeners that do the same thing.

https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Supporting_both_TouchEvent_and_MouseEvent

@ebonow ebonow added this to the 4.3 milestone Mar 10, 2021
@vadimka123

Copy link
Copy Markdown
Author

Hi, @ebonow

I tested only in desktop this behaviour
It's separate events: only touch events fired when I make scroll use mouse wheel, only this mouse events fired when I make mouse drag

@ebonow

ebonow commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

No worries, I think I answered my own question anyway as I confused touchmove with touchend. I'll do a quick smoke test and verify that it works and resolves both of the issues that are mentioned.

@ebonow

ebonow commented Mar 13, 2021

Copy link
Copy Markdown
Collaborator

@vadimka123 I had an opportunity to test this. My concern is that this seems like a partial solution. It is only called when the mouse button is let go so if the user drags to the bottom, doesn't see what they want, drags back up, and then lets go of the mouse button, the onScrollToBottom handler is never called.

It's also unclear to me how this resolves #2862 as this doesn't appear to address the scrollTarget ref being null when first called.

I appreciate the contribution and simply want to make sure we are adding code that resolves the core issues instead of continuing to patch this up with work arounds. I think long term there are some other solutions we can look at, but I'm not sure that adding 2 more event listeners does that, though it also likely doesn't hurt either.

@vadimka123

Copy link
Copy Markdown
Author

@ebonow , sorry, #2862 added by mistake

I agree that a partial solution
Looks like full working solution will be onScroll event listener instead of all others, but it's removed in version 2
Do you know why?

@ebonow

ebonow commented Mar 16, 2021 •

Copy link
Copy Markdown
Collaborator

I agree that a partial solution
Looks like full working solution will be onScroll event listener instead of all others, but it's removed in version 2
Do you know why?
@vadimka123

I do know why. It is related to how iOS used to only call the onScroll event when the scrolling has stopped which was further complicated by the built in inertia.
https://www.tjvantoll.com/2012/08/19/onscroll-event-issues-on-mobile-browsers/

I believe this has since been addressed in iOS8 or later, and put together this codesandbox to test if it can be worked back in.
https://codesandbox.io/s/fancy-cherry-l3vuh

I tested this against an iPhone simulator as well as someone else tested against an iPhone and it looks like scrolling is being updated in real-time now so I think that would be the better way to go. Not sure if you would be open to having a stab at this, though it may be worth waiting until after TypeScript refactoring is completed.

@ebonow

ebonow commented Mar 18, 2021

Copy link
Copy Markdown
Collaborator

Greetings @vadimka123 ,

We appreciate you putting this together but I think we would like to put together a more complete scrolling solution that would likely overwrite the already existing and this proposed code so we will be closing this. I will be sure to include you on any new developments.

Thank you again for putting this together and let us know if you would be instead in putting together a PR that uses the onScroll event handler.

@ebonow ebonow closed this Mar 18, 2021
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.

onMenuScrollToBottom not fired on desktop when the scrollbar is mouse-dragged to the bottom

2 participants