Page MenuHomePhabricator

Refreshing page with lazy loading images enabled causes images to not load *on load*.
Closed, ResolvedPublic

Description

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.

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
OpenNone
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedBBlack
ResolvedJdlrobson
ResolvedJhernandez
ResolvedBBlack
ResolvedNuria

Event Timeline

Jhernandez raised the priority of this task from to Needs Triage.
Jhernandez updated the task description. (Show Details)
Jhernandez added a project: MobileFrontend.
Jhernandez added a subscriber: Aklapper.
Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptFeb 11 2016, 10:27 AM
Jdlrobson triaged this task as Normal priority.Feb 11 2016, 4:56 PM
Jdlrobson added a subscriber: Jdlrobson.
Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.Feb 18 2016, 6:25 PM

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.

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-67-If, Then, Else...? board.

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

https://gerrit.wikimedia.org/r/274319

This comment was removed by Jdlrobson.

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:

  • Trigger lazy load images after html jumpscares (like TOC insertion) (like we do on section collapsing)
    • Think if there are more html jumpscares that we should take into account
  • Change the in viewport detection to have some threshold and to properly calculate if something is in bounds (so that https://i.imgur.com/2XfHeQ1.gif doesn't happen). Here is an example that could be useful

Change 274319 merged by jenkins-bot:
Avoid pushing down content on image load

https://gerrit.wikimedia.org/r/274319

I'll address the TODOs I added.

Change 275875 had a related patch set uploaded (by Jhernandez):
Trigger lazy loading images after Skin changes

https://gerrit.wikimedia.org/r/275875

Change 275876 had a related patch set uploaded (by Jhernandez):
Upgrade isElementInViewport to be more exact

https://gerrit.wikimedia.org/r/275876

Change 275875 merged by jenkins-bot:
Trigger lazy loading images after Skin changes

https://gerrit.wikimedia.org/r/275875

@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.

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?

You're right. I'm unable to replicate without QuickSurveys.

Change 275876 merged by jenkins-bot:
Upgrade isElementInViewport to be more exact

https://gerrit.wikimedia.org/r/275876

@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.

Create new task? back to TODO?

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.

Jdlrobson closed this task as Resolved.Mar 11 2016, 11:47 PM

I'm satisfied we've done enough so far.