@Nirzar: I believe we need a loading indicator asset from you (or for you to point us at one).

Should we also make sure the image dimensions are fixed on page load to avoid content reflow?

Let me come up with a video of how it might look while it's loading.

In T124770#1968205, @bmansurov wrote:

Should we also make sure the image dimensions are fixed on page load to avoid content reflow?

Yes. I believe this was mentioned in the meeting and recorded in the notes. Arguably this is captured in the "The loading of an image should not cause a repaint or jerk in the experience of a user." AC but I'll add a frinstance.

While loading...

After loading...

When the image appears we can add a small css transition of fade in.

Wrap <img /> elements in noscript tags (ignore browsers which support JS but ResourceLoader does not support for the time being)

This category ("Grade C" https://www.mediawiki.org/wiki/Compatibility) is growing rapidly and must not be ignored. Experimentation for the purpose of measuring impact is fine, but make no mistake that this is a use case that will affect the overall design of this feature, it should not be done as an after thought.

Relative size of Grade C is mostly steady, but in absolute numbers it is growing rapidly. New users as the worlds' population grows have newer devices, but users left behind add up. Aside from the growing number unsupported browsers, Grade C is also the experience you get if a user's device failed to load or execute the JavaScript for some reason (e.g. when connectivity is dropping, failing proxies, budget limitations, syntax errors, etc.).

In T124770#1986703, @Nirzar wrote:

While loading...

When the image appears we can add a small css transition

From the research @Peter and I did, it was clear that almost universally use of a spinner is discouraged for UX and perceived performance. It has little added value and the performance overhead of producing said spinner can even work negatively. More common is to have a plain color placeholder, or blurred image (using standard img-src, or base64; the latter is difficult due to the way we offload image processing outside of the web request). Browsers blur by default. Though CSS filters and Canvas are viable options as well.

Spinners could be left out by default and perhaps added later when e.g. an image takes more than 1 second to load to communicate some kind of progress.

@Jdlrobson wrote:

Note: https://github.com/mudkipme/mediawiki-lazyload

This does not seem suitable for inspiration. None of it looks appropiate or salvageable.

See also the summary from last week's meetings at https://phabricator.wikimedia.org/T124390#1987128.

Change 268058 had a related patch set uploaded (by Jhernandez):
Add feature flag for lazy loading images

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

Change 248312 had a related patch set uploaded (by Jhernandez):
WIP: Load images on demand when JS is enabled

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

In progress

https://i.imgur.com/IiQiUMc.gif

@Krinkle absolutely, we'll have to consider what to do for HTML only clients. We've discussed alternatives but at this stage we're letting them work normally. Some of the things discussed where serving downgraded images and just links to images in place of images (that I remember, there must be more). We'll get to it eventually, I've seen the comment on the blocked Epic which is awesome, hopefully more discussion will happen there.

Focus is now on measuring on beta cluster first and maybe in beta mobile later to gather numbers about how worth this is.

About UX, I'll leave comments to @Nirzar. For now IMO it serves the purpose for measuring and works, we'll have to iterate to get it to be awesome, whatever we choose to do now, and depending on what the data tells us.

Change 268185 had a related patch set uploaded (by Jhernandez):
WIP: Lazy load images (only JS, when configured)

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

Change 248312 abandoned by Jhernandez:
WIP: Lazy load images (only JS, when configured)

Reason:
Bmansurov is annoyed by notifications so work continues in https://gerrit.wikimedia.org/r/#/c/268185/. I'll link back to here.

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

What news @Jhernandez?

Using parser hooks for lazy loading images

As part of the implementation of lazy images in MobileFrontend
(https://gerrit.wikimedia.org/r/#/c/268185/) we were planning to use parser
hooks to do the HTML transformations so that upstreaming the feature to core
would be easier.

I've met with @phuedx to discuss options and we've gone through the hooks and
had a look about how we would go about implementing it right now.

TLDR

• We don't have the necessary hooks right now.
• If using parser hooks we need to take into account the parser cache and how we're going to split it.
• Best course seems to implement the feature in MobileFormatter which is basically a OutputPageAfterHTML (which doesn't exist right now).
  • Keep it isolated for easier extraction for upstreaming afterwards.
  • Make it work to start measuring in beta cluster.

PLAN

• Move on with the current patch and polish it for measuring in test environments and justify investment.
• If worth it and wanted:
  • Create a core patch to add OutputPageAfterHTML and figure out how to deal with the parser cache (if needed so).
  • Move lazy images code out of MobileFrontend into core patch.

We had a look at https://www.mediawiki.org/wiki/Manual:Hooks trying to find
what hooks to use.

We found https://www.mediawiki.org/wiki/Manual:Hooks/ImageBeforeProduceHTML
which let's you override the HTML generation (not useful, we want to modify the
generated HTML after the fact, not copypasta HTML generation and deal with alt
and title generation, etc).

Also https://www.mediawiki.org/wiki/Manual:Hooks/ThumbnailBeforeProduceHTML
which let's you tweak attributes before the HTML is produced (also not useful).

We had a look at many more without finding anything satisfying. What we really
need that would help cleaning MobileFormatter and MobileFrontend would be
having a hook like
https://www.mediawiki.org/wiki/Manual:Hooks/OutputPageBeforeHTML but after the
HTML has been generated, which is not available right now (and it is what
MobileFrontend does with MobileFormatter, basically, and without taking parser
cache into account).

We concluded that right now our best option to keep moving is to implement the
lazy loading as part of MobileFormatter (part of the unexistent
OutputPageAfterHTML) and keep the feature as isolated as possible so that it
could be easy to migrate afterwards.

That way we can deploy the change (which is feature flagged) to beta cluster
and would it be necessary a wiki's MobileFrontend Beta mode to perform real
measuring and get data on how useful this is and how strongly we need to push
for it, and once we have the data proceed with proper integration into
MediaWiki core.

That would entail sending a patch to core that would add an OutputPageAfterHTML
hook, and change MobileFrontend to hook into that one and apply the
MobileFormatter transformations. Then remove the lazy loading images functions
into a patch we can upstream, using feature flags, and move the frontend JS to
core too.

We feel this is the best way forward and hopefully all the explanation makes
sense. Any comments or suggestions are greatly welcome. I'll link to this
comment from the gerrit patch so that people with questions about the validity
of making this in MobileFormatter for now can get insight about the reasoning
and plans.

Cheers.

Thanks for write up.

In T124770#2007602, @Jhernandez wrote:

Using parser hooks for lazy loading images

As part of the implementation of lazy images in MobileFrontend
(https://gerrit.wikimedia.org/r/#/c/268185/) we were planning to use parser
hooks to do the HTML transformations so that upstreaming the feature to core
would be easier.

I've met with @phuedx to discuss options and we've gone through the hooks and
had a look about how we would go about implementing it right now.

TLDR

• We don't have the necessary hooks right now.
• If using parser hooks we need to take into account the parser cache and how we're going to split it.
• Best course seems to implement the feature in MobileFormatter which is basically a OutputPageAfterHTML (which doesn't exist right now).
  • Keep it isolated for easier extraction for upstreaming afterwards.
  • Make it work to start measuring in beta cluster.

PLAN

• Move on with the current patch and polish it for measuring in test environments and justify investment.
• If worth it and wanted:
  • Create a core patch to add OutputPageAfterHTML and figure out how to deal with the parser cache (if needed so).
  • Move lazy images code out of MobileFrontend into core patch.

---

We had a look at https://www.mediawiki.org/wiki/Manual:Hooks trying to find
what hooks to use.

We found https://www.mediawiki.org/wiki/Manual:Hooks/ImageBeforeProduceHTML
which let's you override the HTML generation (not useful, we want to modify the
generated HTML after the fact, not copypasta HTML generation and deal with alt
and title generation, etc).

Also https://www.mediawiki.org/wiki/Manual:Hooks/ThumbnailBeforeProduceHTML
which let's you tweak attributes before the HTML is produced (also not useful).

This might be useful if we decide to use images with a base 64 encoded thumbnail rather than the div approach.

We had a look at many more without finding anything satisfying. What we really
need that would help cleaning MobileFormatter and MobileFrontend would be
having a hook like
https://www.mediawiki.org/wiki/Manual:Hooks/OutputPageBeforeHTML but after the
HTML has been generated, which is not available right now (and it is what
MobileFrontend does with MobileFormatter, basically, and without taking parser
cache into account).

We concluded that right now our best option to keep moving is to implement the
lazy loading as part of MobileFormatter (part of the unexistent
OutputPageAfterHTML) and keep the feature as isolated as possible so that it
could be easy to migrate afterwards.

Okay. Given all above this seems like a sane decision. Let's focus on testing and fine tuning UX. Should be straight forward to do again in Parser hooks but let's continuously think about that.

That way we can deploy the change (which is feature flagged) to beta cluster
and would it be necessary a wiki's MobileFrontend Beta mode to perform real
measuring and get data on how useful this is and how strongly we need to push
for it, and once we have the data proceed with proper integration into
MediaWiki core.

That would entail sending a patch to core that would add an OutputPageAfterHTML
hook, and change MobileFrontend to hook into that one and apply the
MobileFormatter transformations. Then remove the lazy loading images functions
into a patch we can upstream, using feature flags, and move the frontend JS to
core too.

We feel this is the best way forward and hopefully all the explanation makes
sense. Any comments or suggestions are greatly welcome. I'll link to this
comment from the gerrit patch so that people with questions about the validity
of making this in MobileFormatter for now can get insight about the reasoning
and plans.

Cheers.

FYI, for people working on this, I've been looking into T60478, which is about distilling MediaTransformOutput into a more distinct/isolated component. Might be interesting as seeing something like this it as an 'alternate' renderer in that context. But i'm not sure when I will have time again to bring that part forward, so please don't wait on me. :)

Thanks for the link @TheDJ. We'll be sure to track T60478.

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.

With this said, I'm going to move the patch to code review. If you have anything to say chime in and let's discuss it.

I think something is wrong with the patch. I'm not sure where to upload my screencast, so I'm doing it here:

In T124770#2013683, @bmansurov wrote:

I think something is wrong with the patch. I'm not sure where to upload my screencast, so I'm doing it here:

This is what I said in the review. If you don't scroll no images will load (same happens for opening a section). I think it's good enough for a first pass to allow us to assess the impact, but if someone wants to fix that up for completeness they should feel free but it shouldn't block us.

@bmansurov I think scoping the initial call to loadImages to document.ready has made that bug work (at least for me), see https://i.imgur.com/zAnE5X8.gif and check the new patchsets. Thanks for catching it.

@Jdlrobson I've also added to the patch that lazy loading images get's triggered on toggling sections and fixed a double load handler being triggered on cached images (the complete stuff, see https://i.imgur.com/yyQFFNI.gif).

Check PS13 & 14. Thanks for the reviews!

@bmansurov I think scoping the initial call to loadImages to document.ready has made that bug work (at least for me), see https://i.imgur.com/zAnE5X8.gif and check the new patchsets. Thanks for catching it.

In my tests this works about 70-80%, but not in all cases. Try refreshing the page multiple times and you'll see that in one of those cases the image doesn't load.

In my tests this works about 70-80%, but not in all cases. Try refreshing the page multiple times and you'll see that in one of those cases the image doesn't load.

@bmansurov: Did you see anything to indicate that an error occurred? Nothing in the DevTools console?

I did not see any errors. If it helps I can make a screencast.

@bmansurov may it be because the document ready event is triggering before the viewport moves to where you were before?

How do we solve this?

@Jhernandez, I think you're right. We had this problem with the pointer arrow some time ago. I think what we did was that we figured out what was causing the viewport changes and emit events. There may be a better solution though.

In T124770#2015687, @Jhernandez wrote:

@bmansurov may it be because the document ready event is triggering before the viewport moves to where you were before?

How do we solve this?

Don't block on this. First version doesn't need to be perfect. We can fine-tune it as we go along.

I've created https://phabricator.wikimedia.org/T126590 to block putting this patch to production, and made a few tasks with the unknowns/questions from this version so that we don't forget to address them before deploying to prod.

The current version seems good enough to run tests on beta cluster.

Sub tasks:

• https://phabricator.wikimedia.org/T126593
• https://phabricator.wikimedia.org/T126592
• https://phabricator.wikimedia.org/T126591

Also, I'm blocked on ^ if we have to address the problems in this patch, and would really appreciate help.

I'm also leaving on a work trip for 2 weeks so I'll need somebody to take ownership of this from next week onwards.

Change 269985 had a related patch set uploaded (by Phuedx):
Replace transition with animation

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

Change 268058 merged by jenkins-bot:
Add feature flag for lazy loading images

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

Change 268185 merged by jenkins-bot:
Lazy load images (only JS, when configured)

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

Change 269985 merged by jenkins-bot:
Replace transition with animation

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

@Jdlrobson: 269985 was -1'd by @Jhernandez…

@Nirzar can you review?
http://reading-web-staging.wmflabs.org/wiki/Syrian_Civil_War?mobileaction=beta

It's a bit funky with tiny images in the infobox but I guess applying to all is the right behaviour.

@phuedx not sure what happened there (assume you mean 269985 please do a follow up, sorry about that - given its feature flagged I'm not too concerned)

Change 270259 had a related patch set uploaded (by Phuedx):
Also fade out image container when image loads

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

@phuedx not sure what happened there (assume you mean 269985 please do a follow up, sorry about that - given its feature flagged I'm not too concerned)

See 270259.

I agree that technically this wasn't too much of an issue and so it wouldn't have been too much of a problem to hold off merging until it was fixed – or even fixing it yourself.

I've deployed 270259 to the staging server.

I've moved this back to Code Review as 270259 still needs to be merged.

Change 270259 merged by jenkins-bot:
Also fade out image container when image loads

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