Images Appear

[supernova-at]

Description

This PR updates some CSS handling in our Image component to no longer apply display: none to images that we think haven't loaded yet, since Chromium (as of version 80) is no longer requesting those images.

Instead, we set visibility: hidden. Chromium still sends off the request and nothing else downstream needs to change.

Related Issue

Closes PWA-367.

Acceptance

Verification Stakeholders

@jimbo

Specification

Verification Steps

Venia

1. yarn watch:venia
2. Navigate to /pomona-skirt.html
3. See that all images load correctly
4. Verify that all images across the entire Venia site load correctly

Storybook

1. yarn workspace @magento/venia-ui run storybook
2. Verify that all the stories in the Image component load correctly

Screenshots / Screen Captures (if appropriate)

Checklist

• I have updated the documentation accordingly, if necessary.
• I have added tests to cover my changes, if necessary.

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-367.

Generated by 🚫 dangerJS against 5025145

[supernova-at]

Well, I'm not sure that this is the way to go anymore. For one thing, the PlaceholderImage still uses the isLoaded flag, which isn't getting set.

And then watch as the load event doesn't fire until I alter a CSS property:

[sirugh]

I wonder if display:none is being used as an optimization to not load the image 😕

[supernova-at]

I wonder if display:none is being used as an optimization to not load the image 😕

Good thought. This article is dated June 2018 but I found

May be worth trying again in Chrome 80 since we didn't see this bug until then.

[supernova-at]

Yep, I think you're right: https://codesandbox.io/s/nervous-minsky-rwqz1

There's an image with loading="lazy" and display: none that is out of the viewport initially. When you scroll down to it, the request never fires in the network tab.

When you remove the display: none styling, you can see it fire the request for the image.

[supernova-at]

So I'm pretty confident we can fix that in our code but the question becomes why does Chrome think our Logo and primary product image are out of the viewport initially - they are most definitely not.

[brendanfalkowski]

I'm wondering if Chromium sees the lazy image occupying the same physical space as the eager image so it chooses not to request the lazy image.

Is there a reason you need two images with different loading attribute values? I would probably have started with something like:

<div class="wrapperWhichSetsThePlaceholderAsBackgroundImage">
    <img loading="lazy" src="xyz" />
</div>

becomes

<div class="wrapperWithNoBackgroundImage">
    <img loading="lazy" src="xyz" />
</div>

The placeholder background-image would be assigned by CSS, and would be guaranteed to load plus be cached to render instantly later. You'd still need to get the "loaded" event properly to change the class, and that seems to be complicating.

I would simplify and not have a loading image. There's a lot happening to support that, but I'd think it's really only important to anchor the physical space. Whether that's a filled by a solid, transparent, or image is secondary / depends on the design.

Aside: isn't base64 decoding a PNG bad for performance on most devices? I recall research that it was always better to take the HTTP hit and have the physical file in cache. Even for an SVG there was a limit to the gain of inlining the markup vs referencing a file source. If the SVG itself was complex the DOM size bloat from repetition was again worse than the one-time HTTP hit.

[supernova-at]

the question becomes why does Chrome think our Logo and primary product image are out of the viewport initially - they are most definitely not.

Update: editing that sandbox, it doesn't matter whether the images are in the viewport initially or not. If they have display: none, Chromium v80 is not requesting them.

So THEN the question becomes why do some of the images work just fine and others don't? 🤪

Edit: Chromium only requests the 2KB placeholder image to approximate dimensions if dimensions aren't already explicitly set. This explains why this affected images (like the Logo) who had their dimensions explicitly set: the request for the placeholder image never took place.

[supernova-at]

PR Updated:

• Changes to using visibility: hidden instead of display: none to "hide" the image until it is loaded

Chromium@80 does request images that are visibility: hidden, so I think this is the smallest code change we can make that will fully solve the problem.

Speaking of small code changes, since this is a Priority 1 I'd like to keep the scope tight and separate out the other suggested improvements to Image into their own tickets.

[sirugh]

Oof. Well at least we fixed it :o

[dpatil-magento]

@supernova-at Not sure if its related to your changes but somehow its happening on this PR. On Win10-Edge - Home page image does not load. Same works if I test on develop

Also same I could reproduce on Safari browser. Whenever Response Header Content-Type: image/webp image loads blank.

[jimbo]

Is there a reason you need two images with different loading attribute values? I would probably have started with something like:

<div class="wrapperWhichSetsThePlaceholderAsBackgroundImage">
    <img loading="lazy" src="xyz" />
</div>

becomes

<div class="wrapperWithNoBackgroundImage">
    <img loading="lazy" src="xyz" />
</div>

The placeholder background-image would be assigned by CSS, and would be guaranteed to load plus be cached to render instantly later. You'd still need to get the "loaded" event properly to change the class, and that seems to be complicating.

I would simplify and not have a loading image. There's a lot happening to support that, but I'd think it's really only important to anchor the physical space. Whether that's a filled by a solid, transparent, or image is secondary / depends on the design.

The reasoning behind the technique used here is that the layout behavior of <img> (namely, preserving aspect ratio while scaling) can't be accomplished with CSS unless the stylesheet is told the aspect ratio. Since we don't tell the CSS what aspect ratio our images are, we use a transparent image to allow the container to perform layout while the image loads. And since <img> has that cool scaling, we can use a very tiny placeholder (4x5).

However...

Since we only use a single, fixed-size placeholder, we're not really gaining anything with this technique. If it's fixed, then the CSS might as well know the size, and we have precedent for doing so in other places by letting the component pass sizes to the stylesheet via CSS variables. Worth a PR in the future probably.

Aside: isn't base64 decoding a PNG bad for performance on most devices? I recall research that it was always better to take the HTTP hit and have the physical file in cache. Even for an SVG there was a limit to the gain of inlining the markup vs referencing a file source. If the SVG itself was complex the DOM size bloat from repetition was again worse than the one-time HTTP hit.

I doubt it's that bad for a tiny PNG (4x5). At the time, though, we did this for expediency.

[jimbo]

Approving this PR, but for posterity, I have to note here that the way this was done (originally) was not the recommended approach.

• the styles on .loaded should be on .image
• .loaded should compose .image
• .notLoaded should not compose .loaded
• . notLoaded should compose .image and override visibility
• (most importantly) .image should not be applied directly to the component

[brendanfalkowski]

@jimbo That makes sense.

FWIW, we might go simpler and have <Image> be dimensionless letting the browser figure it out (or pass params), and split a separate <ProductImage> component that has a fixed aspect ratio + associated styling.

I've never worked on a Magento site that didn't have a standard aspect ratio for product images as part of product data acceptance (overwhelmingly 1:1 but the fashion-torso prevails).

There are cases we carve a recess for <Image> with padding on a parent and absolutely position it within so repaint doesn't affect reflow, but the vast majority of image declarations would be <ProductImage> with a hardcoded scale.

[dpatil-magento]

Chrome, Firefox, Win10-latest Edge, Android Chrome loads images fine.
We are tracking intermittent Safari issue with Fastly team.