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

Remove NodeBB scroll anchoring/anti-jellypotato code #6150

Closed
julianlam opened this Issue Dec 6, 2017 · 3 comments

Comments

Projects
None yet
1 participant
@julianlam
Copy link
Member

julianlam commented Dec 6, 2017

For the purposes of discussion, "scroll anchoring", "jellypotato fix", and "scroll shifting" all refer to the same thing.

As discussed on The Daily WTF, the old jellypotato fix code is no longer of sufficient quality and is in need of replacement. This degradation is likely due to the scroll shifting code being reliant on precise calculations of relative elements that either no longer exist or have moved, thus causing the calculation to no longer be correct. It also happened to not work with iframely, or any other plugin that manipulated the DOM after initial page load, as this code reacted only to images.

Also:

The current implementation (I'm guessing) seems to behave more like "oh ****, he's trying to display that part we delayed. Better load that image now"

☝️ which happens to be spot on, too.

Overall, the jellypotato-fix code was meant to be a band-aid while a more robust solution was developed. Not long afterwards, scroll anchoring was introduced in Chrome via a flag.

Rationale for complete removal:

  • If handling scroll anchoring/shifting is to be the responsibility of the browser, NodeBB should do its best to stay out of the way
  • Simpler is better:
    • I also recall that Chrome was beta testing some code in their browser that would compensate for jellypotato, and it wreaked bloody havoc on my code because Chrome would fix the scroll shift, and then NodeBB would fix™️ it again, making things worse.

In the end the decision was made to schedule removal of the scroll anchoring code if significant headway was made by a major browser vendor towards a solution. As of 6 Dec, Chrome has made this standard, while development of it still seems ongoing in Firefox. Perhaps we are already at this point:


Relevant discussions:

@julianlam julianlam self-assigned this Dec 6, 2017

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Dec 6, 2017

Upon testing the removal of NodeBB's scroll anchoring code, both Chrome and Firefox seem to handle image loading adequately. Firefox's throttling seems a little wonky, so navigating to a particular post didn't work, but Chrome's scroll anchoring works quite well.

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Dec 6, 2017

It seems that us moving the minfile to the footer may have had an unintended side effect in that images are now loaded first before requisite require.js modules, and so Topic.init may be fired late, avoiding the jellypotato problem altogether, at least in situations where you are navigating to a post directly.

julianlam added a commit that referenced this issue Dec 6, 2017

Added ajaxify.isCold()
And used it in app.js, so window.scrollTo(0, 0); is not called
on cold load (since you're already at the top). Useful in low-
bandwidth modes since you might accidentally get kicked to the
top of the page due to a slow connection and delayed .init().

Slightly related to testing in #6150

@julianlam julianlam closed this in 98c14e0 Nov 20, 2018

@julianlam

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment