feat(gatsby-image): Image cache logic improvements #26090
Conversation
Introducing non-IO logic into `handleRef()` in future commits. Guard is to prevent running code when `ref` is null(component unmount). `isVisible` guard should be safe to move. Will only run the logic once now as it should, once visible. Beneficial to Art Directed images, so that each variant doesn't create a IO listener as they're switched in via another PR that introduces `handleImageVariant()`.
This method will be useful in future commits elsewhere as well, hence is DRY. It also allows for simplifying logic (at the expense of some more verbosity), dropping the usage of `!!(currentSrc)`, as instead of casting to a bool, we can rely on falsy values in conditional statements instead. `imageInCache` is used to decide if a single render can be performed instead as `setCachedState()` would be unnecessary, otherwise we make the image visible to begin loading it and can attempt to check if the browser has cached the image, doesn't work for Firefox however.
Also adds a guard to not pointlessly set the `imgLoaded` state in `handleImageLoaded()` to true if it already is (triggers a render). It can otherwise hide or create problems. Such as a picture element in art directed images changing source, React would render the component again only after new image is downloaded and fires `handleImageLoaded()`, which was too late anyway on slow connections.
This is only a fix if the hydration PR is merged, as the order of `componentDidMount()` and `handleRef()` changes. That impacts when `imageRef` is available. The loaded event should fire afterwards still in my testing. When this feature was originally added to `componentDidMount()` it was to ensure transition CSS did not need to be applied if the image had already loaded. As the `handleImageLoaded()` is still called, other logic there should continue to run at that point as expected. `img.complete` is still more reliable here than `img.currentSrc` as by this point, since such images are already apart of the original DOM, they will likely have a `img.currentSrc` available, even if they've not finished loading, if they have then `img.complete` will be true. Potentially a cache detection concern after initial page load for critical images, as they'll effectively be like any other image, `img.complete` isn't likely to be useful at this point, where `img.currentSrc` should instead be the more reliable source of information..
Presently, due to v2 of `gatsby-image` avoiding breakage, classname support is for the `img` element of the placeholder, `picture` was introduced afterwards. If one wants to avoid the placeholder image being briefly visible on page load due to it being inlined into the DOM, a CSS animation with CSS keyframes can set opacity to 0 and after a delay set it to 1, or smoothly transition to reveal the image. If the image is already cached, this would instead allow for not revealing the placeholder for 300ms or less if React hydrates before then and the animation is appended as a `<style>` element that's only added for SSR. Cannot apply such animation to the `img` element which has `opacity` as an inline style. `!important` does not work for overriding it with CSS `animation` which ignores `!important` in keyframes, setting the `opacity` value with `!important` would prevent `animation` from applying different values. Thus, this opacity toggle needs to be applied one level higher, `gatsby-image-wrapper` alone is not useful if you want to still have a `backgroundColor` visible, unless you target a specific hierarchy and expect that not to break.
Renders a blank content for initial frame rendered, prevents the otherwise unavoidable frame of placeholder being rendered during a page transition when the image has not been added to the internal cache(which skips the Intersection Observer render path). Drawback is a slight flicker with initial page loads (between inlined DOM placeholder and hydration). Combined with CSS keyframes would only be noticeable when loading an HTML file with images for the first time (or longer than the CSS keyframe opacity delay). Under ever other render path `isVisible` is true, so nothing else is impacted. Trades a better UX for repeated visits instead of first time.
Reworded for better clarity. Added note about the IO render path and future instances/mounts of the image resource.
Found a render bug with Chrome if displaying the `img` element prior to the image request returning a response. Most notable on fluid type images, if the requested image does not share the same aspect ratio of it's container, it will be distorted (resized to conform to the container aspect ratio) until the response is returned. Appears to be a bug with Blink implementation of `object-fit: cover`, it's as if that rule is not applied until the image response has returned. The delay is minimal in testing with the browser "Online" network, so opting to hide the placeholder like `imgLoaded` would, but not reveal the main image straight away does resolve the visual issue nicely when paired with CSS keyframe animation opacity delay. However, on higher latency networks, at least when simulated with Chromes "Fast 3G", 300ms was not sufficiently long enough for the CSS keyframe animation prior to React kicking in, so placeholder blur was visible for a moment before hiding placeholder again. Not visible for non-cached images, but potentially degrading UX for repeated visits on higher latency connections. Could use `performance.now()`to know when `gatsby-image` has started since browser load to opt-in/out of `imgCached` behaviour if it exceeded the CSS keyframe animation duration. Note that prior to this PR, the `object-fit: cover` distortion on fluid images is present when `fadeIn` prop is set to false, provided the container and image differ in aspect ratio.
Required for both `currentSrc` and `complete`, better to check early.
`<picture>` is only used for the Art Direction feature, thus this is an unreliable element to add a class for targeting CSS animation on. For now, the wrapper class will have to be targeted instead.
Prettier rules removed the parenthesis around the OR condition that followed a ternary statement assignment, personally that reduced readability, so extracted to a variable. To avoid confusion with the similar opposite named variable, it was renamed and better describes it's purpose.
gatsbot
bot
added
the
status: triage needed
|
Drafting, but would appreciate review/discussion to continue.
Could include the CSS keyframes animation as a const isBrowser = typeof window !== `undefined`
// ...
return (
<>
{ !isBrowser && (
<style>
{`
.gatsby-image-wrapper {
opacity: 0;
animation:0s ease 0.3s normal forwards 1 gatsby-placeholder-delay;
}
@keyframes gatsby-placeholder-delay {
from { opacity: 0; }
to { opacity: 1; }
}`}
</style>
)}
<GatsbyImage {props} />
</>
)Unfortunately, cannot target the |
polarathene
added
help wanted
Test observationsProp:
|
|
There has been a report of Safari with Intersection Observer for lazy loading always reporting I'd still appreciate a few other Safari users to confirm that That behaviour is the opposite of Firefox which would always report false. And more interestingly conflicts with Epiphany (Webkit browser) which I've been using for Safari comparisons as it's usually able to reproduce the same issues/behaviour. |
|
hey @polarathene, I've just tested this on:
I can confirm that The effect is that, without the CSS hack described in #20126 (comment), the images just appear suddenly, instead of fading in nicely. |
|
@c0derabbit Awesome thankyou, did you happen to log to console the truthy value? Was it an asset URL? I'm curious if it's the appropriate one on desktop when you load the page without browser cache at different viewport widths or if it's just placeholder filling it with the Safari 14 was released recently (September 16th 2020) and available on Catalina, this has native image lazy loading afaik. As However with recent updates to |
|
@c0derabbit PR available, I've not tested it, but it should make Safari users of |
|
Closing, this is fixed in our new gatsby-plugin-image component. Thank you for your contribution! <3 |
Problem
Using a placeholder such as base64 is nice to have for first retrieval of assets. However when they are cached this results in:
gatsby-imagecache:It's a minor UX issue, but it would be nice to smooth these out.
Examples
Description
When loading a Gatsby page with an image in the viewport, if the page has been previously visited and the browser has cached the image, there is still a flicker of a placeholder if used(base64/SVG), this can look unsightly/jarring and be unexpected.
This PR tries to resolve that issue as best as possible, favoring a blank(no placeholder) rendered frame over a frame with a placeholder. This is a React rendered frame, in that it can last longer than 16ms, dependent on when the next frame is rendered, often long enough to be visible for a moment that it's noticeable.
Not fully tested, nor is the approach a complete solution(
currentSrctechnique does not work for Firefox, a variety of other browsers that I've tested with are able to leverage it).Individual commits provide better details on changes, and try to make for easier review.
Related Issues
Original attempt prior to native image lazyloading in Chrome/Firefox: #12254 (comment)
A user mentions it was a problem for them when providing their Gatsby site for offline usage, they ended up switching to SVG resources: #12254 (comment)
Fixes: #12836
Fixes: #18858
Previous PR by me introducing
imgCached: #12468A prior PR of mine mentions the issue will exist for images not using Intersection Observer render path: #14158
Fixes: #25942 (recent issue by me looking into the issue)
Related: #22255
TODOcomment.