As per https://phabricator.wikimedia.org/T124770#2014696
It seems that with https://gerrit.wikimedia.org/r/#/c/248312/ there are some cases in which when reloading the page when being in the middle of it won't load the images on the viewport.
Suspicions are that the document ready event is triggering before the browser moves the viewport to where it was before triggering the loadImages on the initial viewport instead of on the moved one.
Investigation needed.
There are various features that result in the browser moving or hanging the viewport during the load phase.
I think running the loadImages() check function on the window load event as well (not just document ready) should solve this problem. In some cases, the code in question might only start running after window load, it'll be a no-op in that case as the load event won't fire a second time or retroactively, but that's fine. the document-ready callback will still be invoked.
Google's mod_pagespeed lazy-load implementation, for example, does that too.
So I've investigated this and I can only replicate this when QuickSurveys is installed and running.
QuickSurveys pushes content down and causes a reflow. The table of contents and image placeholders borders may also be adding to this issue but that's not clear right now to me as I haven't been able to replicate with that.
@bmansurov can you confirm whether you can replicate the issue without QuickSurveys?
Change 274319 had a related patch set uploaded (by Jdlrobson):
Avoid pushing down content on image load
I've merged the tweak (which is correct) but I don't think that helps with this bug.
In my tests the weird jump happens because the table of contents on Tablet+ widths and it can actually leave an image in viewport that wasn't without actually being loaded.
I've also noticed that the inViewport function doesn't consider images partially in viewport to be in viewport (from a certain point), so it won't load images which are half way in in viewport, leaving them gray when they probably shouldn't be. See https://i.imgur.com/2XfHeQ1.gif for an example.
I'd suggest a couple of TODOs:
Change 275875 had a related patch set uploaded (by Jhernandez):
Trigger lazy loading images after Skin changes
Change 275876 had a related patch set uploaded (by Jhernandez):
Upgrade isElementInViewport to be more exact
@jhobs Please, can you please give me pointers to the core implementation of this patch?
Alright, the core implementation is broken, I've raised T129466 for fixing it, and added followup patch+task removing the custom implementation here T129468 when core's version is fixed.
For now I think we should merge https://gerrit.wikimedia.org/r/#/c/275876/ which improves the custom implementation until the core version is fixed and we can then merge the custom removal patch that follows up.
@bmansurov Should we trigger a "changed" event on the skin object from Extension:QuickSurveys after changing the html content? It really makes sense given the definition of the event (https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.startup/Skin.js#L156-L159) and that would make sure that skin handlers get triggered when the HTML jumps (in this case because quick surveys).
Yes we should.
The only problem is QuickSurveys shouldn't know about the Skin class
We should probably thus make the Skin class use mw.hook and QuickSurveys subscribe to it.
I've setup T129571
Given QuickSurveys do not run all the time and we have control over when they do, I think this task can be signed off without it.