Page MenuHomePhabricator

Lazy loaded images: Styles are not copied across from img tags when an image is lazy loaded
Open, NormalPublic5 Story Points

Description

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

The page WP:EIS#Vertical_alignment has some examples of the vertical-align parameter for images. They don't appear to work on Minerva; the image tag has class="image-lazy-loaded" and no vertical-align in the style parameter. Copying and pasting that section's code into the sandbox (old revision) works however; the images have no lazy-loading class and style="vertical-align: text-top" (or whatever was requested).

I don't have a reliable way to force lazy-loading of images on and off and so guarantee that it is the cause of the problem but it seems to be the cause; I did some further similar tests and the pattern persisted.

(Edit: references to MinervaNeue removed as the same bug can be achieved with the Monobook or Vector skin when using URLs like https://en.m.wikipedia.org/wiki/Wikipedia:Extended_image_syntax?useskin=monobook.)

Developer notes

Style attribute does not get copied across when an image gets lazy loaded.

The problematic function is inside includes/transforms/LazyImageTransform.php (doRewriteImagesForLazyLoading)

If the style is copied across, with the width and height appended at the end all should be good

                        $style = $img->getAttribute( 'style' ) || '';
			$imgPlaceholder->setAttribute( 'style', $style . $dimensionsStyle );

Appending at the end is important as we don't use an img tag for the placeholder, so if width and height attributes are used we need to retain them if they exist.

If a style width/height is already defined we will define twice which is harmless.

There is obviously risk in copying styles meant for an img tag across to a placeholder, so we may also want to consider having an attribute whitelist to make sure we only copy across ones we know to be harmless e.g. vertical-align.

Replication

Create the following local content

Text
== Lazy section == 
<span style="break-inside:avoid-column;"><code>baseline</code>:<br /> [[File:Flag of Hungary vertical.svg|baseline|frameless|upright=0.1|link=|alt=]]Align the bottom of the image with the [[Baseline (typography)|baseline]] of the text.</span>

Open the browser in mobile emulator mode (with sections collapsed).
Drop connection to super slow and expand the section.

Event Timeline

GKFX created this task.Oct 25 2018, 8:28 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 25 2018, 8:28 AM
Jdlrobson added a subscriber: Jdlrobson.

you can disable lazy loading images by viewing Minerva in desktop mode.
https://en.wikipedia.org/wiki/Wikipedia:Extended_image_syntax?useskin=minerva

I"m seeing vertical alignment when I view in Minerva so could you clarify what you mean by "don't appear to work" ?

GKFX added a comment.Oct 25 2018, 7:25 PM

This is working: the flags have different vertical alignments (using the URL Jdlrobson supplied). The HTML of the image tag (taken with Firefox developer tools' Copy > Outer HTML) is

<img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/20px-Flag_of_Hungary_vertical.svg.png" style="vertical-align: baseline" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/30px-Flag_of_Hungary_vertical.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/40px-Flag_of_Hungary_vertical.svg.png 2x" data-file-width="600" data-file-height="1200" width="20" height="40">

This is not working: the flags all are at vertical-align: middle, which is the default set by a stylesheet. URL is the one I supplied in my original comment, https://en.m.wikipedia.org/wiki/Wikipedia:Extended_image_syntax. The HTML of the image tag is now

<img src="//upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/20px-Flag_of_Hungary_vertical.svg.png" alt="" style="width: 20px;height: 40px;" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/30px-Flag_of_Hungary_vertical.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/d/d4/Flag_of_Hungary_vertical.svg/40px-Flag_of_Hungary_vertical.svg.png 2x" class="image-lazy-loaded" width="20" height="40">

GKFX renamed this task from Lazy-loaded images in MinervaNeue skin have vertical-align parameter ignored to Lazy-loaded images have vertical-align parameter ignored.Oct 25 2018, 7:39 PM
GKFX removed a project: MinervaNeue.
GKFX updated the task description. (Show Details)
Jdlrobson renamed this task from Lazy-loaded images have vertical-align parameter ignored to Styles are not copied across from img tags when an image is lazy loaded.Oct 26 2018, 12:15 AM
Jdlrobson updated the task description. (Show Details)
ovasileva triaged this task as Normal priority.Nov 6 2018, 3:56 PM
Jdlrobson renamed this task from Styles are not copied across from img tags when an image is lazy loaded to Lazy loaded images: Styles are not copied across from img tags when an image is lazy loaded.Nov 9 2018, 9:07 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added subscribers: Pkra, TheDJ, Physikerwelt.
Jdlrobson updated the task description. (Show Details)Nov 9 2018, 9:09 PM
Debenben moved this task from Incoming to Blocked: needs help on the Math board.Nov 11 2018, 8:18 PM
Jdlrobson set the point value for this task to 5.Nov 21 2018, 5:54 PM

We estimated this as a 5 as although the code is well tested and the code change simple, we anticipate lots of testing and associated risk about the fact we are blindly copying across styles. There may be certain styles that interfere with our lazy image loading code.

Pkra added a comment.Nov 22 2018, 10:12 AM

we anticipate lots of testing and associated risk about the fact we are blindly copying across styles. There may be certain styles that interfere with our lazy image loading code.

With T209148 in mind, could a first step be an attribute whitelist?

Or with T209148 in mind, why not disable lazy loading for math "images" completely? After all they are not images, but part of the text.

Or with T209148 in mind, why not disable lazy loading for math "images" completely? After all they are not images, but part of the text.

Given all img tags are just raw HTML it's tricky to determine if an image is a Math image or something else, and we'd want a general solution. We do have a way to disable lazy loading on smaller images, but this is likely to have an impact on performance and we haven't had the resources to investigate this. The problem is tha although they look like text, they are images and part of the HTML so are treated like every other image.

I think the proposal to copy styles across should resolve this problem and also fix a bunch of other unrelated to Maths bugs in the process so is worth the investment in time.

Pkra added a comment.Dec 13 2018, 8:41 PM

@Jdlrobson I did a quick check and I'm reasonably certain that math images will only have

  • width
  • height
  • vertical-align
  • margin-top/left/right/bottom

(Once again, this is a bug that would disappear if SVGs were inlined.)

Once again, this is a bug that would disappear if SVGs were inlined.

Not sure I understand this - the SVGs are display: inline even when lazy loaded. Can you clarify what you mean by this?

Pkra added a comment.EditedDec 13 2018, 9:52 PM

Not sure I understand this - the SVGs are display: inline even when lazy loaded. Can you clarify what you mean by this?

Sorry for causing confusion! With "inlining" I meant inlining the svg code in the html code (as in: have svg markup in the html, not img tags pointing to an svg file).

I hope that makes more sense.

Oh got it! thanks for the clarification @Pkra . I could have sworn there was a bug to do this, but can't find this anywhere.

Pkra added a comment.Dec 13 2018, 10:52 PM

Oh got it!

Thanks for your patience.

I could have sworn there was a bug to do this, but can't find this anywhere.

There isn't one I think.
IIUC when the svg rendering was introduced, it was rejected early on (when pure mathml on firefox was considered). It would be good to know if this could be revisited. I think the advantages of inlined svg markup outweigh the disadvantages (i.e., caching).

TheDJ added a comment.EditedDec 14 2018, 9:52 AM

There isn't one I think.
IIUC when the svg rendering was introduced, it was rejected early on (when pure mathml on firefox was considered).

I can answer that. The reason for that was largely that correct client side math rendering requires web fonts in order to have any sorts of predictable quality. And the web math fonts were rather problematic for a long while. Mostly this had to do with their massive download size. Another big problem was Flash of Unstyled Text. With the new font-display, this should be slightly better, but support is still weak. We also had a lot of trouble getting accurate client side detection of which browsers supported SVG and which didn't complicating the fallback chain to where it was hard to maintain (should be less of a priority now I think). Last reason was that the foundation had some BAD webfont experiences the last time they dipped into that.

Someone should attempt to write a RL module to deliver MathJax'es web fonts and see what we can achieve there. Should probably be a separate ticket.

Pkra added a comment.Dec 14 2018, 11:43 AM

The reason for that was largely that correct client side math rendering requires web fonts in order to have any sorts of predictable quality.

Sorry for causing a few things to be mixed up.

My suggestion was to revisit the decision that equation SVGs are delivered via img tags instead of inlining them in the HTML.

I think using the equation SVGs remains the best solution for the forseeable future (if not ever). Of course those equation SVGs do not need any webfonts.

@TheDJ, I think you're referring to the discussion around using native MathML in Gecko, correct? Both Gecko and WebKit's implementations also need webfonts from a practical point of view: those implementations really only work with fonts that have OpenType Math tables and only Windows ships such a font (to make things worse, only a handful such fonts exist and all of which are very large).

Of course, MathJax's CSS-based output would also require webfonts, albeit a set of fairly small ones.

Anyway, I hope that helps clarify what I meant earlier.

Pkra added a comment.Dec 14 2018, 11:52 AM

Ok, since this has gone off thread anyway, let me make a quick case for using svg markup inlined in html:

IIRC inlined-svg was rejected to reduce the page payload because, back then, a meta tag hack was used to prevent the browser from loading the img source if MathML was used for visual rendering.

Nowadays, neither the meta tag hack nor the option for using MathML for visual layout are still around, so that argument seems no longer valid. The main remaining advantage is browser caching of img sources across pages. I suspect the question would be if that performance benefit is large enough in practice to outweigh the rendering issues caused by using img tags.

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)