Page MenuHomePhabricator

Lazy loaded images: Lazily loaded inline images cause reflow - should retain other classes such as `thumbborder`
Closed, ResolvedPublic3 Estimated Story Points

Description

Note: There are several lazy loaded images bugs T207929, T199351, T209183. We'll probably want to fix these 3 bugs together.

Steps to Reproduce

  1. Visit https://en.m.wikipedia.org/wiki/2018_FIFA_World_Cup
  2. Scroll quickly to the bottom of the page

Expected Results

  • Images load in place without shifting contents

Actual Results

  • Images sometimes shift the contents horizontally and vertically:

Environments Observed

Browser Version

  • Chromium v66.0.3359.181

OS Version

  • Ubuntu v18.04

Device Model

  • Desktop

Device Language

  • English

Precursor

Ensure T197188 and T200518 is estimated and also on the sprint board.

Developer notes

Original HTML (ignoring noscript)

<span style="white-space:nowrap">
  <span class="flagicon">
      <span class="lazy-image-placeholder" style="width: 23px;height: 15px;" data-src="//upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/23px-Flag_of_Tunisia.svg.png" data-alt="" data-width="23" data-height="15" data-srcset="//upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/35px-Flag_of_Tunisia.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/45px-Flag_of_Tunisia.svg.png 2x" data-class="thumbborder"> </span>
  </span>
  <a href="/wiki/Tunisia_national_football_team" title="Tunisia national football team">Tunisia</a>
</span>

is replaced with

<span style="white-space:nowrap">
  <span class="flagicon">
    <img class="thumbborder image-lazy-loaded" width="23" height="15" src="//upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/23px-Flag_of_Tunisia.svg.png" alt="" style="width: 23px;height: 15px;" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/35px-Flag_of_Tunisia.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/c/ce/Flag_of_Tunisia.svg/45px-Flag_of_Tunisia.svg.png 2x">&nbsp;</span>
  <a href="/wiki/Tunisia_national_football_team" title="Tunisia national football team">Tunisia</a>
</span>

The issue here is the thumbborder class adds a border to the image which changes the height and width of the image.
Let's ensure the lazy loaded image placeholder retains this thumbborder.
e.g.

<span class="lazy-image-placeholder"

becomes

<span class="lazy-image-placeholder thumbborder"

https://gerrit.wikimedia.org/r/448415 pulls out the LazyImageTransform from the MobileFormatter.
Right now we don't inspect the classes of the img tag - only the style.

Another root of issue is inconsistency in css between img and lazy-image-placeholder.

.content img {
    vertical-align: middle;
}

The thing is that originally placeholder is a span that dynamically changed on img. Therefore css rule starts working after that and causes that shift.

See: https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/146cb2bb3b3acb3c0c3da84e97fc2f585cd95230/resources/skins.minerva.content.styles/images.less

Acceptance criteria

  • When creating the lazy-image-placeholder copy across the thumbborder class if it exists.
  • To minimise risk, do not copy across other classes, but let's ensure our solution allows expansion in future in case other classes need to be copied across
  • Make sure to add a test case to the new LazyImageTransform class.

QA Results - Beta - Aug 8, 2020

ACStatusDetails
1T199351#6370886

QA Results - Beta - Aug 17, 2020

ACStatusDetails
1T199351#6391790

QA Results - Beta - Aug 24, 2020

ACStatusDetails
1T199351#6408055

QA Results - Prod - Aug 26, 2020

ACStatusDetails
1T199351#6414848

QA Results - Prod - Aug 27, 2020

ACStatusDetails
1T199351#6417638

QA Results - Prod - Aug 27, 2020

ACStatusDetails
1T199351#6418022

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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)

Jdlrobson renamed this task from Lazy loaded images: Lazily loaded inline images cause reflow - should retain class attribute such as thumbborder to Lazy loaded images: Lazily loaded inline images cause reflow - should retain other classes such as `thumbborder`.Jul 21 2020, 12:20 AM
Jdlrobson removed ovasileva as the assignee of this task.
Jdlrobson added a subscriber: Peter.ovchyn.

@Peter.ovchyn I'd suggest this task next given you are in this code already.

Change 616216 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Factor Style-related functions out of LazyImageTransform

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

Change 616222 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Implement copying 'thumbborder' class from image to image placeholder

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

Change 616216 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Factor Style-related functions out of LazyImageTransform

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

Change 616222 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Copy 'thumbborder' class from image to image placeholder

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

Change 618058 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/MobileFrontend@master] DNM: LazyImageTransform: Introduce CssClassList value object

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

phuedx added a subscriber: phuedx.EditedAug 3 2020, 5:19 PM

@Peter.ovchyn: I created the above as a quick sketch of what I was suggesting during code review. If you've got a moment, I'd appreciate your review of it.

Edtadros reassigned this task from Edtadros to Peter.ovchyn.Aug 8 2020, 6:13 PM
Edtadros added a subscriber: Edtadros.

Test Result - Beta

Status: ❌ FAIL
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhone 5

Test Artifact(s):

QA steps

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Flag_images
Scroll quickly to the bottom of the page
❌ AC1: Images load in place without shifting contents
There is some shifting of the images.

Edtadros updated the task description. (Show Details)Aug 8 2020, 6:14 PM
Peter.ovchyn added a comment.EditedAug 9 2020, 9:41 AM

This shifting caused by css:

.content img {
    vertical-align: middle;
}

The thing is that originally placeholder is a span that dynamically changed on img. Therefore css rule starts working after that and causes that shift.

See: https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/146cb2bb3b3acb3c0c3da84e97fc2f585cd95230/resources/skins.minerva.content.styles/images.less

Change 619724 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] DNM: Add vertical-align:middle; to .lazy-image-placeholder class.

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

@Jdlrobson, I've added the fix to MobileFrontend, but think it breaks the breaks the equity of root and fix level principle. My thoughts are following:

I assume that MobileFrontend is an extension that supplies adaptation of html to mobile screen sizes. It knows (and it shouldn't ) nothing about theme and should work with any theme possible. So doesn't depend on theme (at least shouldn't). Opposite: themes know about specific of MobileFrontend and customise the html as they need.

In fact the issue is caused by MinervaNeue that specify vertical-align: middle; for img.

Adding the fix to MobileFrontend we let it know the fact that the theme adds some styles to img tags. That's wrong, as MobileFrontend starts depending on the specific theme, and we get bi-directional dependency.

I see at least 2 possible consequences:

  • When Vector or any other theme also decide to use lazy loading, they will face the opposite problem they can't without bringing issue to MinervaNeue.
  • We don't know what side-effect this may cause for themes that are used now in mobile mode.

When Vector or any other theme also decide to use lazy loading, they will face the opposite problem they can't without bringing issue to MinervaNeue

That's the issue I was trying to avoid here. skinStyles solves this issue by allowing skins to theme certain modules provided by other extensions.
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/skin.json#L167

Then I didn't get you correctly.

But what's the purpose of the skinStyles? Can't we just use already loaded less file:

https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.base.styles/images.less#L6

Minerva operates in desktop and mobile mode. You can add to resources/skins.minerva.base.styles/images.less yes but it would mean the styles unnecessarily apply to desktop too.
e.g. https://en.wikipedia.org/wiki/Dog?useskin=minerva as well as https://en.m.wikipedia.org/wiki/Dog

The benefit of styles is you can avoid loading unnecessary styles.

It's actually a bug that those styles live in resources/skins.minerva.base.styles/images.less - they should be skin styles. For a third party who doesn't install MobileFrontend those rules are always unused for example.

That said it's already broken so I think it would be fine to add them to resources/skins.minerva.base.styles/images.less for now with a note at the top of that file saying:

FIXME: Move to skinStyles/mobile.init.styles.less

What's a wonderful world :)

I think (as far as we've found this already) it wouldn't it be good if we solve this now. Unless you see some other risks (say for incompatibility or so).

Change 619724 abandoned by Peter.ovchyn:
[mediawiki/extensions/MobileFrontend@master] DNM: Add vertical-align:middle; to .lazy-image-placeholder class.

Reason:
https://phabricator.wikimedia.org/T199351#6379197

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

Change 620286 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/skins/MinervaNeue@master] Move css styles related to lazy-image-placeholder to skinStyles

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

Peter.ovchyn updated the task description. (Show Details)Aug 14 2020, 7:13 AM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptAug 14 2020, 7:13 AM

Change 620287 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/skins/MinervaNeue@master] Make lazy-image-placeholder vertically aligned middle by default

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

Change 620287 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make lazy-image-placeholder vertically aligned middle by default

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

Edtadros added a comment.EditedAug 18 2020, 2:29 AM

Test Result - Beta

Status: ❌ FAIL
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhone 11 Pro Max

Test Artifact(s):

QA steps

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Flag_images
Scroll quickly to the bottom of the page
❌ AC1: Images load in place without shifting contents
There is some shifting of the images.

Edtadros updated the task description. (Show Details)Aug 18 2020, 2:38 AM

@Jdlrobson it is still shifting, I'm sending it back to Needs More Work and assigning it to you. Please verify that I'm testing this correctly.

Edtadros reassigned this task from Edtadros to Jdlrobson.Aug 18 2020, 2:40 AM

@Jdlrobson it is still shifting, I'm sending it back to Needs More Work and assigning it to you. Please verify that I'm testing this correctly.

Looks like this is something completely different from original requirements, as it shifts text horizontally.
I think there’s another one class or style that should be copied from img to placeholder.

Yeh, I think this has can be considered as passing QA. That's a problem with the beta cluster that I've fixed now [1] if you want to test it again.

[1] https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/445924?diffmode=source

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Aug 18 2020, 5:16 PM

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhone 11 Pro Max

Test Artifact(s):

QA steps

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Flag_images
Scroll quickly to the bottom of the page
✅ AC1: Images load in place without shifting contents

Edtadros updated the task description. (Show Details)Tue, Aug 25, 1:15 AM
Mhurd added a subscriber: Mhurd.Thu, Aug 27, 2:04 AM

Test Result - Prod

Status: ❌ FAIL
Environment: prod/enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhone X
Test Artifact(s):

QA steps

Visit https://en.m.wikipedia.org/wiki/List_of_flags_by_color_combination
Scroll quickly to the first flags, which are just below the opening text.
❌ AC1: When images load the text is shifting vertically a small amount

Visit https://en.m.wikipedia.org/wiki/2018_FIFA_World_Cup
Scroll quickly to the "Teams" section.
❌ AC1: When images load the text is shifting horizontally a small amount


Note: I was a little unsure if this fix made it to prod yet... according to this ReleaseTaggerBot comment T199351#6390485 it looks like it was part of 1.36.0-wmf.5, and when I checked mediaWiki.config.values.wgVersion on prod that's what I got back 🧐🤔

Mhurd claimed this task.Thu, Aug 27, 2:06 AM
Mhurd updated the task description. (Show Details)

@Niedzielski Heya! Did I jump the gun on that ^ prod test?

@Mhurd the code should be live so nope you didn't jump the gun.

Can you please try testing with safemode enabled on the test? We had a few issues with user styles during QA on BC. https://en.m.wikipedia.org/wiki/2018_FIFA_World_Cup?safemode=1 and https://en.m.wikipedia.org/wiki/List_of_flags_by_color_combination?safemode=1

Mhurd added a comment.Thu, Aug 27, 7:19 PM

@Jdlrobson Oh thanks will do!!

Mhurd added a comment.Thu, Aug 27, 9:22 PM

Test Result - Prod

Status: ❌ FAIL
Environment: prod/enwiki (with &safemode=1)
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhone X
Test Artifact(s):

QA steps

Visit https://en.m.wikipedia.org/wiki/List_of_flags_by_color_combination
Scroll quickly to the first flags, which are just below the opening text.
❌ AC1: When images load (with &safemode=1) the text is still shifting vertically a small amount

Visit https://en.m.wikipedia.org/wiki/2018_FIFA_World_Cup
Scroll quickly to the "Teams" section.
❌ AC1: When images load (with &safemode=1) the text is no longer shifting horizontally a small amount, but the placeholders jump vertically a tiny amount once

Mhurd updated the task description. (Show Details)Thu, Aug 27, 9:23 PM
Mhurd added a comment.Thu, Aug 27, 9:26 PM

@Jdlrobson

With safemode there's only a single tiny jitter once on the placeholder images on 2018_FIFA_World_Cup - so it's definitely improved. List_of_flags_by_color_combination however seems to do about the same amount of jitter as the images appear.

I've just realized that List_of_flags_by_color_combination doesn't use lazy loading because the page is too complex and these reflows appear to be present in desktop too, so we can ignore that.

The tiny jitter on 2018 FIFA world cup is passable IMO. Thanks for the thoroughness here.

Mhurd added a comment.Fri, Aug 28, 1:41 AM

yw! sounds good!

Mhurd reassigned this task from Mhurd to ovasileva.Fri, Aug 28, 3:20 AM
Mhurd updated the task description. (Show Details)
Mhurd added a comment.Fri, Aug 28, 3:34 AM

@Niedzielski "Alabama" network throttling profile 🤣

ovasileva closed this task as Resolved.Fri, Aug 28, 11:46 AM

All done, thanks @Mhurd!

Change 618058 abandoned by Phuedx:
[mediawiki/extensions/MobileFrontend@master] DNM: LazyImageTransform: Introduce CssClassList value object

Reason:

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