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

Improve smooth scroll #2016

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Conversation

Spindlyskit
Copy link
Contributor

Currently, smooth scroll is considerably slower than default scrolling when scrollline is called during an active scroll, it seems this is because the new endpoint is based of the current scroll position rather than the current destination. This change calculates the new endpoint using the previous endpoint rather than current position if a scroll animation is already in progress.

@glacambre
Copy link
Member

Hi, thanks for opening this PR! The code looks good to me but scrolling is super tricky to get right so I'd like to test it on these websites before it is merged:

I'll do that in a few hours :).

@Spindlyskit
Copy link
Contributor Author

Spindlyskit commented Nov 25, 2019

I've briefly looked over the sites (I don't exactly know what tests need to be done) and almost all seem to work correctly, that said, I was unable to get the riot room to scroll however this was also the case on the latest tridactyl from amo with smoothscroll on or off, this is likely a problem on my end.

The other issue I had was with twitter, I found it would scroll as expected on both the latest (amo) and my changed version of tridactyl whilst logged in however the page would not scroll without a login session. This issue also seems to be present on latest tridactyl and might need investigation, it seems to be caused by the logged-out version of twitter rendering the profile feed in the background of the post which does not happen when logged in.

EDIT: The twitter issue seems to be (#1521)

@glacambre
Copy link
Member

Thanks for taking the time to test your PR on these websites. I tested it too (the test is just checking if scrolling still works and didn't become super slow both with and without smoothscroll - we've had seemingly innocuous changes break scrolling in bizarre ways before) and everything works as expected.

Copy link
Member

@glacambre glacambre left a comment

Choose a reason for hiding this comment

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

Makes smoothscroll much better - to the point where we can probably close all issues about smoothscrolling. Thank you!

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.

3 participants