Page MenuHomePhabricator

Images with dimensions expressed via styles are being loaded even when hidden
Closed, ResolvedPublic3 Story Points

Description

If an image has no width and height attribute and is inside a collapsed section, they will be loaded unexpectedly (as identified in T144734)

This is due to an erroneous fix in T143768 which loads any images without a height.

Instead we should copy the dimensions from the style attribute to the element so that a placeholder element cannot be without dimensions.


Test Plan

Repeat the test with a Grade A and C browsers, e.g. Safari for iOS on an iPhone 6 and Opera Mini respectively.

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptSep 9 2016, 5:51 PM

Change 309409 had a related patch set uploaded (by Jdlrobson):
Copy height and width attributes from style to lazy placeholder

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

ori moved this task from Inbox to Radar on the Performance-Team board.Sep 12 2016, 6:07 PM

I'm out tomorrow at an all day Wikimedia event. If there's any issues with the patch please take it over. We should aim to get this live by next week so it's not reported post blog post.

I'm out tomorrow at an all day Wikimedia event. If there's any issues with the patch please take it over. We should aim to get this live by next week so it's not reported post blog post.

Delaying publishing a blog post is less risky than rushing a fix into production. We'll do what we can 🙂

phuedx claimed this task.Sep 14 2016, 1:15 PM

There's a minor bug in rEMFR0b0fc2a2bca9: Copy height and width attributes from style to lazy placeholder where img elements like

<img src="..." alt="Foo" />

will be transformed to

<img src="..." alt="Foo" style="width: px; height: px" />

There's a minor bug in rEMFR0b0fc2a2bca9: Copy height and width attributes from style to lazy placeholder where img elements like

<img src="..." alt="Foo" />

...

As I've noted in my review, this is the case for all of the images generated by Math on our staging server.

Thanks Sam for the catch and fix up!

We need to take the time to get the staging server using Mathoid to render equations. I'm also a little concerned that, by default, equations are rendered without dimensions.

phuedx reassigned this task from phuedx to Jdlrobson.Sep 16 2016, 5:26 PM

Un-licking this cookie for now…

There's not been much activity on this since Wednesday where Sam identified a bug that is now fixed. We continue to load these images unnecessarily and I'm keen to rectify this asap given a blog post goes out today drawing more attention to this work.

Can someone explicitly state what is blocking them from reviewing this and how concretely I can help? Right now I'm a little lost.

I've enabled the math role on the staging server and configured it to use the Mathoid server available at formulasearchengine.com.

rEMFR32a9ad20befc: Get height and width from style to placeholder is working as expected.

Unfortunately, rEMFR33689eccda81: Treat list items as inline causes placeholders for equations to have display: inline applied to them, leaving the width and height rules ignored, and, consequently, untouchable by the lazy image loader.

Consider the markup for "inline" and "block" equations:

Inline
<span>

  <span class="mwe-math-mathml-inline mwe-math-mathml-a11y" style="display: none;">
    <math xmlns="http://www.w3.org/1998/Math/MathML"><!-- ... --></math>
  </span>
  <noscript><!-- ... --></noscript>
  <span class="lazy-image-placeholder" style="width: 4.679ex;height: 2.509ex;" data-src="https://api.formulasearchengine.com/v1/media/math/render/svg/957584ae6a35f9edf293cb486d7436fb5b75e803" data-alt="{\displaystyle \lambda _{\max }}" data-class="mwe-math-fallback-image-inline"></span>

</span>
Block
<dl>
  <dd>
    <span>

      <span class="mwe-math-mathml-inline mwe-math-mathml-a11y" style="display: none;">
        <math xmlns="http://www.w3.org/1998/Math/MathML"><!-- ... --></math>
      </span>
      <noscript><!-- ... --></noscript>
      <span class="lazy-image-placeholder" style="width: 10.271ex;height: 5.343ex;" data-src="https://api.formulasearchengine.com/v1/media/math/render/svg/c7864b3550dd5ccca92ae55314e0b00c9bacc3e0" data-alt="{\displaystyle \lambda _{\max }={\frac {b}{T}}}" data-class="mwe-math-fallback-image-inline"></span>

    </span>
  </dd>
</dl>

In both cases the span .lazy-image-placeholder applies.

Sam which article are you testing this on? I can't find any here - http://reading-web-staging.wmflabs.org/wiki/Special:Contributions/Phuedx

This comment was removed by phuedx.

@Jdlrobson, re: https://phabricator.wikimedia.org/T144567#2650579 - what were you thinking in terms of exploratory testing?

phuedx added a comment.EditedSep 21 2016, 12:20 PM

@Jdlrobson: I've submitted a new patchset of rEMFRa698ca513786: Get height and width from style to placeholder, which uses DOMDocument#createEntityReference as you suggested in your comments and I've tested it locally using an almost identical setup to our staging server's.

@phuedx I'm not seeing the nbsp character in the raw HTML for http://reading-web-staging.wmflabs.org/w/index.php?title=Black-body_radiation&mobileaction=toggle_view_mobile nor locally. This was the issue I was having with using createEntityReference

Is it being encoded by a unicode character and is that why it has been confusing me?

@phuedx seems it's not unicode and it's not working because of the above.
http://reading-web-staging.wmflabs.org/w/index.php?title=Black-body_radiation&mobileaction=toggle_view_mobile
In the section under the heading "Virtual temperature of Earth" - no images will load.

This is why I was resorting to JS there is something weird going on in the formatter.

Patch has +1s from @phuedx @jhobs and myself. Jenkins failures are due to the module ready bug that's also in flight, but we should be able to merge regardless since those are intermittent. Anyone care to hit the +2 button?

Patch has +1s from @phuedx @jhobs and myself. Jenkins failures are due to the module ready bug that's also in flight, but we should be able to merge regardless since those are intermittent. Anyone care to hit the +2 button?

/cc @bmansurov @Jhernandez

Change 309409 merged by jenkins-bot:
Get height and width from style to placeholder

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

I'm moving this into Needs QA as rEMFR1ac2bf5688fc: Get height and width from style to placeholder affects a user-facing feature. While the change will need to be signed off by a developer (@Jhernandez), I think it's worth asking TSG to take a look at the lazily-loaded images feature beforehand.

ovasileva added a comment.EditedSep 22 2016, 1:02 PM

@phuedx, @Jdlrobson - testing criteria? (or maybe just lazy-loading exploratory testing criteria?)

phuedx updated the task description. (Show Details)Sep 22 2016, 1:12 PM

Device/app specs:

  1. iOS 10.0.1, iPhone 6s, app 5.2.1 (942) - Safari
  2. Galaxy Express 3 (Android 6.0.1), app 2.4.156-alpha-2016-09-21 - Chrome

All images load in the Black-body_radiation article - the inline equations take longer than the other images to load but they eventually do load within a few seconds of being scrolled at in the article.

ovasileva closed this task as Resolved.Sep 23 2016, 3:49 PM