Page MenuHomePhabricator

Lazy loaded images: MobileFormatter should take into account mobile image dimension adjustments
Open, NormalPublic5 Story Points

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

Image

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.

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

Kaartic created this task.Jun 13 2018, 8:52 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 13 2018, 8:52 PM
Jdlrobson renamed this task from Placeholder for lazy loaded image has incorrect dimension compared to actual image in a particular case to MobileFormatter should take into account mobile adjustments.Jun 15 2018, 8:17 PM
Jdlrobson triaged this task as Normal priority.
Jdlrobson added a project: Performance.
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from MobileFormatter should take into account mobile adjustments to MobileFormatter should take into account mobile lazy loading adjustments.Jun 19 2018, 4:22 PM
Jdlrobson renamed this task from MobileFormatter should take into account mobile lazy loading adjustments to MobileFormatter should take into account mobile image dimension adjustments.
Vvjjkkii renamed this task from MobileFormatter should take into account mobile image dimension adjustments to v1aaaaaaaa.Jul 1 2018, 1:04 AM
Vvjjkkii raised the priority of this task from Normal to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
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 Normal.
Kaartic updated the task description. (Show Details)
Kaartic added a subscriber: Aklapper.
phuedx added a subscriber: phuedx.Jul 24 2018, 4:46 PM

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

phuedx added a comment.EditedJul 24 2018, 4:48 PM

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 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 updated the task description. (Show Details)
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)