Don‘t show placeholder if image is served from Browser cache

toisrael commented 4 years ago

Description

Even if images are served from the browsers cache, the placeholder and blur effect is shown.

Steps to reproduce

You could see that behavior when revisiting a site (e.g. https://www.buschmais.de).

Expected result

The Image served from the Browsers cache is used and no placeholder nor blur effects takes place.

At least, if intentional, the behavior should be a configuration option whether or not the placeholder animation should be used in cases where images are from the browser cache.

timhagn commented 4 years ago

Hi @toisrael,

would you mind sharing a little reproduction of your use case and (especially) a gatsby info of your project? Had no problems in FF or Chromium on Linux...

Best,

Tim.

toisrael commented 4 years ago

Hi @toisrael,

would you mind sharing a little reproduction of your use case and (especially) a gatsby info of your project? Had no problems in FF or Chromium on Linux...

Best,

Tim.

Sure. We’re using gatsby 2.17.0 with background images plugin V0.8.15.

Results with FF on Windows:

timhagn commented 4 years ago

I thought bout running the command gatsby info in the cli at the root folder of your project, but at the moment yours' is enough ; ). Would you be so kind and try upgrading gatsby-background-image to the most current v0.9.12? There should have been some progress on this...

Best,

Tim.

github-actions[bot] commented 4 years ago

Hi there! As @timhagn momentarily is the main contributor to this package, this issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs, though we're open to suggestions on how to get more maintainers! Thank you for your contributions : )!

toisrael commented 4 years ago

I upgraded the plugin to v0.9.12 as proposed. The upgrade itself did not changed the described behavior - the placeholder image and animation still takes place on 304, cache served, images. Tested with latest versions of FF under Windows, Linux and macOS.

gatsby info

  System:
    OS: macOS 10.14.6
    CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 10.17.0 - /usr/local/opt/node@10/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 6.11.3 - /usr/local/opt/node@10/bin/npm
  Languages:
    Python: 2.7.16 - /usr/bin/python
  Browsers:
    Chrome: 79.0.3945.88
    Firefox: 70.0.1
    Safari: 13.0.4
  npmPackages:
    gatsby: 2.17.0 => 2.17.0
    gatsby-background-image: 0.9.12 => 0.9.12
    gatsby-cli: 2.8.3 => 2.8.3
    gatsby-image: 2.2.29 => 2.2.29
    gatsby-plugin-layout: 1.1.13 => 1.1.13
    gatsby-plugin-react-helmet: 3.1.13 => 3.1.13
    gatsby-plugin-sass: 2.1.20 => 2.1.20
    gatsby-plugin-sharp: 2.2.32 => 2.2.32
    gatsby-plugin-sitemap: ^2.2.22 => 2.2.22
    gatsby-source-filesystem: 2.1.33 => 2.1.33
    gatsby-transformer-remark: 2.6.30 => 2.6.30
    gatsby-transformer-sharp: 2.2.23 => 2.2.23
  npmGlobalPackages:
    gatsby-cli: 2.8.3

Thanks! Tobias

jamessimone commented 4 years ago

It seems both this package and gatsby-image don't support either the browser or disk cache when reloading the main image, which causes server-side / hard-refreshes of any page with background images to be flagged in ImageCache's inImageCache method as not in the cache (because the image cache is just an empty object on page refresh / non-client side nav). Interestingly, the placeholder images for both packages are served from the cache on subsequent reloads. I don't think the blame is on you, but perhaps that might help in researching this issue.

timhagn commented 4 years ago

Hi @all,

as @jamessimone correctly pointed out, the cache object resets on refreshes. I played around a little with the CacheStorage API, as the Browser Cache sadly doesn't seem to be directly accessible by JS (as far as I gathered for security reasons), one seems only able to ask if it's active and clear it programmatically.

You may have a look at my experiments in the chache-storage-try & second-cache-try branches, though both weren't fruitful : (.

Would be open for any suggestions, though : ).

Best,

Tim.

jamessimone commented 4 years ago

@timhagn thanks so much for exploring the options. I wonder if anybody on the core Gatsby team has any further experience with this. I believe you’ve worked with them on parts of this package in the past? It does seem bizarre that in both gatsby image and here the base 64 placeholder images do end up getting served from the cache but the loaded image does not.

I’ve done some work on gatsby-image in the past but never really looked into the caching side of things. It seems like something the Gatsby team would be incentivized to work on though ... I’m happy to raise an issue on the core gatsby project or if you were open to doing so, that would also be cool.

timhagn commented 4 years ago

Still trying to get this to work... We're now at v0.9.16, though it doesn't seem to help on reload, only on page-change by nav : /.

Keeping at it, but was sick for the last 1 1/2 weeks and am plastered in work anyway % ).

Best,

Tim.

EDIT: @jamessimone Yup, they lent a hand one or two times for gbi, but most of times it's the other way around and I'm a contributor at multiple gatsby core packages % ). Didn't find the time to open an issue over there, so go for it if you want - was sick for the last 1 1/2 weeks while plastered with work ^^.

jamessimone commented 4 years ago

@timhagn understood! Yes, I can open an issue with them - feel better!

timhagn commented 4 years ago

Currently I see no solution to this. The CacheStorage API only works over https, so local testing would be out of the question:

Note: Chrome and Safari only expose CacheStorage to the windowed context over HTTPS. window.caches will be undefined unless an SSL certificate is configured. from MDN

LocalStorage would trigger problems with the GDPR (or would it?), Cookies even more (and probably even CacheStorage or any other form of "persistence")...

Perhaps over at Gatsby they might have some more ideas, but for now I sadly see no way : (.

Best and have a nice weekend,

Tim.

github-actions[bot] commented 4 years ago

Hi there! As @timhagn momentarily is the main contributor to this package, this issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs, though we're open to suggestions on how to get more maintainers! Thank you for your contributions : )!

timhagn commented 4 years ago

Hi @all!

As even Kyle Matthews mentioned this as default behavior for gatsby-image, I'm closing this for the moment. I just couldn't think of a way to solve this without the GDPR kicking in... Still open for suggestions, though, and I'm gonna reopen it : )!

Best, nevertheless,

Tim.

polarathene commented 4 years ago

Ello, this bothered me in the main gatsby-image package, I don't use gatsby-background-image and only ended up here while searching related issues on the topic.

I worked on gatsby-image cache improvement over a year ago and describe the options I explored here. I discarded the Cache API too, and had a Promise approach that seemed reliable but you'd have to account for some delay, a simpler and reliable approach was leveraging currentSrc from the image element itself.

This was prior to native lazy loading support arriving iirc, and my fix there may only work with Intersection Observer API being used as a fallback. I'll look into that soon.

My concern was with page loads/refreshes flashing the base64 placeholder very briefly when the original image is cached. As my linked comment describes, it's due to the base64 being inlined in the HTML and keyframes CSS not being possible for inline styles... A recent PR I contributed uses <style> tags within gatsby-image instances, so this should also work there at delaying placeholder visibility for long enough that it won't be visible if the image loads fast enough from being in cache.

timhagn commented 4 years ago

Thanks a lot, @polarathene, gonna look into that as soon as possible : )!

polarathene commented 4 years ago

@timhagn Just to confirm, when you've been looking into this yourself, how have you been testing the caching?

I had been using gatsby build and gatsby serve for a local webserver that gatsby provides, which I'm sure had some benefit over develop such as setting network cache headers, but that doesn't appear to be the case anymore if it ever was.

I had been noticing an intermittent network caching behaviour when switching between two image sources via the Art Direction feature and an active PR I have waiting for merge which improves the responsiveness of that switch(without the PR it's latency bound, as currentSrc isn't updated until the new image source request receives a response).

With Chrome(on Linux), there was some odd network caching behaviour, seemed to send out a request and then return a 304 if content hadn't changed(so it doesn't re-download it if already in cache), as the cache-control from the gatsby server was public, max-age=0, I assume the ETag was compared to get the 304.

As I resized across the media condition width to switch the image, no network requests are made for the images besides the first one since a refresh...until a few seconds after load, which was always reproducible, other times it'd trigger is unclear. I'm assuming that's browser dependent on when it decides it's own internal cache can be used(currentSrc is valid/updated) and when it's decided to evict from that cache and make a new network request that gets logged(currentSrc is empty again, even if the image has been used before, since it's performing a new network request for it).

Without Cache-Control header providing a rule to actually cache the image assets, on Fast 3G it's about 500ms for a response, and 2sec for a response on Slow 3G. When the network requests are allowed to be cached, it's less than 2ms even on Slow 3G pulling from memory-cache or disk-cache.

Network caching issues aside, the problems discussed here are:

First visit/load, hydration briefly renders the placeholder before cached image replaces it.
Navigating to pages from the initial load, the gatsby-image cache is lost, so transitions play when they shouldn't need to.

Correct? These are both resolvable (at least for gatsby-image, not familiar with this projects internals).

Hydration can be resolved with CSS keyframe animation to delay placeholder visibility, 100-200ms should do it. Drawback is that for a brief moment the delay would be the background of where the image is, so it might appear like a slight flicker, can be reduced with backgroundColor placeholder that matches the dominant colour of the image if that's an issue to soften the delayed appearance. The CSS keyframe can reside in a <style> tag that is added during SSR and removed upon hydration, on slower connections it will allow for the placeholder to appear(it just also shares the slight visibility delay drawback, instead of the placeholder being immediately visible as it would be now).
The page transition with internal cache was resolved by me in the past as mentioned. imgCached logic does work and prevent that problem, it's just due to the switch to native lazy loading that it'd be visible issue again. Restoring the imgCached functionality is fairly simple and should resolve this issue.

I should have a PR up soon on the main gatsby repo, I can link it here when I do if that'd be helpful to reference?

EDIT: Appears to be some difference between gatsby develop and gatsby serve as handleRef() doesn't recognize the currentSrc field, although I'm not sure what the difference is yet. This is regarding a browser refresh and the initial currentSrc being falsy always in dev, but only falsy on first load from a build.

polarathene commented 4 years ago

My imgCached approach works mostly for the browsers that use Intersection Observer, which for most is Safari or older versions of Edge I think, Chrome and iirc Firefox have native lazy load support.

currentSrc can't be acquired fast enough with Intersection Observer due to the actual image element not being added to the DOM until it triggers(isVisible is then set to true to render it), but the component itself will have the placeholder already rendered prior even in a cached scenario where you're clicking a link to visit another "page"(without the gatsby-image cache having a hit), thus for 1 frame(lasts 40ms for me via Chrome performance profile, regardless of network throttle speed) the placeholder is visible.

The options are either to create temporary dummy picture element with sources srcSet and sizes to have the browser select the appropriate image source and provide a correct currentSrc ahead of time, I think that would require updating the DOM, which while you could remove the test element in the following frame is more processing and network requests that probably is undesirable, as every image on the page would run this when mounted.

Using the CSS keyframes should work, but would move the "flicker" frame to hydration phase for slower connections. Doesn't resolve the issue properly, but perhaps less of a UX problem and more acceptable to push it there? A cleaner transition would be a solid colour placeholder for pre-hydration at SSR with the base64/svg placeholder transitioning in via CSS keyframe if the cache isn't detected in 100ms kicking in at hydration.

polarathene commented 4 years ago

LocalStorage would trigger problems with the GDPR (or would it?), Cookies even more (and probably even CacheStorage or any other form of "persistence")...

I just couldn't think of a way to solve this without the GDPR kicking in...

@timhagn I don't think it would fall under GDPR if you're only running generic logic on the client, you're not doing anything with personal attributable data, nor sending the data anywhere. If you're just wanting to store some response cache metadata similar to the headers and the URLs match what are on the page content already, it doesn't seem like anything malicious could be done?

MDN mentions for img loading attribute that it only kicks in when JS is available to avoid abuse of lazy loading images as trackers. Perhaps that applies if your locally stored data is available to third-party JS to access, which might be a concern if the image package was common enough to encounter online, I just don't see how it'd be much different from sending a fetch request of URLs from available data to get their response headers and check for the same information.

My draft PR for my attempt on gatsby-image of this is available here, I did manage to avoid the placeholder with Intersection Observer usage(although it's a blank frame instead), and share some gifs of various tests.

Doesn't work for Firefox however in my testing, currentSrc doesn't provide a value like Blink and Webkit based browsers are doing. This is at least with the hydration placeholder flicker tests, and quite likely due to Firefox reload which IIRC enforces a full revalidation of all resources, Chrome modified their reload functionality in 2017 in collaboration with Facebook, so it behaves a bit differently from traditional reload that Firefox has. Facebook also collaborated with Firefox, they opted to keep their standard reload behaviour and instead support the immutable directive for Cache-Control header, in testing this didn't seem to help with currentSrc though(nor when opening a new tab and visiting the URL).
Doesn't work when cache is disk-cache either.
There was an issue with Blink browsers when revalidating the cached response, it shows the cached(stale) image but width/height dimensions seem to be invalid until a response which prevents object-fit: cover CSS from working properly resulting in image distortion if aspectRatios don't match(can happen with Fluid type images). This is avoidable if using Cache-Control: stale-while-revalidate=0, but most users may not know about that and have Cache-Control: max-age=0 on platforms like Netlify or Vercel by default causing the visual glitch like related gif in my PR shows.

I think I could test locally with HTTPS, maybe give the CacheStorage API a look(and your earlier attempts), or consider storing the response expiry in LocalStorage. I don't see that solution being anymore likely to be accepted into gatsby-image however(and would probably cause some developer confusion since most aren't using HTTPS locally and cite enough disparity issues already between develop and production). Probably won't have time for this however.

timhagn / gatsby-background-image