Page MenuHomePhabricator

Lazy loaded images: MobileFormatter should take into account mobile image dimension adjustments
Open, MediumPublic

Description

Problem: The lazy loading image placeholders do not always match the height of the images that get loaded on mobile and this causes a reflow.

bug report

This happens for the The famous The Creation of Adam by Michelangelo, c.1512 image in the the God article ('Christianity' sub-section of 'Depiction' section). It seems the placeholder span (.lazy-image-placeholder) is not getting contained within it's parent. This seems to be hindering with single pagedness of the article and leads to some wasted space in left just because of that placeholder. This is particularly frustrating when the 'Expand all sections' preference is enabled.

Before loading the placeholder has the following HTML

<div class="thumbinner" style="width:502px;">
  <a href="/wiki/File:The_Creation_of_Adam.jpg" class="image view-border-box">
    <noscript>
      <img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/6/63/The_Creation_of_Adam.jpg/500px-The_Creation_of_Adam.jpg" width="500" height="230" class="thumbimage" data-file-width="1614" data-file-height="741">
    </noscript>
    <span class="lazy-image-placeholder" style="width: 500px;height: 230px;" data-src="//upload.wikimedia.org/wikipedia/commons/thumb/6/63/The_Creation_of_Adam.jpg/500px-The_Creation_of_Adam.jpg" data-alt="" data-width="500" data-height="230" data-class="thumbimage">&nbsp;
    </span>
  </a>
  ....
</div>

After getting loaded, the image has the following HTML

<div class="thumbinner" style="width:502px;">
  <a href="/wiki/File:The_Creation_of_Adam.jpg" class="image view-border-box">
    <noscript>
      <img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/6/63/The_Creation_of_Adam.jpg/500px-The_Creation_of_Adam.jpg" width="500" height="230" class="thumbimage" data-file-width="1614" data-file-height="741">
    </noscript>
    <img class="thumbimage image-lazy-loaded" src="//upload.wikimedia.org/wikipedia/commons/thumb/6/63/The_Creation_of_Adam.jpg/500px-The_Creation_of_Adam.jpg" alt="" style="width: 500px;height: 230px;" width="500" height="230">
  </a>
  ...
</div>

Screen shot
Placeholder

lazy-load-placeholder-oversized.png (800×480 px, 75 KB)

Image

Screenshot_2018-06-14-02-20-52.png (800×480 px, 261 KB)

Developer notes

The problem here is the image is 500px - greater than a mobile screen. All images on mobile are restricted to 320px.
Currently the lazy loading image placeholder doesn't take this restriction to account so it's height and width will always reflect the non-mobile-optimised version of the image.

Approach 1 - use img tag

Instead of a span for the placeholder we could use an img tag and use a base64 URL for all images. We lean on the browser to keep the aspect ratio of the images.

Approach 2 - guess height

Given we know about the restriction at formatting time and that the image dimensions - 680x312- will exceed this and the maximum size (320px) we can calculate the height:
320 / 680 * 312 = 146px.
and set the height to this parameter:
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/includes/MobileFormatter.php#L398

When the image is less than 320px we can safely ignore any height calculations.

Downside of this approach is that we'll need to sync CSS value which sets a max-width of 320px to the MobileFormatter value and for devices less than 320px this will continue to be a problem (albeit less noticeable)

An early attempt exists: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/447765/

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Kaartic renamed this task from v1aaaaaaaa to MobileFormatter should take into account mobile image dimension adjustments.Jul 1 2018, 3:40 PM
Kaartic lowered the priority of this task from High to Medium.
Kaartic updated the task description. (Show Details)
Kaartic added a subscriber: Aklapper.

It seems like the proposed solution is specific to mobile devices. What will this look like on a tablet, for example?

Downside of this approach is that we'll need to sync CSS value which sets a max-width of 320px to the MobileFormatter value and for devices less than 320px this will continue to be a problem (albeit less noticeable)

IIRC we can use the [[ https://www.mediawiki.org/wiki/Manual:Extension.json/Schema#ResourceLoaderLESSVars | ResourceLoaderLESSVars ]] hook for this.

I agree, an image may have article content or device constraints. I propose we use the solution in the wikimedia-page-library instead, a padded span within a span for placeholders to enforce a ratio that is kept regardless of rendered width. For example, the image:

<!-- An image with a 1 to 2 width to height ratio. -->
<img width=100 height=200 src=picture.png>

Becomes:

<span
  class='pagelib_lazy_load_placeholder pagelib_lazy_load_placeholder_pending'
  style='width: 100px'
  data-width=100
  data-height=200
  data-src=picture.png>
    <span style='padding-top: 200%'><!-- Proportional height of fully rendered parent. --></span>
</span>

Change 447765 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Add span inside lazy image placeholder to cope with image resizes

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

instead, a padded span within a span for placeholders to enforce a ratio that is kept regardless of rendered width

Wow. Did not know this was a possibility. That's awesome. I don't understand why it works though.

https://blog.wikimedia.org/2017/12/06/think-twice-code-once/ doesn't mention why the span with padding top solves this problem. Could it be added in a comment/follow up or did I miss something?

Change 447765 abandoned by Jdlrobson:
Add span inside lazy image placeholder to cope with image resizes

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

Jdlrobson set the point value for this task to 5.Jul 31 2018, 2:02 AM
Jdlrobson added subscribers: nray, pmiazga, Stephen.

Async estimation (@pmiazga: 3, @Stephen: 5, @Jdlrobson: 5, @nray: 3)

jdlrobson [17 hours ago]

I think MobileFormatter changes in general are a little risky and images can define width/height in styles and attributes and in pixels and other units (don't forget auto!). We'll need to use some regexs here and I guess provide some test cases.

jdlrobson [17 hours ago]

@pmiazga want to reflect on your 3?

pmiazga [11 hours ago]

I think that 5 is reasonable estimate, I went with 3 points as I think I know that part of code and it is not that difficult. But I agree, that part of code is tricky, Let's go 5

Both @nray and @Niedzielski feel free to speak up if you are not happy with the outcome estimation :)

Jdlrobson renamed this task from MobileFormatter should take into account mobile image dimension adjustments to Lazy loaded images: MobileFormatter should take into account mobile image dimension adjustments.Nov 15 2018, 6:56 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: ovasileva.

@ovasileva any chance we can schedule some time for this and the other 3 bugs listed as subtasks post-AMC deploy? (T229472 tracks them)

Change 617099 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Adjust placeholder dimensions (if possible) to max 320px keeping aspect ratio

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

Jdlrobson removed the point value for this task.Aug 5 2020, 5:16 PM

Dropping the points on this task as this didn't work out first time round.

So, use-cases are:

  1. for wide-images (panoramas) we have to keep the height of original image
  2. for normal images we need to transform placeholder's size to image's one keeping aspect ratio

Developer notes:

  • It's impossible simply scale width and height as then we break 1.
  • It's impossible to use min-height is also not going to work as it changes size of element once image is loaded in case 1.

So it's hard to fix this using style property.

The possible solution is to implement conditional scaling: scale for 2) and leave original for 1).
We can rely "noresize" class of one of parents for that.

ovasileva set the point value for this task to 5.Aug 24 2020, 5:28 PM

Some more details on this issue for the sake of history:

  1. Panorama feature is implemented as Lua module:

https://en.wikipedia.org/wiki/Module:Wide_image

  1. It wraps an image with div with 'noresize' class.
  1. We need implement different behaviour on javascript side, so we need to pass some information about the image and how to interpret it anyway.

In the patch I've chosen a way to pass different sizes depends on wrapping div size.

I need to setup a meeting between @phuedx, myself and @Peter.ovchyn to talk through this one.

Change 617099 abandoned by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Adjust placeholder dimensions (if possible) to max 343px keeping aspect ratio

Reason:
Without peter, we don't have the bandwidth to work on this right now. It was clear from working on this that the bug oversimplifies the solution here so this exercise was really useful.

We will refer back to this patch when we have the time to focus on this problem again.

Thanks Peter for your work here!

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

Jdlrobson removed the point value for this task.

Change 643543 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Avoid reflows when lazy loading images

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

Change 643543 abandoned by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Avoid reflows when lazy loading images

Reason:
abandoning for now. The natural height of a 1x1 square is considered as well as the width and height attributes of the img tag itself. https://web.dev/aspect-ratio/ will be needed to resolve this problem. Perhaps that is something @Gilles could help the whatwg prioritize for this use case?

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