Page MenuHomePhabricator

Images above the fold should not be lazily loaded
Closed, ResolvedPublic3 Story Points

Description

I've heard this mentioned but never explicitly written down as a requirement.
Lazy loading an image that appears above the fold is wasteful and we should avoid as much as possible.
It was suggested we should look to lazy load images only if they appear in a section greater than 0 (the lead section)

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
OpenNone
Resolved Jdlrobson
Resolved Jdlrobson
DeclinedBBlack
Resolved Jdlrobson
ResolvedJhernandez
ResolvedBBlack
ResolvedNuria

Event Timeline

Jdlrobson updated the task description. (Show Details)
Jdlrobson raised the priority of this task from to Normal.
Jdlrobson added a project: MobileFrontend.
Jdlrobson moved this task from Backlog to Tasks on the MobileFrontend board.Feb 18 2016, 6:24 PM
dr0ptp4kt set Security to None.
dr0ptp4kt added a subscriber: dr0ptp4kt.

I'm landing this in sprint 67, but we'll be discussing next steps on lazy loading stage gating.

Peter added a subscriber: Peter.Feb 25 2016, 6:43 PM
Peter added a comment.Feb 25 2016, 6:48 PM

Also think this is important so that the look-ahead-parser can pick up the most important images and start download as fast as possible. Would love to have a metrics when the first image is displayed for the user.

Jdlrobson set the point value for this task to 2.Feb 26 2016, 10:34 PM
Jdlrobson changed the point value for this task from 2 to 3.Feb 29 2016, 5:34 PM

This proved to be a pain as I couldn't simply traverse upwards and look for the lead section (the section wrapping happens on getText function).

An alternative approach could involve adding data-section-ids to all images via a parser hook to avoid this scanning and turn this into a simple getAttribute lookup operation for section id.

Change 274786 had a related patch set uploaded (by Jdlrobson):
Only lazy load images outside the lead section

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

I don't agree with the approach in the patch. It seems very wasteful and inefficient (recursive traversal of siblings up a tree for each image node), I'd rather we do it the correct way if we have to do this feature right now.

As I wrote here https://phabricator.wikimedia.org/T124770#2011918

Another comment on unmet criteria for this task:

  • Wrap <img /> elements in noscript tags outside the lead section where the lead section is defined as section number > 0

After much dancing around in the code, this is right now really difficult or extremely hacky. The sectioning happens at the latest stage of the HtmlFormatter with regular expressions while the DOM transformation happens before, meaning when transforming the DOM (in this case for the images) there is no way to know in which section we are cleanly.

A fork that @phuedx has fixes this by performing sectioning cleanly using DOM transformations at the same stage where the other ones happen, but I don't feel comfortable changing the performance profile of the MobileFormatter and adding another factor that can affect the performance of the tests we want to run.

My suggestion is have a follow up patch for sending this feature to production (performing the sectioning cleanly and doing the lazy image transformation for sections > 0) after we have tested this patch and proven that it is useful and that we really want it.

I'd suggest changing sectioning to be dom based like in @phuedx's fork (integrate his changes), move it to happen before the transformations/filtering, and applying the transformations/filtering to the intended sections.

Happy to take over if you agree @Jdlrobson. Going to put it in -1 for discussing.

I don't agree with the approach in the patch. It seems very wasteful and inefficient (recursive traversal of siblings up a tree for each image node), I'd rather we do it the correct way if we have to do this feature right now.

As I wrote here https://phabricator.wikimedia.org/T124770#2011918

Another comment on unmet criteria for this task:

  • Wrap <img /> elements in noscript tags outside the lead section where the lead section is defined as section number > 0

After much dancing around in the code, this is right now really difficult or extremely hacky. The sectioning happens at the latest stage of the HtmlFormatter with regular expressions while the DOM transformation happens before, meaning when transforming the DOM (in this case for the images) there is no way to know in which section we are cleanly.

A fork that @phuedx has fixes this by performing sectioning cleanly using DOM transformations at the same stage where the other ones happen, but I don't feel comfortable changing the performance profile of the MobileFormatter and adding another factor that can affect the performance of the tests we want to run.

My suggestion is have a follow up patch for sending this feature to production (performing the sectioning cleanly and doing the lazy image transformation for sections > 0) after we have tested this patch and proven that it is useful and that we really want it.

I'd suggest changing sectioning to be dom based like in @phuedx's fork (integrate his changes), move it to happen before the transformations/filtering, and applying the transformations/filtering to the intended sections.

Happy to take over if you agree @Jdlrobson. Going to put it in -1 for discussing.

I'm fine with this... given we are now relying more on the MobileFormatter. It will also reduce bugs related to this. Feel free to propose an alternative patch.

Change 275878 had a related patch set uploaded (by Jhernandez):
Perform sectioning in MobileFormater before filtering

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

Change 275879 had a related patch set uploaded (by Jhernandez):
MobileFormatter: Move lazyLoadImages rewrite to be per-section

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

Change 275880 had a related patch set uploaded (by Jhernandez):
Only lazy load images after the first section

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

Change 274786 abandoned by Jdlrobson:
Only lazy load images outside the lead section

Reason:
Heading in favour of Joakino's solution

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

@Jhernandez would be great if you could grab me Friday morning before merging!

Change 275878 merged by jenkins-bot:
Perform sectioning in MobileFormater via DOM

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

URL for verification?

Change 275879 merged by jenkins-bot:
MobileFormatter: Move lazyLoadImages rewrite to be per-section

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

Change 275880 merged by jenkins-bot:
Only lazy load images after the first section

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

Jdlrobson closed this task as Resolved.

Deployed and signed off on http://reading-web-staging.wmflabs.org/wiki/Doctor_Who
First image is in HTML rest are not and are lazy loaded