Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Check scrollability by actually scrolling elements #854

Merged
merged 1 commit into from
Dec 27, 2020
Merged

Check scrollability by actually scrolling elements #854

merged 1 commit into from
Dec 27, 2020

Conversation

femnad
Copy link
Contributor

@femnad femnad commented Sep 25, 2020

Addresses #825

When findScrollable yields no results, check if we can return the current scrollingElement instead, ignoring doneScrolling check this time. This fixes the behaviour when it's not possible to scroll up from the bottom of a page.

@ghost
Copy link

ghost commented Oct 9, 2020

I have tested this commit on the following web pages:

  1. https://en.wikipedia.org/wiki/Main_Page
  2. https://music.youtube.com
  3. https://github.com/enkore/j4-dmenu-desktop

It works on Wikipedia but, unfortunately, it does not work for the others.

Tested on Firefox 82.0b8.

@femnad femnad changed the title Return scrollingElement if no child scrollables Assume scrolling done on exceeding element height Oct 9, 2020
@femnad
Copy link
Contributor Author

femnad commented Oct 9, 2020

Yeah, you're right, embarrassingly I've only tested on Hacker News. However, the behavior on those sites you've mentioned is similarly broken on master. So I propose changing the scrolling logic somewhat which has the downside of reintroducing #267 but fixes the scrolling on other sites. This could be applied as a temporary measure to fix the existing scrolling bug until there is another implementation which can address #267 without causing problems in other sites.

Addresses #825

In addition to style checks, actually scroll an element (and restore
back) to verify that it can be scrolled.

Inspiration from [Vimium](https://github.com/philc/vimium) scrolling
code.
@femnad femnad changed the title Assume scrolling done on exceeding element height Check scrollability by actually scrolling elements Oct 10, 2020
@femnad
Copy link
Contributor Author

femnad commented Oct 10, 2020

New proposal; when seeking scrollable elements, evaluate style checks first and then actually scroll (down and back up) the element to see if it can be scrolled. Credit for that approach goes to Vimium.

Tested on:

  • Hacker News
  • Gmail
  • Youtube Music
  • Github repo pages
  • Wikipedia main page

@ghost
Copy link

ghost commented Oct 11, 2020

I can confirm it's working great on all those previously affected web pages with the latest commit. Excellent work @femnad!

Copy link
Owner

@ueokande ueokande left a comment

Choose a reason for hiding this comment

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

great!

@ueokande ueokande merged commit 1456574 into ueokande:master Dec 27, 2020
@femnad femnad deleted the 825-fix-scroll-up-from-bottom branch December 27, 2020 23:15
@aminroosta
Copy link
Contributor

aminroosta commented Dec 28, 2020

Just saw this in the release notes.
Guys, you rock! Thank you so much for keeping this the best vim plugin for firefox.

@apiraino
Copy link

thanks @femnad for fixing this up :)

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

Successfully merging this pull request may close these issues.

4 participants