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

If gatsby-image already has an item in cache, don't show the placeholder #12254

Closed
KyleAMathews opened this issue Mar 3, 2019 · 13 comments
Closed
Assignees
Labels
type: bug An issue or pull request relating to a bug in Gatsby

Comments

@KyleAMathews
Copy link
Contributor

You can see this happening on this blog post. Click to it and back several times. The blurred up base64 image is shown every time.

https://www.gatsbyjs.org/blog/2019-03-01-localization-with-gatsby-and-sanity/

@sidharthachatterjee sidharthachatterjee added the type: bug An issue or pull request relating to a bug in Gatsby label Mar 4, 2019
@polarathene
Copy link
Contributor

By "in cache" is this referring to the browser cache for images or the lookup object/dict imageCache in gatsby-image? If the latter, that state would not persist would it?

Also note the src value used as a key is not equivalent to the image elements currentSrc that the browser sets(eg for webp when supported), not that it matters for a cache key(it does if you want to check browser cache, but making the relevant ref available when cache is checked in the components constructor isn't an option afaik?

If ignoring the currentSrc issue above, the browser cache could be checked and work with single image format sources?

@polarathene
Copy link
Contributor

polarathene commented Mar 10, 2019

I have spent time looking into resolving this. Considered Cache API and types of persistent storage, realizing they are not suitable(invalid if viewport width has changed requiring a different source size).

I came across the currentSrc check for cache I mentioned earlier, and I've got that working(either via componentDidUpdate, or an additional ref to the picture element so the currentSrc can be properly checked/queried once the picture element is mounted/visible.

A) Prior browser cache check logic via Promise

Previously, I was doing a more thorough check of the browser cache via a promise that would wait about 10ms to check the browser cache(0ms for in-memory cache, but disk-cache on SSD with my system would require up to 10ms, on slower systems or disk access, that could be even longer making it less reliable:

// A function outside of `Image` component
const isImageCached = (currentSrc) => {
  return new Promise((resolve, reject) => {
    // empty string if not in the browser cache
    if (currentSrc.length > 0) {
        const image = new window.Image();
        image.src = currentSrc;

        const checkBrowserCache = () => {
          const isCached = image.complete
          image.src = ""

          resolve(isCached)
        }

        // Image should be loaded via browser cache within 10ms
        setTimeout(checkBrowserCache, 10);
    } else { 
      resolve(false) 
    } 
  })
}

// Within `handleRef()`:
const img = this.imageRef.current
isImageCached(img.currentSrc).then(isCached => 
    this.setState({ imgCached: isCached })
)

I have since removed the need for the picture element ref or componentDidUpdate() method. handleRef() is appropriate, and can be set right after isVisible is set to true(but not at the same time as for whatever reason imageRef is not valid prior until after that setState() call/line). I moved imgLoaded into this 2nd setState() call, which optimizes by avoiding the browser initializing animation/render frames inbetween the state updates if the image is in the browser cache.

Also depending how transition styles are managed, if a transition is already active, changing it's duration/delay has no effect iirc, you had to adjust the opacity value to trigger a new transition animation, so this was another benefit of keeping these two state updates together.

B) Trust currentSrc value as a cache hit

AFAIK, this can be simplified to just checking the length of currentSrc(only tested locally in Chrome). It provides the relevant src url that the browser had chosen once the element is rendered from isVisible == true, and thus we can assume it's cached as for whatever reason, when the image/element is not cached by the browser, the currentSrc value is an empty string.

Simplified(also now updates with imgLoaded since we don't have to wait on a browser cache result):

this.setState({ 
    imgLoaded: imageInCache, 
    imgCached: this.imageRef.current.currentSrc.length > 0
})

C) Chained/nested setState() call

Should this 2nd setState() be used as the 2nd param for the 1st setState()? due to the async nature of setState(), it might be more reliable/consistent this way(it would then always be applied after the first state has been updated and rendered IIRC, vs potentially being batched and breaking like described above as a single state update(I haven't experienced such when doing the 2nd state update directly after).

React may batch multiple setState() calls into a single update for performance. -Source

this.setState( 
    { isVisible: true }, 
    () => this.setState({ 
        imgLoaded: imageInCache, 
        imgCached: this.imageRef.current.currentSrc.length > 0
    })
)

Placeholder can briefly be seen, could use CSS Keyframes to avoid?

You're likely to still see the placeholder graphic for a brief moment when it's delivered in the initial html payload(base64), until the JS kicks in.** Is that acceptable? **

If you want to avoid that as well, then I believe we need something like CSS Keyframe animation to initialize the placeholder opacity at 0, and fade-in after an initial delay(250-500ms for slow devices), in cached scenarios the JS should be able to cancel that keyframe animation from starting.

imgCached boolean added to negate transition styles for browser cache hits

I introduce a imgCached boolean state as shown above(could maybe use the existing seenBefore instead?). Using it as a conditional in styles, we can remove the transition styles for browser cached image hits.

If the developer wants the cached image to still have a fade-in effect, they'll need to add the additional style to override imgCached cancelling it out(documentation will need to reflect this, as it'd probably be confusing when fadeIn prop is looked up, which would now only be applied without a browser cache hit).

transitionDelay for placeholders, remove conditional, use 0.5s always

transitionDelay on the placeholder is 0,5s for base64 and 0.35s for bgcolor. The base64 delay does have 0.25s mentioned, but I can't see any reason it'd actually be used? imgLoaded affects when opacity for these placeholders is set to 0, and the actual image has 0.5s opacity transition duration. I'm of the understanding that 0.25s and 0.35s values aren't serving a purpose, and they should be 0.5s to wait for the actual image to complete it's fade-in(preventing whatevers behind a placeholder bleeding through, which is unpleasant when image and background are contrasting).

The 0.25 and 0.35 delays for these placeholders, perhaps they were intended for fade-in? Which won't work due to their opacity starting at 1 any how?

A developer may want to override the placeholder transition styles for transparent images during loading as well, so that the placeholder is removed/replaced by the full image fading in ASAP. Presently, there is no control for the developer to do this with bgcolor placeholders.

transition duration on image placeholder

This style has been with the component since it's arrival. The style was later moved, and someone observed that it was no longer applied to the base64 placeholder, made a PR and got it merged back. Thing is, with the transition delay and current behaviour, you're not actually going to see this placeholder do a transition(except on transparent images where it probably doesn't look nice). Is removing it ok?

Styles for browser cache hits, avoiding transitions

const imagePlaceholderStyle = {
  opacity: this.state.imgLoaded ? 0 : 1,
  transitionDelay: this.state.imgCached ? `0s` : `0.5s`,
  ...imgStyle,
  ...placeholderStyle,
}

const shouldDisplay = this.state.imgLoaded || this.state.fadeIn === false
const shouldFadeIn = this.state.fadeIn === true && !this.state.imgCached
const imageStyle = {
  opacity: shouldDisplay ? 1 : 0,
  transition: shouldFadeIn ? `opacity 0.5s` : `none`,
  ...imgStyle,
}

With transition duration (and alternative to avoid assigning transition rules):

const transitionStyle = {
  transition: `opacity 0.5s`,
  transitionDelay: `0.5s`
}

const imagePlaceholderStyle = {
  opacity: this.state.imgLoaded ? 0 : 1,
  ...(!this.state.imgCached && transitionStyle),
  ...imgStyle,
  ...placeholderStyle,
}

I'll go ahead and setup a PR.

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Apr 1, 2019
@gatsbot
Copy link

gatsbot bot commented Apr 1, 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! 💪💜

@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 1, 2019
@polarathene polarathene self-assigned this Apr 1, 2019
wardpeet pushed a commit that referenced this issue Apr 2, 2019
…ache" (#12468)

Placeholder transition CSS continues to work as normal, only when the image exists in the browser cache are the transition styles removed. This is mainly to address/reduce the undesirable transition for transparent images as #12254 describes.
johno pushed a commit to jlengstorf/gatsby that referenced this issue Apr 10, 2019
…ache" (gatsbyjs#12468)

Placeholder transition CSS continues to work as normal, only when the image exists in the browser cache are the transition styles removed. This is mainly to address/reduce the undesirable transition for transparent images as gatsbyjs#12254 describes.
@devrnt
Copy link

devrnt commented Jun 14, 2019

I see the branch was already merged, but it seems like I'm still experiencing the described issue by @KyleAMathews.
I'm displaying a logo on every page, but whenever the page is changed the logo is showing the blur effect and using the fluid image, even when the image is already cached.

@polarathene
Copy link
Contributor

polarathene commented Jun 14, 2019

even when the image is already cached.

@devrnt Do you have an example/reproduction that demonstrates this? Or a link to an affected project online?

Are you using any props like critical? Is lazy-load in use, and if so, is it via Intersection Observer or native support in Chrome?

There's a possibility that the image config(props & feature support) for that logo is causing the browsers image cache check to not be performed.

@devrnt
Copy link

devrnt commented Jun 14, 2019

@polarathene Since I didn't figure out the problem I moved to use a svg. It was a strange behaviour. The 'problem' is the same as on this webpage: https://www.gatsbyjs.org/blog/2019-03-01-localization-with-gatsby-and-sanity/

Since everything was cached and available offline (in my case) I would think that that the blur should disappear, but it was reloading every time (like on the example) when changed from page. (home to contact etc.)

When the problem was occuring I used a fixed <Img/> with the fixed property provided.

@polarathene
Copy link
Contributor

The 'problem' is the same as on this webpage: https://www.gatsbyjs.org/blog/2019-03-01-localization-with-gatsby-and-sanity/

@devrnt can you explain what action you're doing to reproduce the issue you're experiencing? In the gatsby blogpost link, what should I click/navigate? If you just navigate to another page in the header links, and click the browsers back button(or via mobile), you may briefly see the base64 placeholder, but you won't see the transition effect anymore.

Just to clarify. Are you referring to the low quality base64 image as the blur effect? Or the actual CSS transition that fades it out over 0.5 sec? This feature does not have the ability to remove the base64 image until the JS for the react gatsby-image component can run. The HTML is sent with that base64 placeholder, so it will be present still on a refresh, even if your cache has the full image. The slower the device to process the JS (and for React to update the DOM after), the longer that placeholder will be visible.

I proposed earlier in this issue, that it could be addressed with a CSS keyframe transition, but this cannot be done via inline styles afaik. It would allow you to not show the placeholder for a small duration to give React some time to kick in.

Also, as you've not mentioned your project as being online, if this is something you're experiencing during development, please be aware that gatsby develop does not set cache headers in the dev server, so the feature(no fade transition if image is cached) would not work in that environment either.


TL;DR: If the network tab does confirm the image is cached and you are experiencing a fade transition effect that occurs when there is no cache, then that is indeed a bug. Otherwise, it may briefly be visible.

@devrnt
Copy link

devrnt commented Jun 14, 2019

@polarathene Thanks for the extensive information. I've reproduced the issue: https://yvesdevrient.netlify.com/

As I can see in the network tab it says both the logo and banner are in memory cache, but as you can see the effect is still occurring whenever you change a page.

@polarathene
Copy link
Contributor

polarathene commented Jun 14, 2019

@devrnt the url doesn't seem to be valid. I get "Not Found".

@devrnt
Copy link

devrnt commented Jun 14, 2019

@polarathene It's back online.
I'm having some strange behaviour, first when I changed from page it was showing the logo and banner exactly like the example. But know as you can see there is a small 'stutter' when I change the page.

@polarathene
Copy link
Contributor

polarathene commented Jun 14, 2019

But know as you can see there is a small 'stutter' when I change the page.

@devrnt Yes I can see what you are referring to. That is the base64 placeholder flashing/"stuttering". Is it in the Layout component? Or are you placing the header in each page component? It looks like the logo is being unmounted and remounted each time for some reason.

Whatever the cause, this behaviour is unrelated to this issue or the PR that addressed it. I would suggest reaching out via community support, such as the Discord server. If no one is able to assist you in identifying the cause of the issue, create a public github repo that demonstrates the behaviour and then raise a new issue here linking to it.

In the meantime, you can use a graphql fragment that won't apply a base64 placeholder:

GatsbyImageSharpFixed_noBase64, more are listed here.

@devrnt
Copy link

devrnt commented Jun 14, 2019

@polarathene Thank you very much!

Edit: I saw that the parent component of my logo was rebuilding on changing page, that explains why it does rebuild.

@spaut
Copy link

spaut commented Oct 20, 2019

@devrnt
I think I'm having the same problem. How do you diagnose that the parent component is rebuilding? How did you fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

No branches or pull requests

6 participants