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

Add support for native lazy loading to gatsby-image #13201

Closed
KyleAMathews opened this issue Apr 7, 2019 · 20 comments · Fixed by #13217
Closed

Add support for native lazy loading to gatsby-image #13201

KyleAMathews opened this issue Apr 7, 2019 · 20 comments · Fixed by #13217
Assignees

Comments

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Apr 7, 2019

https://twitter.com/addyosmani/status/1114777583302799360

We'll want to feature detect this using the code in Addy's blog post:

if ('loading' in HTMLImageElement.prototype) { 
    // Browser supports `loading`, don't setup IntersectionObserver
} else {
   // Use existing IntersectionObserver code
}

If lazy-loading exists, then we'll want to use it, if not, we'll continue to use our IntersectionObserver setup.

For images using the critical prop, we should use loading="eager" and for other images, we should use loading="lazy" — this should replicate our existing semantics.

<!-- Lazy-load an offscreen image when the user scrolls near it -->
<img src="unicorn.jpg" loading="lazy" alt=".."/>

<!-- Load an image right away instead of lazy-loading -->
<img src="unicorn.jpg" loading="eager" alt=".."/>
@KyleAMathews KyleAMathews added the help wanted Issue with a clear description that the community can help with. label Apr 7, 2019
@addyosmani
Copy link
Contributor

Excited to see the loading attribute being considered. Initially, those mappings for lazy vs eager LGTM.

@andrewfairlie
Copy link
Contributor

I would like to have a go at this, it would be my first contribution though so I'm unsure what the procedure is in taking an issue?

@sidharthachatterjee
Copy link
Contributor

Hey @andrewfairlie

Thank you for offering to take this up! We really appreciate it and this would be an awesome first contribution to Gatsby. Would you like to pair on this sometime tomorrow and get this in?

@andrewfairlie
Copy link
Contributor

andrewfairlie commented Apr 7, 2019

@siddharthkp 👍my schedule's a little wonky this week with a fair bit of travel. If it's cool with you I'll fork this and attempt when I have an hour or two and if I don't make much progress could we then arrange a pair session to get me on the right lines?

Really appreciate the offer of pairing, very cool! 👍

@KyleAMathews
Copy link
Contributor Author

@andrewfairlie we want to get this in pretty quickly so if you're not available this week, @sidharthachatterjee might just take it up directly. There's plenty of other issues tagged with help wanted Issue with a clear description that the community can help with. so please help out when you can!

We also offer pair programming sessions you can sign up for https://www.gatsbyjs.org/contributing/pair-programming/

@andrewfairlie
Copy link
Contributor

That makes sense to me. I'll keep an eye on this and see how the problem was solved and will take another issue when I have the time.

This will be a great feature 🤘

@polarathene
Copy link
Contributor

I can take it on since I've contributed a bit to gatsby-image already and fairly comfortable with it.

It just involves adding the loading attribute with eager or lazy and using feature detection based on the linked tweet example to avoid the current IO support/implementation?(IOSupported set to false?)

@KyleAMathews
Copy link
Contributor Author

Yup! Though we'd reuse the critical prop. We could add a loading prop as well to stay in sync with the new API.

You want to find a time to pair with @siddharthkp?

@polarathene
Copy link
Contributor

You want to find a time to pair with @siddharthkp?

I've not done pair programming before, especially remotely. I doubt my internet connection here is good enough for screen sharing or video chat, audio only probably isn't good either as the environment is often noisy(out of my control).

Or was it more to do with some real-time sync with text editors and some chat (slack, discord, etc)? I don't know if it'll be all that useful to arrange if the work to be done is minimal. Sounds rather straight-forward with just a few lines to PR?

I'm in NZ, it's 10.30PM atm, I've still got some other tasks to handle but should be free tomorrow to tackle this, if I get stuck I'll reach out :)


I can see above though that you seem to want to get this out quickly, so by all means if you'd rather assign it to @siddharthkp to get merged ASAP, go for it! Otherwise, tomorrow I'll assign myself to this if it's unclaimed.

@siddharthkp
Copy link
Contributor

/s/@siddharthkp/@sidharthachatterjee

thx bye 😋

@sidharthachatterjee
Copy link
Contributor

I missed these. Sorry @polarathene! 😞

Want to help review the PR at #13217?

(@siddharthkp What's up? 😂)

@Raincal
Copy link
Contributor

Raincal commented Apr 8, 2019

Should we support IE11?
https://caniuse.com/#feat=lazyload

@sidharthachatterjee sidharthachatterjee added type: feature or enhancement and removed help wanted Issue with a clear description that the community can help with. labels Apr 8, 2019
@polarathene
Copy link
Contributor

@Raincal that behaves a bit differently. It lowers loading priority but doesn't enable actual lazy loading.

I guess it could be added easily enough, lazyload="1" when the critical prop isn't present. But behaviour wise, images won't react differently because you've scrolled them into view, all images would still be loading, just other content is given higher priority.

Is that beneficial? Since future versions of IE/Edge are adopting Blink, and the current browser usage for that attribute being low (~4.5% Global, ~2.5% China, ~0.25% India), it's probably best suited to the user adding the attribute manually when it can make a meaningful difference.

@gatsbot
Copy link

gatsbot bot commented Apr 30, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

Thanks for being a part of the Gatsby community! 💪💜

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Apr 30, 2019
@LekoArts LekoArts added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Apr 30, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby-image@2.1.0

@trickydisco78
Copy link

I'm getting a console warning
[Intervention] An element was lazyloaded with loading=lazy, but had no dimensions specified. Specifying dimensions improves performance.

@alinenko
Copy link

It seems like because of Chrome native lazy loading thresholds custom gatsby-image lazy loading behavior is lost. In Firefox images are loaded as soon as they are in viewport but in Chrome hadrcoded thresholds are used and there is no way to neither customize native Chrome threshold nor refuse it and just use gatsby-image custom logic, is it right?

@polarathene
Copy link
Contributor

@alinenko yes that's correct. In a browser that supports native lazy loading, or does not have IntersectionObserver support, the visibility of the full image to load will be enabled(which will allow the browser to apply it's lazy/eager loading logic). The code that affects that is in the gatsby-image Image constructor:

    const isVisible =
      this.isCritical ||
      (isBrowser && (hasNativeLazyLoadSupport || !this.useIOSupport))

gatsby-image would need some other prop in this case to prevent that and allow for IntersectionObserver to be used. There was a PR in the past providing custom rootMargin instead of the fixed 200px(far smaller than the browser ranges you link to), but it was dismissed in favour of deferring to native browser handling. I don't think there is any other difference besides that.

If you have a case for needing to control the trigger distance, perhaps look up the previous issue or PR about this, or create a new issue. You'll probably have to wait until gatsby-image can be refactored into smaller components where you can compose your own variation. That won't happen this year. It's dependent upon a new major version of webpack and some other packages afaik(Gatsby 3.0?).

@alinenko
Copy link

@polarathene thank you for your response! Seems like the only way to handle this now is downgrading to pre-2.1.0. Works fine for Chrome but brings an issue to IE (even with polyfill usage) :(

@polarathene
Copy link
Contributor

@alinenko if there is breakage and no existing issue for it, perhaps you should open a new issue about that regarding IE. More likely to be reviewed by others than this closed issue. You will want to provide more details about what version of IE and a small example to reproduce it so it can be investigated and confirmed.

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 a pull request may close this issue.

10 participants