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

Allow optional disabling of lazy loading images in SSR to improve performance #4297

Closed
jlengstorf opened this issue Mar 1, 2018 · 17 comments
Labels
help wanted Issue with a clear description that the community can help with.

Comments

@jlengstorf
Copy link
Contributor

In #4270 (comment), @KyleAMathews said:

We actually should add a prop that lets you disable lazy loading of certain images during SSR (but would lazy load still in the client) for exactly this reason — if an image loading is the critical path for a page, you could simply specify that image as not-lazy.

I'm capturing this as its own issue in hopes that someone wants to pick it up.

@m-allanson m-allanson added help wanted Issue with a clear description that the community can help with. 🏷 type: feature and removed Help Wanted for Plugins labels Apr 13, 2018
@tbrannam
Copy link
Contributor

I'm looking into issues related to this. Is there a general feeling as to the mechanism for opting-in or opting-out of lazy-loading? (what should the default be)? It seems that a global config is too limiting.

@KyleAMathews
Copy link
Contributor

We want the default to stay lazy loading. This issue is about adding the ability to optionally opt-out of lazy loading by setting a prop in gatsby-image. If you'd like to tackle adding a PR for this that'd be great!

@jlengstorf
Copy link
Contributor Author

@tbrannam @KyleAMathews maybe the API could look something like this?

import React from 'react';
import Image from 'gatsby-image';

export default ({ fluid }) => (
  <div>
    <h1>Hero Section</h1>
    <Image fluid={fluid} critical />
  </div>
);

This would signal that it's a critical image in a human-readable way, allow us to skip lazy-loading in SSR, and allows normal client-side lazy-loading.

Is there a better prop name? Anything additional this feature would need to account for?

@KyleAMathews
Copy link
Contributor

Critical is a user-friendly way to describe it 👍

@tbrannam
Copy link
Contributor

Things are progressing well - but I think there's a related issue regarding webp support.

The default SSR behavior for webp is assume it is not supported. So the non-lazy loaded markup would be default only include a jpeg - and likely switch to webp on re-render. see: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-image/src/index.js#L79

I'm experimenting with <picture> which allows the browser to decide the best file format. It appears to have good coverage with the exception of ie11, and Opera mini - https://caniuse.com/#feat=picture . But it would seem that the fallback markup of <img> inside picture tag would make these fallback to a reasonable experience. Will comment again after verifying against ie11.

Additionally if lazy-loading is skipped - there is no need for a corresponding <noscript> block.

Any objections to this approach?

<div class=" gatsby-image-outer-wrapper" style="position:relative">
    <div class="gatsby-image-wrapper" style="position:relative;overflow:hidden">
        <div style="width:100%;padding-bottom:41.55927835051546%"></div>
        <picture>
            <source type="image/webp" srcset="//example.com/image.webp?w=285 285w,
                //example.com/image.webp?w=270 270w,
                //example.com/image.webp?w=1140 1140w,
                //example.com/image.webp?w=1552 1552w"
                sizes="(max-width: 1140px) 100vw, 1140px">

            <source type="image/jpeg" srcset="//example.com/image.jpeg?w=285 285w,
                //example.com/image.jpeg?w=270 270w,
                //example.com/image.jpeg?w=1140 1140w,
                //example.com/image.jpeg?w=1552 1552w"
                sizes="(max-width: 1140px) 100vw, 1140px">

            <img alt="alt" src="//examples.com/image.jpg?w=1140" srcset="//example.com/image.jpeg?w=285 285w,
                //example.com/image.jpeg?w=270 270w,
                //example.com/image.jpeg?w=1140 1140w,
                //example.com/image.jpeg?w=1552 1552w"
                sizes="(max-width: 1140px) 100vw, 1140px"
                style="position:absolute;top:0;left:0;transition:opacity 0.5s;width:100%;height:100%;object-fit:cover;object-position:center;opacity:1">
        </picture>
    </div>
</div>

@KyleAMathews
Copy link
Contributor

Yeah, that's the right way to do webp — I just skipped figuring out all that since we were lazy loading everything anyways, it was simpler to just do the webp test in the browser :-)

@tbrannam
Copy link
Contributor

tbrannam commented Jul 27, 2018

I've got the baseline working in v2 - but have come across an issue related to the role of placeholders in 'critical' resources. SSR rendered img onLoad are not rendered - which makes sense - but that also means that there would be a dependency on JS to update the loaded state to update the opacity on the placeholders.

I see two potential solutions.

* Change the z-order of critical Gatsby-Image's such that the placeholder images are rendered lower in the z-order than the img tag. Once script kicks in, it can adjust the opacity of the color placeholder? Will need to experiment more with how this looks if using a SVG or DataUri placeholder.
* seems like we'd see 'progressive' loading since it would be visible at all times - and it wouldn't support a nice transition from blur to normal, etc.
The Z-order was already as I thinking, the remaining problem would be that of defaults for opacity

-or -

  • Consider placeholders to be strictly a function of lazy loading and omit them if a Gatsby-Image is marked as critical. If leaning this way - I wonder if there should actually be two component implementations.

@KyleAMathews
Copy link
Contributor

Yeah, having placeholders is still useful for critical images. Ideally we can support them still.

@tbrannam
Copy link
Contributor

Fair enough - although I'm at a loss on how to maintain the cross-fade effect when the images are loaded before the components mount.

@tbrannam
Copy link
Contributor

At best it's going to be a race condition whether the base64 image or background placeholder fades to the loaded image. 'onLoad' won't be called since React isn't live. If it is acceptable to load without the cross-fade I think things will work out.

Is there a supported way to keep React from stripping onLoad from img tags on server render, perhaps some helper script could be injected to manage transitions?

@KyleAMathews
Copy link
Contributor

gatsby-plugin-sharp could definitely inject a little helper script which tracks onLoad for images up until the Gatsby runtime starts. That'd be fairly easy to setup and work nicely.

@tbrannam
Copy link
Contributor

tbrannam commented Aug 7, 2018

It appears that onLoad is stripped by the server-side-renderer. So it's not entirely clear to me how to wire-up a helper script here due to that?

@KyleAMathews
Copy link
Contributor

It'd query all the images in the dom and add onLoad event listeners and once gatsby-image is running, it'd be able to read which images have loaded already.

@tbrannam
Copy link
Contributor

PR #7083 addresses this. using ref I was able to resolve the onLoad issue without a helper script.

DSchau pushed a commit that referenced this issue Sep 11, 2018
Address #4297 

* Add `critical` attribute to opt-out of lazy-loading behavior
* Picture tag to allow browser to select best matching file format (webp detection)
* if fadeIn is true (default) - then the loaded image still will not appear until after the component is mounted

```
	    <Img fixed={images.file1.childImageSharp.fixed} />
	    <Img fixed={images.file1.childImageSharp.fixed} />
	    <Img fixed={images.file2.childImageSharp.fixed} critical fadeIn={false}/>
	    <Img fixed={images.file2.childImageSharp.fixed} critical/>
```
@alexandernanberg
Copy link
Contributor

This can be closed now since #7083 was merged right?

@tbrannam
Copy link
Contributor

tbrannam commented Nov 4, 2018

I would think so

@KyleAMathews
Copy link
Contributor

Yup, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue with a clear description that the community can help with.
Projects
None yet
Development

No branches or pull requests

5 participants