Page MenuHomePhabricator

Problems with lazy loaded inline images
Closed, ResolvedPublic3 Story Points

Description

There are various issues now with regards to inline images (see subtasks).
Let's try and find a good solution for them.

Strawman solution

Any image where width is under 50px or the height is under 50px is not lazy loaded and the markup will be left untouched. This may negatively impact first paint on slow connections due to parallel HTTP requests but it's probably not worth the effort given all the bugs we've had.

Articles with math equations, flag icons should not be lazy loaded.

Sign off

  • Go through every subtask associated with this bug and resolve any bugs fixed by the change. IF something is still broken comment on it with the generic comment "This was not fixed by T162623"

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2017, 6:03 PM
Jdlrobson triaged this task as High priority.Apr 10 2017, 6:09 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from To Triage to Upcoming on the Readers-Web-Backlog board.

Let's get this done so we can put a dent in the backlog.

Jdlrobson updated the task description. (Show Details)Apr 11 2017, 4:51 PM

@phuedx asked me to write this.
We talked about the proposal and I argued that this is the lowest risk change we can make to resolve all the subtasks of If we find this doesn't help those bugs and doesn't we should revert the patch and try something else.

Preliminary estimates varied between 2 and 5.

ovasileva set the point value for this task to 3.Apr 11 2017, 5:09 PM

Change 347649 had a related patch set uploaded (by Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Don't lazy load small images (smaller than 50px)

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

Jdlrobson updated the task description. (Show Details)Apr 13 2017, 6:53 PM
Jdlrobson added a comment.EditedApr 20 2017, 7:56 PM

The following table tests different solutions against our open bugs.

SolutionNirzar approved animationsT154478T160527T159100T160946T150052 (small ios)T146298 (avoids reflows)Tradeoff
CurrentYN?NNNNN/A
rEMFR6177cf09cb63: Don't lazy load small images (smaller than 50px)Y (same)N?YNYYUnconditonally Load inline images
rEMFRdc00c36bdb8f: Restrict lazy loaded images to the viewportYY?NNNNNone
rEMFR162394330f99: POC: Use JavaScript to animate the ease-in of lazy loaded imageYY?YNYNReflows still possible

A couple of questions:

  • How does the proposed solution work when images can be in non-pixel, e.g. ex, units?
  • Are we not trying to reduce bytes shipped with lazy-loaded images? If so, then the acceptance criterion of looking at the width or height of an image and comparing it to 50px doesn't sound good because, an image may have a width of 50px and a height of 999px. It would be wise to lazyly load such images.

I suggest we use the weight of an image, in KB for example, to decide whether to lazyly load it or not.

@bmansurov proposed solution allows all images smaller than 50px or 10ex (width or height)

@bmansurov proposed solution allows all images smaller than 50px or 10ex (width or height)

I don't see the mention of '10ex' in the description. Also, that doesn't answer my second question, does it?

These are the solutions on the table:

  1. We unconditionally load images where width or height is <50px. ( rEMFR6177cf09cb63: Don't lazy load small images (smaller than 50px)+ rEMFRdc00c36bdb8f: Restrict lazy loaded images to the viewport)

The trade off of this solution is we will load more bytes of images. So @bmansurov yes in your example, 50x999 will be loaded. That's a rare edge case on Wikipedia but it's something we have to agree with to go with this solution.

  1. We use replaceWith ( rEMFR162394330f99: POC: Use JavaScript to animate the ease-in of lazy loaded image )

The issue with this is that there will be noticeable reflows in certain articles (see T146298 ... but I suspect there's a bug there unrelated to lazy loading images) and there is a slight change in the loading animation which has been approved by @Nirzar

  1. Use bytes ( ? )

As @bmansurov suggests we could use bytes, but I'm not sure how that works. We have a URL not a filepath. How does that work for foreign files on a different domain? I'm worried that might be a little expensive. If someone wants to put a strawman solution for this go for it.

I should add none of the three solutions will fix T160946. It turns out that's unrelated to lazy loaded images.
I believe T160527 is already fixed on master so I haven't verified solutions against that.
Evaluating solution 1 and 2, both would be fine and solve the underlying bug.
I'm going to investigate T146298 a little more to see if I can understand what's going on there since it seems to be the main disadvantage of solution 2.

@bmansurov yes, I just answered your first question.

As we found out that all math images use ex instead of px I added another check - dimension has to be smaller than 10ex or 50px.
10ex was picked by checking many pages with Math equations and most of images were smaller than 10ex.

PS3 rEMFR0737ec068fe5: Use JavaScript to animate the ease-in of lazy loaded image seems to reduce the issue of reflows. @Krinkle I'd be interested in your thoughts about it. I've been trying it out on some real articles such as http://reading-web-staging.wmflabs.org/w/index.php?title=Black-body_radiation and I'm not seeing any nasty side effects...

@pmiazga let's chat tomorrow or Monday. Sorry for the change in scope!

Change 347649 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Don't lazy load small images (smaller than 50px or 10ex)

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

Jdlrobson reassigned this task from pmiazga to ABorbaWMF.Apr 24 2017, 9:46 PM
Jdlrobson moved this task from Needs Code Review to Needs QA on the Reading-Web-Sprint-96 board.

Can you do some exploratory testing over the course of this week with regards to lazy loading images? This can be done on the beta cluster today, on mediawiki.org tomorrow, he.wikipedia.org on Wednesday and en.wikipedia.org on Thursday.

Essentially we want to see if there are any problems with images not loading on certain pages or looking strange in the context. The subtasks will provide some ideas for how we can do this.

No worries if not, but we should hold off signing off on this till next week.

Jdlrobson closed this task as Resolved.Apr 28 2017, 4:19 PM

I've confident we've eradicated most of the issues with lazy loading images.
There are a couple of minor issues left:

  • There are minor barely noticeable reflows (see T146298) with images which have widths greater than the viewport
  • When connects drop it's still possible for an image to never load (see T136693)

Both need some analysis and for us to work out solutions.