feat(gatsby-image): don't fadein image when already loaded "browser-cache"

[polarathene]

Description

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.

Additionally:

• base64 and bgcolor placeholder delays 0.25s and 0.35s respectively have been replaced with the 0.5s duration of the fadein transition.
• base64 transition duration was removed. It's not something that would be witnessed unless using transparent images, in which case it only prolongs the visibility after the main image has transitioned in.

See my comment in the referenced issue for some considerations/variations or further reasoning for the changes made.

imgLoaded and imgCached could be renamed to isImgLoaded isImgCached?
delayHide maybe should be delayHideStyle?

Related Issues

Fixes: #12254, Fixes #10937

[polarathene]

noscript, removed transition and transition-delay, dropped width/height props in "fixed" image section

noscript version migrated sometime ago via this PR(states due to errors but no citation of what those were, edit: here it is and direct link to issue/workaround comment, As of late 2018 it is fixed, but only for React 16.5+, if you use 16.4, you don't get the error but lose lazy loading.. ). So gatsby-image could go back to supporting noscript without a string for the image, probably more maintainable, just need to ensure users are aware to use React 16.5+, thoughts?

Previously it used the <Img /> component, which at the time had opacity and transitionDelay props(although the noscript version never used them afaik), those were brought over into the noscriptImg function during the change from using the <Img /> component to returning a regular <img /> tag string. And then another PR to address an issue with that new approach changing the function to resemble what we have presently.

The code for "Fixed" images , also passes in width/height, this is taken from the image object(fluid/fixed prop types) width/height fields, although this object is passed in as props anyhow via spread syntax which overwrites the prior width/height props (just like custom styles could override this PR feature from working correctly, even if not related to opacity transition).

CSS Transition on placeholder unintended?

The CSS transition duration for opacity was removed from the default <Img /> styles in this commit, it ensured that the transition was only applied to the actual image to fade-in, while the placeholder would delay changing it's opacity until after the fade-in animation completed... however, someone thought this omission of the transition on the placeholder style was an error, but no actual issue/reason justified it being added back in the PR.

Transition delay also seems to have varied. While it's 0.35s for for bgcolor, it used to also be that for base64 placeholder, both placeholders have mixed delays at this commit, just noticed that bgcolor has different delay values for fixed/fluid in master currently... I think it's safe to say this was a typo that moved around.

[polarathene]

Well this failure is confusing. I had run the tests locally and updated them noting in the relevant commit that it seemed odd the error hadn't been raised before this PR, and now the CI is unhappy with the snapshot(while local is fine with the exact same code).

yarn run jest ./packages/gatsby-image:

yarn run v1.13.0
$ jest ./packages/gatsby-image
 PASS  packages/gatsby-image/src/__tests__/index.js
  <Img />
    ✓ should render fixed size images (31ms)
    ✓ should render fluid images (6ms)
    ✓ should have correct src, title and alt attributes (21ms)
    ✓ should have correct placeholder src, title, style and class attributes (6ms)
    ✓ should call onLoad and onError image events (14ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   2 passed, 2 total
Time:        1.072s
Ran all test suites matching /.\/packages\/gatsby-image/i.
Done in 1.68s.

cat ./packages/gatsby-image/src/__tests__/index.js:

// `display: inline`, oddly doesn't show up in the snapshot either?
const setup = (fluid = false, onLoad = () => {}, onError = () => {}) => {
  const { container } = render(
    <Img
      backgroundColor
      className={`fixedImage`}
      style={{ display: `inline` }}
      title={`Title for the image`}
      alt={`Alt text for the image`}
      {...fluid && { fluid: fluidShapeMock }}
      {...!fluid && { fixed: fixedShapeMock }}
      onLoad={onLoad}
      onError={onError}
      itemProp={`item-prop-for-the-image`}
      placeholderStyle={{ color: `red` }}
      placeholderClassName={`placeholder`}
    />
  )

  return container
}

cat ./packages/gatsby-image/src/index.js:

// Img component has the styles that are now in the snapshot and CI should recognize the last 3 lines of style
const Img = React.forwardRef((props, ref) => {
  const { sizes, srcSet, src, style, onLoad, onError, ...otherProps } = props

  return (
    <img
      sizes={sizes}
      srcSet={srcSet}
      src={src}
      {...otherProps}
      onLoad={onLoad}
      onError={onError}
      ref={ref}
      style={{
        position: `absolute`,
        top: 0,
        left: 0,
        width: `100%`,
        height: `100%`,
        objectFit: `cover`,
        objectPosition: `center`,
        ...style,
      }}
    />
  )
})

My snapshot was updated by running yarn run jest -u like yarn test suggests upon failures. I thought I had updated everything correctly and since it "worked on my machine", pushed it. Any ideas why the CI is acting differently?(some sort of cache or different environment?)

[polarathene]

this.state.fadeIn === false could be added for negating the placeholder delay too? Probably isn't a case where you have fadeIn disabled and want to delay the placeholder opacity toggle? That'd cater to #10937

EDIT: Done.

Still need advice about the failing snapshot test.

EDIT: Fixed once #12719 merges, otherwise tests can be reverted.

[polarathene]

@wardpeet Would you be able to review/merge this please? TL;DR version is:

Addresses both linked issues:

• Avoid image transition if cached.
• Respect fadeIn=false to avoid undesirable transitions with transparent images due to placeholder transition style.

Three main changes(each commit provides more details):

• Check if image is available in the browser cache. (it was more complex but this seems to work fine)
• Removed some dead code related to supporting noscript (we could also revert the change to string approach if React >=16.5 can be enforced)
• Refactor/organize style logic:
  • Move some conditionals out into variables with clear names for what they're checking for.
  • Remove the 0.25s duration as it was a typo some time ago, and instead
  • Use a shared duration variable to avoid the typo issue in future
  • Placeholder style no longer has a transition, only a delay for opacity toggle. (Doesn't appear to be a good reason to keep an opacity transition effect).

[wardpeet]

looking at this right now! thanks for you patience and great comments!

[wardpeet]

This looks great! Such a great improvement. I love this and so will our users.

Just a few questions but not blocking!

[wardpeet]

not sure but maybe shouldLazyLoad is a better name? not sure if lazyload is the right term here

[polarathene]

I think the condition for that boolean gives the context/clarity what it is for, it was just more appropriate to summarize it as a should variable where it was used so the code for the style was still easy to read when the logic would otherwise get a bit long/messy.

If shouldDisplay doesn't feel right, how about shouldReveal? The variable is describing when the image is ready to display by toggling the opacity, a shouldLazyLoad seems to describe the wrong relationship, you could try isLazyLoaded or isReady(since ultimately it's a true value you're after, there isn't really a distinction to lazy-loading or not.

[wardpeet]

👍 for shouldReveal! let me change that and merge afterwards!

[polarathene]

The Windows unit test is failing unrelated to the contents of this PR:

> prettier "**/*.{md,css,scss,yaml,yml}" "--check"

Checking formatting...
docs\blog\2019-03-29-interview-with-david-eads\index.md
Code style issues found in the above file(s). Forgot to run Prettier?

Related issue: #13011
EDIT: Fixed the issue with this PR

[wardpeet]

Thanks again! this is awesome!

[sidharthachatterjee]

Published in [email protected]