Handle scrolling on Reddit redesign

[marcotc]

This PR adds support for scrolling the through the new Reddit post popups, introduced during the ongoing Reddit redesign effort.

Before scrolling with j and k would scroll the background, instead of the foreground article:

With the changes in this PR, it will now scroll the popup post correctly:

Implementation notes

Unfortunately document.scrollingElement will always return the document's body, meaning we never reach our fallback codes that could handle edge-cases like this.

The solution implemented in this PR is ad-hoc, which is subpar to say the least. It is based on a recent similar fix for Twitter.

If this solution is acceptable for now, I'm willing to refactor these two ad-hoc to reduce code duplication.
I could move them into an external configuration file instead, as the only variables are the URLs affected, and how to find the scrollable element.

[smblott-github]

This is more or less what I was going to try to get to. Reddit is sufficiently important to warrant a site-specific hack.

If this solution is acceptable for now, I'm willing to refactor these two ad-hoc to reduce code duplication.

Yes. Please refactor to eliminate duplication. Perhaps with configuration via a dictionary mapping hostnames to CSS. No need to go so far as an external configuration file just yet. It's enough to have the code isolated from the real scrolling logic at the bottom of scroller.coffee.

[marcotc]

Thank you for the review @smblott-github, I'll make changes to implement your feedback.

[marcotc]

@smblott-github I took at stab at refactoring it and pushed a few changes.

One thing that I'm not satisfied with is how I had to bust the value of activatedElement.
Because activatedElement is cached globally inside scroller.coffee, it ends up storing the first value it sees, and only refreshes when you navigate to another page.
Unfortunately Reddit uses history manipulation to move between from list of posts, to each post itself.

I found some code around vimium_frontend.coffee:checkIfEnabledForUrl() that gets triggered on history manipulation. That code actually calls installModes() the first time it runs, which initializes the Scroller, but on further history updates, it gets skipped.

It seems to me that any scrolling state should be reset when history changes, so my thought was to refactor scroller.coffee to make the whole scrolling logic be created anew on history change, but that's a rather large endeavour. This solution is my preference, but I don't like the possible sizable refactoring required for it.

Another option is to create a vimium_frontend.coffee:installModes() function, that gets called like so:

 checkIfEnabledForUrl = do ->
   Frame.addEventListener "isEnabledForUrl", (response) ->
    # ...
    if normalMode
      refreshModes()
    else
      installModes()

That function would contain only Scroller.init() currently. (Or even better, Scroller.refresh()) .

[smblott-github]

Thanks, @marcotc.

It seems to me that any scrolling state should be reset when history changes...

That's an interesting observation. You're right.

I made a couple of tweaks (72aaf05). Let me know if you spot a mistake.

Once again, thanks.

[marcotc]

Hi @smblott-github, thank you for merging this.
Seems fine to me, thanks again for all the help!

[devanshuDesai]

Still facing this issue with j and k scrolling as of December 9, 2018.

[marcotc]

Confirmed regression, almost certain Reddit has changed their layout.

[tehp]

Bump, also experiencing this issue.

[kitaev]

Doesn't work for me with Firefox 65.0.2 on Linux. Vimium version 1.64.3.

[sooriravindra]

Doesn't work on Chrome Version 73.0.3683.103 (Official Build) (64-bit)

[dseeni]

same here as well, using the 74.0.3729.108 build of chorme