Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch image loading to use srcset #1507

Merged
merged 1 commit into from
Jul 23, 2019
Merged

Switch image loading to use srcset #1507

merged 1 commit into from
Jul 23, 2019

Conversation

bookernath
Copy link
Contributor

@bookernath bookernath commented May 27, 2019

Preface

The philosophy behind modern responsive image thinking is that the browser itself has the best knowledge of how large an image needs to be when it's painting the page, and srcset enables the browser to optimize the image size to download based on its expected size.

As of now, srcset has 94% penetration of browser market share.

This is the first iteration of a solution to allow merchants to use responsive images (srcset) in Cornerstone.

This depends on a new helper: bigcommerce/paper-handlebars#42
And a new stencil-utils method: bigcommerce/stencil-utils#106

What problem are we solving?

The goal is to meet the needs of two different types of shoppers on opposite ends of the performance spectrum:

  • Those on high-resolution displays (such as Retina macs) and good internet connections who want high-quality imagery
  • Those on smaller-screen mobile devices on unreliable or slow connections, who want a faster loading page and can't benefit from high-resolution imagery even when it's available

Historically, merchants have had to choose which shoppers and devices they want to cater to when uploading their image assets - if I uploaded a 4k carousel image to look good on desktop, I'd harm my load speed on mobile devices. If I optimized for mobile load speeds, the image would look bad when stretched out on the desktop.

Our first effort to fix this problem was by enabling Akamai Image Manager for all Stencil storefronts. This CDN feature optimizes file sizes and performs device specific optimizations - sending the best format based on your user agent (for example, .webp images on Chrome) as well as actually sending a smaller image in terms of dimensions if it detects you're on a device that can't display a larger image's full width. For example, a user agent that's 480px wide (according to Akamai's heuristics) would not receive a 4k image, but instead a smaller version closer to its screen size.

This solution was nice because it was "magical" and benefitted everyone immediately, but it caused a few problems:

  • Sometimes images need to be high res, for example product zoom images which are usually blown up larger than the screen size of the device in order to "zoom"
    • To fix this, we created the imbypass=on query string to selectively disable Akamai optimizations on a given image, and we use this for zoom images
  • Edge cases where the browser was expecting specific image dimensions, only to receive something smaller (this issue was usually due to poorly-written CSS that wasn't up to spec with the latest responsive practices)
  • Making a sizing decision based on "user agent screen resolution" is imperfect. For example, I might only need the image to be 25% of the page's width, so downsizing to 100% of its width is under-optimizing
  • In general, Akamai Image Manager takes the control away from the theme developer, where BigCommerce prefers to empower developers to make their own decisions

How are we solving it?

In this PR, I've introduced a srcset component in templates/components/common/responsive-img.html that acts as a drop-in replacement for generating <img> tags with the image objects you'd typically pass to {{getImage}}. Exposing this as a theme component (instead of a handlebars helper) allows it be configurable by theme developers, which may be preferable for something this complex. This component aims to absorb the complexity of generating a srcset in such a way that it's compatible with legacy browsers and also lazy-loadable.

Cornerstone already makes use of @aFarkas's lazysizes module for lazyloading. As the default behavior, I've implemented the pattern described here to allow srcset-capable browsers to get a low-quality placeholder image while the full resolution image is downloaded in the background. Via theme configuration, you can also do standard lazyloading (with no placeholder) via this pattern. Both of these methods have graceful backwards compatibility for browsers that do not support srcset such as IE11.

Things left to do

  • Get feedback from the developer community (hence this PR!)
  • LOTS of testing. This is a fundamental change to how Cornerstone works, and we need to make sure we're both accomplishing our goals and not breaking anything
    • Performance testing (browser) - are we making the page load slower in our test cases? If so, we need to re-evaluate the approach.
    • Performance testing (server) - we'll be generating a lot more versions of images using this approach, and we need to make sure we can support it on infrastructure side.
    • Legacy browser testing - we still need to support IE11, let's make sure the experience there is reasonable.
    • Visual regression testing - Need to test across many browsers and screen resolutions to make sure this doesn't cause crappy styling anywhere.
  • Allow images to be upsized, not only downsized - DONE!
    • This is necessary so we can generate srcsets "in the blind" without know the sizes of the original images
    • As an alternative, we could do work elsewhere to understand the size of images at runtime and thus be more intelligent with srcset generation. This might be a larger project, so perhaps it will come later.
  • Create a means of getting images which are optimized by Akamai in terms of file size and best-for-device file type, but NOT actually resized by Akamai.
    • srcset is intended to allow the browser to take control of resizing, so Akamai's features will interfere with this when active
  • Figure out the best image sizes to use. Done!

Demo stores

https://srcset-test.mybigcommerce.com/
https://srcset-test-lqip.mybigcommerce.com/

Markup Before/After

Example is from the first product card image on https://cornerstone-light-demo.mybigcommerce.com/

Cornerstone 3.4.4 (before srcset):

Raw HTML in document:
<img class="card-image lazyload" data-sizes="auto" src="https://cdn11.bigcommerce.com/s-n0i50vy/stencil/35380e80-6482-0137-8118-0242ac110010/e/369fc3d0-61ff-0137-5424-0242ac110007/img/loading.svg" data-src="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/500x659/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2" alt="" title="">

After lazysizes evaluates:
<img class="card-image lazyautosizes lazyloaded" data-sizes="auto" src="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/500x659/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2" data-src="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/500x659/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2" alt="" title="" sizes="263px">

Cornerstone with srcset:

Raw HTML in document:
<img src="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/640x640/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2" alt="" title="" data-sizes="auto" srcset="data:image/gif;base64,R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==" data-srcset="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1280x1280/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1280w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/160x160/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 160w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1920x1920/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1920w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/2560x2560/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 2560w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/320x320/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 320w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/640x640/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 640w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/80x80/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 80w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/960x960/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 960w, " class="lazyload card-image" loading="lazy" />

After lazysizes evaluates:
<img src="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/640x640/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2" alt="" title="" data-sizes="auto" srcset="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1280x1280/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1280w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/160x160/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 160w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1920x1920/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1920w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/2560x2560/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 2560w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/320x320/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 320w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/640x640/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 640w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/80x80/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 80w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/960x960/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 960w, " data-srcset="https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1280x1280/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1280w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/160x160/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 160w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/1920x1920/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 1920w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/2560x2560/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 2560w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/320x320/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 320w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/640x640/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 640w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/80x80/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 80w, https://cdn11.bigcommerce.com/s-n0i50vy/images/stencil/960x960/products/86/286/ablebrewingsystem4_1024x1024__07155.1456436672.jpg?c=2 960w, " class="card-image lazyautosizes lazyloaded" loading="lazy" sizes="263px">

Tickets / Documentation

ping @bigcommerce/storefront-team @bigcommerce/frontend @chrisboulton

@bigbot
Copy link

bigbot commented May 27, 2019

Autotagging @bigcommerce/storefront-team @davidchin

@jsimm-guidance
Copy link

This is good thinking. I would like to see some before/after output of the markup generated.

How does the product admin support the theme developer w/ multiple images? Will someone maintaining the product upload multiple resolutions each image, or is the assumption still one resolution per image?

@bookernath
Copy link
Contributor Author

bookernath commented May 30, 2019

Thanks @jsimm-guidance I've added some markup examples, both before and after Lazysizes JS evaluates.

In terms of the merchant management of images, our guidance is to simply upload the highest-resolution imagery you have in the control panel. From there, we can generate all the resized versions for srcset - it's best if we're just generating smaller versions from a high-quality original.

Copy link
Contributor

@davidchin davidchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bookernath nice work man, it's looking good!

package.json Show resolved Hide resolved
templates/components/common/responsive-img.html Outdated Show resolved Hide resolved
@bookernath bookernath changed the title [RFC] feat(storefront): STRF-3251 Switch image loading to use srcset [RFC] Switch image loading to use srcset Jul 8, 2019
@@ -49,7 +49,7 @@
.card-image {
@include lazy-loaded-img;
border: 0;
width: auto;
width: 100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more explicit CSS sizing is helpful for lazysizes - this prevents page flickering.

@@ -1,7 +1,3 @@
.lazyload, .lazyloading {
height:100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this as it was causing flicker during lazyloading

@@ -1313,7 +1341,7 @@
},
{
"type": "checkbox",
"label": "Show &quot;Shop by Price&quot; in filters",
"label": "Show \"Shop by Price\" in filters",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fixing this while I was in here, I think it's easier to read.

@@ -21,14 +21,14 @@ <h5 class="account-listShipping-title">{{lang 'account.orders.details.ship_to_mu
</div>
<figure class="account-product-figure">
{{#if type '===' 'giftcertificate'}}
<img class="lazyload" data-sizes="auto" src="{{cdn 'img/loading.svg'}}" data-src="{{cdn ../../theme_settings.default_image_gift_certificate}}" alt="Gift Certificate" title="Gift Certificate">
<img src="{{cdn ../../theme_settings.default_image_gift_certificate}}" alt="Gift Certificate" title="Gift Certificate">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this image doesn't need lazyloading.

fallback_size='1280w'
lazyload='lazyload'
}}
{{/if}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was necessary to split this because I want the first carousel slide to never lazyload, but the 2nd and beyond slides to always lazyload. I couldn't figure out how to do that with a nested handlebars expression, and this was easier to read anyways.

{{> components/common/responsive-img
image=image
class="cart-item-image"
lazyload='disabled'
Copy link
Contributor Author

@bookernath bookernath Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once you're far enough down the purchase funnel to be looking at the cart page, it's better to show images ASAP than to optimize the load speed of the page further. There's a decent chance you already have the image in cache as well, since you viewed the product and/or category page.

{{> components/common/responsive-img
image=image
class="productOptions-list-item-image"
lazyload='lazyload+lqip'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm forcing lazyloading on these because the entire structure of the product list element gets significantly modified by JS after the page loads, so there's no need to load these immediately - it's better to wait for the sizing to settle so the browser can pick the right size.

window.lazySizesConfig = window.lazySizesConfig || {};
window.lazySizesConfig.loadMode = 1;
</script>
<script async src="{{cdn 'assets/dist/theme-bundle.head_async.js'}}"></script>
Copy link
Contributor Author

@bookernath bookernath Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, javascript in the head?!?!

Yes - we need this to make sure images actually show up on the page, so this few KB file is mission-critical.

I've marked it as async so it won't block render, but we want it to execute as soon as it has downloaded.

Lighthouse doesn't complain about it.

@@ -12,7 +12,9 @@
{{/partial}}
{{#partial "amp-scripts"}}
<script async custom-element="amp-accordion" src="https://cdn.ampproject.org/v0/amp-accordion-0.1.js"></script>
<script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js"></script>
{{#any product.videos product.images}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing another AMP validation error

<img class="lazyload" data-sizes="auto" src="{{cdn 'img/loading.svg'}}" data-src="{{getImage brand.image 'thumb_size'}}">
{{> components/common/responsive-img
image=brand.image
lazyload='disabled'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brand image is critical above-the-fold content on the brand page, so not lazyloading this.

@@ -10,6 +10,7 @@ module.exports = {
context: __dirname,
entry: {
main: './assets/js/app.js',
head_async: ['lazysizes'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading lazysizes module in the head script. This file should only be used for mission-critical head JS.

@mattolson
Copy link
Contributor

Do we have a sense of how much this will inflate the overall HTML document size for the average site?

@bookernath
Copy link
Contributor Author

@mattolson About 10kb for a PLP like a category page.

Here's Cornerstone 3.5.1 on a category page with 12 products:

image

Here's this branch:

image

As you can see, it gzips quite well as to make the over-the-wire difference inconsequential.

Copy link
Contributor

@mattolson mattolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. My only concern is around the hard coding of sizes throughout the theme and losing the current configuration. Do we want to continue to allow the merchant to configure certain things (like the zoom image size) or should this be entirely handled by the theme now?

templates/components/products/product-view.html Outdated Show resolved Hide resolved
templates/components/amp/products/list-item.html Outdated Show resolved Hide resolved
assets/js/theme/common/product-details.js Outdated Show resolved Hide resolved
@bookernath
Copy link
Contributor Author

bookernath commented Jul 17, 2019

@mattolson I didn't get rid of any of the sizes, and in most cases they still affect the sizing of the images via CSS; they're simply decoupled from the pixel dimensions of the image, because with srcset, that's less important. We're not going for a perfect match between CSS sizing and pixel sizing with this methodology.

EDIT: I kept the image sizes tied to config for zoom images and fallback images on PDPs. This increases the total number of sizes we have to deal with, but it might be safer overall.

@bookernath
Copy link
Contributor Author

In the interest of reducing the changeset (and thus the risk), I've removed any updates to AMP templates.

templates/pages/category.html Outdated Show resolved Hide resolved
templates/pages/brands.html Outdated Show resolved Hide resolved
templates/components/products/modals/writeReview.html Outdated Show resolved Hide resolved
templates/components/products/list-item.html Outdated Show resolved Hide resolved
templates/components/products/card.html Outdated Show resolved Hide resolved
templates/components/common/cart-preview.html Outdated Show resolved Hide resolved
@@ -183,11 +190,16 @@ <h2 class="productView-brand"{{#if schema}} itemprop="brand" itemscope itemtype=
<li class="productView-thumbnail">
<a
class="productView-thumbnail-link"
href="{{getImage this 'product_size' (cdn ../theme_settings.default_image_product)}}"
href="{{getImageSrcset this (cdn ../theme_settings.default_image_product) 1x=../theme_settings.zoom_size}}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

href in case you open in new tab should return a large image.

mattolson
mattolson previously approved these changes Jul 23, 2019
Copy link
Contributor

@mattolson mattolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for taking this on, this is a huge improvement for page speed!

@bookernath bookernath changed the title [RFC] Switch image loading to use srcset Switch image loading to use srcset Jul 23, 2019
@bookernath bookernath merged commit e047884 into bigcommerce:master Jul 23, 2019
@bookernath bookernath deleted the lazysizes-srcset branch July 23, 2019 19:31
@VH-WRK
Copy link

VH-WRK commented Aug 16, 2019

@bookernath This was a brilliant idea!
I'm struggling to implement this correctly for my store. I'm using an edited version of cornerstone and the problem lies in our images taking on different aspect ratios because we sell books and they come in landscape and portrait.
What happens is our portrait images get stretched as a result of width: 100%; change in assets/scss/components/citadel/cards/_cards.scss
For .card-image we had both width and height set to auto and max-width and max-height set to 100%. This allowed the src attribute create the correct size and aspect ratio and the image would fill the image container.
Now the image loaded is the default lqip_size of width 80px. Is there away around this?

@bookernath
Copy link
Contributor Author

@VH-WRK that was a bug with this PR actually. It's been fixed in #1559 if you want to pull in that fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants