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

gatsby-image browser support #4021

Closed
TuckerWhitehouse opened this issue Feb 13, 2018 · 32 comments
Closed

gatsby-image browser support #4021

TuckerWhitehouse opened this issue Feb 13, 2018 · 32 comments
Labels
good first issue Issue that doesn't require previous experience with Gatsby

Comments

@TuckerWhitehouse
Copy link
Contributor

Description

gatsby-image uses object-fit/object-position which are not supported in IE.
https://www.gatsbyjs.org/docs/browser-support/ states support for IE 9+.
Some discussion about the issue and a workaround can be seen here: #2470 (comment)

Expected behavior

a) Either a note on gatsby-image that includes a "blessed" solution for IE,
b) an object-fit/object-position polyfill in gatsby-image.

I'd be happy to open a pull request for either of these options (not sure what including the polyfill directly would look like), but maybe adding the note about object-fit-images would be enough?

@KyleAMathews
Copy link
Contributor

Oh hmmm yeah — we should definitely polyfill gatsby-image for IE. object-fit-images looks great from a quick look.

@mwickett
Copy link

I've just come across this issue on a project that has (sadly) about 15% IE11 usage. I'm happy to jump in and take a crack at adding the polyfill - just wanted to check in here and see if there's any WIP or other thinking around this. Thanks!

@thebarty
Copy link

plus one - got the same problem

@ooloth
Copy link
Contributor

ooloth commented May 11, 2018

Hey all! I also noticed this issue. Here's how you can fix it:

npm i intersection-observer object-fit-images
// gatsby-browser.js

exports.onClientEntry = () => {
  // NOTE: Don't polyfill Promise here (Gatsby already includes a Promise polyfill)

  // IntersectionObserver polyfill for gatsby-image (Safari, IE)
  if (typeof window.IntersectionObserver === `undefined`) {
    require(`intersection-observer`)
    console.log(`👍 IntersectionObserver is polyfilled`)
  }

  // Object-fit/Object-position polyfill for gatsby-image (IE)
  const testImg = document.createElement(`img`)
  if (
    typeof testImg.style.objectFit === `undefined` ||
    typeof testImg.style.objectPosition === `undefined`
  ) {
    require(`object-fit-images`)()
    console.log(`👍 Object-fit/Object-position are polyfilled`)
  }
}

The intersection-observer polyfill works out of the box.

However, the 'object-fit-images` polyfill requires that you add the following CSS to your image's styles:

font-family: "object-fit: VALUE_HERE; object-position: VALUE_HERE"

You can either add this font-family declaration manually to each image, or create a wrapper for gatsby-image like the following:

// Img.js (a gatsby-image wrapper)

const Img = props => {
  // Construct font-family declaration for object-fit-images
  const objFit = props.objFit ? props.objFit : `cover`
  const objPosition = props.objPosition ? props.objPosition : `50% 50%`
  const fontFamily = `"object-fit: ${objFit}; object-position: ${objPosition}"`

  const imgStyle = {
    objectFit: objFit,
    objectPosition: objPosition,
    fontFamily: fontFamily
  }

  return (
    <Image
      sizes={props.sizes}
      alt={props.alt}
      className={props.className}
      style={props.style}
      outerWrapperClassName={props.outerWrapperClassName}
      imgStyle={{ ...imgStyle }}
      position={props.position || `relative`}
      backgroundColor={props.backgroundColor || `transparent`}
      Tag={props.Tag || `div`}
    />
  )
}

/*
 *
 * Imports & Exports
 * 
 */

import React from 'react'
import Image from 'gatsby-image'

export default Img

Hope that helps!

@fk
Copy link
Contributor

fk commented May 15, 2018

Thanks for sharing @ooloth! 🤗

After copy-pasta-ing your fix, more specifically the gatsby-image wrapper component, I noticed that resolutions aren't passed on, and the gatsby-image imgStyle prop got lost.

Here's my quick fix for that:

const Img = props => {
  // Construct font-family declaration for object-fit-images
  const objFit = props.objFit ? props.objFit : `cover`
  const objPosition = props.objPosition ? props.objPosition : `50% 50%`
  const fontFamily = `"object-fit: ${objFit}; object-position: ${objPosition}"`

  const polyfillStyles = {
    objectFit: objFit,
    objectPosition: objPosition,
    fontFamily: fontFamily,
  }

  return (
    <Image
      sizes={props.sizes}
      resolutions={props.resolutions}
      alt={props.alt}
      className={props.className}
      style={props.style}
      outerWrapperClassName={props.outerWrapperClassName}
      imgStyle={{ ...props.imgStyle, ...polyfillStyles }}
      position={props.position || `relative`}
      backgroundColor={props.backgroundColor || `transparent`}
      Tag={props.Tag || `div`}
    />
  )
}

@ooloth
Copy link
Contributor

ooloth commented May 17, 2018

@fk Awesome! Thanks for the code review. 😎

@jlengstorf
Copy link
Contributor

To simplify this a bit further, we can pass all props through except the ones we actually need:

const Img = ({ objFit = `cover`, objPosition = `50% 50%`, ...props }) =>(
    <Image
      {...props}
      imgStyle={{
        ...props.imgStyle,
        objectFit: objFit,
        objectPosition: objPosition,
        fontFamily: `"object-fit: ${objFit}; object-position: ${objPosition}"`,
      }}
    />
  )

This should allow usage of this wrapper exactly the same as gatsby-image but with the polyfill in place.

I feel like it probably makes sense to roll this into gatsby-image itself, rather than requiring people to add a wrapper. @KyleAMathews @m-allanson @pieh any objections to adding this polyfill directly into core for IE9 support?

@KyleAMathews
Copy link
Contributor

oh, if that's all the polyfill is then by all means, let's add it into the component directly.

@jlengstorf
Copy link
Contributor

jlengstorf commented Jun 29, 2018

@KyleAMathews There are also two dependencies we'd need to add to onClientEntry: #4021 (comment)

@KyleAMathews
Copy link
Contributor

Hmmm so the two together are 3.7kb

That seems a bit much to add to every user of gatsby-image?

Perhaps just document well this for people who need to support older IEs?

@jlengstorf
Copy link
Contributor

@KyleAMathews Would dynamic import remove that extra size for modern browsers?

exports.onClientEntry = async () => {
  // NOTE: Don't polyfill Promise here (Gatsby already includes a Promise polyfill)

  // IntersectionObserver polyfill for gatsby-image (Safari, IE)
  if (typeof window.IntersectionObserver === `undefined`) {
    await import(`intersection-observer`)
    console.log(`👍 IntersectionObserver is polyfilled`)
  }

  // Object-fit/Object-position polyfill for gatsby-image (IE)
  const testImg = document.createElement(`img`)
  if (
    typeof testImg.style.objectFit === `undefined` ||
    typeof testImg.style.objectPosition === `undefined`
  ) {
    await import(`object-fit-images`)()
    console.log(`👍 Object-fit/Object-position are polyfilled`)
  }
}

(Please note that I haven't tried this and I'm not 100% sure if I've got the syntax right.)

@KyleAMathews
Copy link
Contributor

Oh yeah! That'd totally work.

No reason to await though (also await isn't well-supported in browsers or babel so best still to avoid it in frontend)

@ooloth
Copy link
Contributor

ooloth commented Jul 19, 2018

In Gatsby v2, will Babel 7 automatically polyfill IntersectionObserver as a result of it being referenced in the gatsby-image code?

I'm currently loading the intersection-observer polyfill manually (using the dynamic import approach shown above) but I'd be happy to skip that step if manual JS polyfills aren't needed in v2.

(I assume CSS polyfills like object-fit-images still need to be applied manually.)

@kakadiadarpan
Copy link
Contributor

@jlengstorf @KyleAMathews dynamic import would be really great so that everyone gets out of the box support for legacy browsers. Since we already have the solution let's fix this soon?

@jlengstorf
Copy link
Contributor

@kakadiadarpan seems reasonable to me. Does anyone have time to implement this? We're effectively looking to add the solution I proposed in #4021 (comment), but without the async/await part.

@ooloth
Copy link
Contributor

ooloth commented Oct 26, 2018

@jlengstorf Sure! I’m happy to take this one. I’ve been using this solution on my end (in gatsby-browser) and it would be nice to bake it in.

@ooloth
Copy link
Contributor

ooloth commented Oct 26, 2018

Quick Gatsby plugin authoring question for the group!

I'm following the contributing instructions here and my local updates to the gatsby-image package in my local gatsby fork are being successfully applied to my gatsby test site (i.e. the gatsby-image files in my Gatsby test site's node_modules folder are automatically updating as I make changes).

Although everything appears to be wired up correctly, the gatsby-browser.js I've added to gatsby-image does not appear to be working.

For example, the following console log does not appear on the test site:

// gatsby/packages/gatsby-image/gatsby-browser.js
// same as: test-site/node_modules/gatsby-image/gatsby-browser.js

"use strict"

exports.onClientEntry = () => {
  console.log(`Working`)
}

Is there a trick to getting gatsby-browser.js exports in a gatsby package to be picked up by a site using the package?

@pieh
Copy link
Contributor

pieh commented Oct 26, 2018

For gatsby-*.js files to be registered and executed by gatsby - package would need to be in gatsby-config.js plugin list - so if You add code in gatsby-image/gatsby-browser.js, users would need to add gatsby-image to plugin list

@ooloth
Copy link
Contributor

ooloth commented Oct 26, 2018

@pieh Thank you!

Such a simple fix. 👍

@trickydisco78
Copy link

Has this been added to gatsby-image? If not what's the preferred solution for IE11?

@ooloth
Copy link
Contributor

ooloth commented Dec 7, 2018

Not quite yet! (The PR is in progress.)

In the meantime,

  1. Install the polyfills:
npm i intersection-observer object-fit-images
  1. Dynamically load the polyfills in gatsby-browser.js:
exports.onClientEntry = () => {
  // IntersectionObserver polyfill for gatsby-image (Safari, IE)
  if (typeof window.IntersectionObserver === `undefined`) {
    import(`intersection-observer`)
    console.log(`👍 IntersectionObserver is polyfilled`)
  }

  // Object-fit/Object-position polyfill for gatsby-image (IE)
  const testImg = document.createElement(`img`)
  if (
    typeof testImg.style.objectFit === `undefined` ||
    typeof testImg.style.objectPosition === `undefined`
  ) {
    import(`object-fit-images`)()
    console.log(`👍 Object-fit/Object-position are polyfilled`)
  }
}
  1. And wrap gatsby-image with a component that passes in the values needed to implement the fontFamily declaration that activates the object-fit-images polyfill:
import Image from 'gatsby-image'

const Img = ({ objFit = `cover`, objPosition = `50% 50%`, ...props }) => (
    <Image
      {...props}
      imgStyle={{
        ...props.imgStyle,
        objectFit: objFit,
        objectPosition: objPosition,
        fontFamily: `"object-fit: ${objFit}; object-position: ${objPosition}"`,
      }}
    />
  )

@mik-laj
Copy link

mik-laj commented Dec 21, 2018

https://gist.github.com/mik-laj/d9455f4543c71376c9b2220f3437f5de
I am currently using such code, which on older devices excludes all image optimizations, but does not cause the page speed to decrease on new browsers. This is not the final solution, but when it is not developed better, it is worth using.

@JustinTRoss
Copy link
Contributor

JustinTRoss commented Jan 22, 2019

For folks copying from here, import('object-fit-images')() results in an error for invocation of a non-function. Without the async/await from the initial snippet, we're invoking a promise.

The following works as an alternative.

import('object-fit-images').then(({ default: ObjectFitImages }) => ObjectFitImages());

@gatsbot
Copy link

gatsbot bot commented Feb 17, 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 Feb 17, 2019
@marcysutton
Copy link
Contributor

How's the PR going, is there anything we can help with? I just ran into this myself, so very interested to see what we do here. IE11 is still very much in use for Windows users.

@ooloth
Copy link
Contributor

ooloth commented Feb 21, 2019

Hi @marcysutton!

The PR has gone a bit quiet:

  1. At first, it seemed like a great idea to bundle intersection-observer and object-fit/position polyfills with gatsby-image.
  2. Then, it was decided that intersection-observer polyfilling should be an app-wide decision (since it affects more than gatsby-image), so the PR focused on just adding an object-fit/position polyfill to gatsby-image.
  3. Then, concerns were raised about increasing the bundle size of gatsby-image for users who don't want this polyfill as well as the ugliness of the font-family hack required to make the polyfill work.

At this point, it's unclear to me whether adding the object-fit/position polyfill to gatsby-image is the preferred approach.

It may be cleaner to simply add a page to the docs that explains how to wrap gatsby-image with your own custom GatsbyImageWithIEPolyfill component that adds the object-fit/position polyfill if you want it in your app. My Dec 7, 2018 comment above (with @JustinTRoss' improvement on Jan 22, 2019) shows all that would need to be documented.

A documentation-based approach has the advantage of keeping the gatsby-image code simpler and cleaner (the font-family declarations look pretty weird), which keeps the bundle size smaller and makes it easier for contributors to work on.

I'm interested in hearing your thoughts!

@KyleAMathews
Copy link
Contributor

It may be cleaner to simply add a page to the docs that explains how to wrap gatsby-image with your own custom GatsbyImageWithIEPolyfill component that adds the object-fit/position polyfill if you want it in your app.

That's a really interesting idea — we could actually ship that as part of gatsby-image so all you'd need to do is import Img from "gatsby-image/withIEPolyfill"

@TuckerWhitehouse
Copy link
Contributor Author

Would it be possible to check the browserslist and include the correct version automatically if the user needs to support IE? Otherwise I think the wording of https://www.gatsbyjs.org/docs/browser-support/ should be updated to reflect Gatsby supports evergreen browsers, and with a little effort, IE9+ can be supported as well, but it's not by default.

@jlengstorf jlengstorf added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Feb 21, 2019
@jlengstorf
Copy link
Contributor

That's a really interesting idea — we could actually ship that as part of gatsby-image

I really like the idea of offering it by default, so if we can support import Img from 'gatsby-image/withIEPolyfill' I'm way into it.

@ooloth
Copy link
Contributor

ooloth commented Mar 19, 2019

Alrighty, I was finally able to carve out some time to resolve this puppy!

I've created a new PR (#12681) that allows users who want object-fit/object-position support in IE to import Img from "gatsby-image/withIEPolyfill" instead of importing from "gatsby-image".

Thanks to @KyleAMathews and @jlengstorf for suggesting this approach.

Please feel free to review the PR and let me know if the implementation can be improved in any way.

wardpeet added a commit that referenced this issue Mar 26, 2019
…t-fit/position support (#12681)

## Description

This PR will allow users who want `object-fit/object-position` support in IE to simply `import Img from "gatsby-image/withIEPolyfill"` instead of importing directly from `"gatsby-image"`.

Please feel free to review the PR and let me know if the implementation can be improved.

The `/withIEPolyfill` version of `gatsby-image` currently does the following:

1. Checks if the browser supports the `object-fit/position` CSS properties.
  a. If yes, no polyfill is loaded.
  b. If no, the `object-fit-images` polyfill is imported and called.
2. Wraps `gatsby-image` in a component that passes the new `objectFit` and `objectPosition` prop values to the polyfill implementation (which requires a weird `font-family` hack).

### New props

To make the implementation simple, I've added new `objectFit` and `objectPosition` props that will needed to be used to pass the corresponding values to the polyfill. This is to avoid a scenario where a user attempts to set these values in a way we can't pass to the polyfill (e.g. via an external CSS stylesheet) and doesn't understand why the polyfill isn't working. Let me know if this API can be improved.

### Loading polyfill in /withIEPolyfill/index.js

I opted to load the `object-fit-images` polyfill directly in `withIEPolyfill/index.js` rather than in `gatsby-browser.js`. If anyone knows of a better way to approach this, please let me know and feel free to make improvements.

### Polyfill repo has been archived

I was surprised to see that the `object-fit-images` [repo](https://github.com/bfred-it/object-fit-images) has recently been archived by its owner. It still works, but if anyone is concerned about this or knows of a reliable alternative polyfill, please let me know.

### Docs to be updated once implementation is finalized

Once we've settled on the API for this `/withIEPolyfills` component, a brief explanation will need to be added to the `gatsby-image` docs. I held off on adding one for now in case we end up changing the API.

## Related Issues

Fixes #4021.

Co-authored-by: Ward Peeters <[email protected]>
@PolGuixe
Copy link
Contributor

@ooloth great work!

Have you observed any issue in production using the polyfill?

Are there any performance issues on using it?

Do you know any websites that are using it? I am wondering if gatsbyjs.org is using it as I can see the images in IE11.

@ooloth
Copy link
Contributor

ooloth commented Sep 20, 2019

@PolGuixe Thanks!

Have you observed any issue in production using the polyfill?

Nope! Please let me know if you do.

Are there any performance issues on using it?

Not that I've encountered.

Do you know any websites that are using it?

I've used the object-fit-images polyfill on ~20 Gatsby sites and found it works well.

I am wondering if gatsbyjs.org is using it as I can see the images in IE11.

The gatsbyjs.org site isn't using it (you can verify this by searching the www folder). To clarify, gatsby-image works just fine in IE11 on its own; what doesn't work are the object-fit and object-position CSS properties, since IE11 doesn't support them.

The /withIEPolyfill import applies the object-fit-images polyfill for you to get those two CSS properties working if your project needs them. For projects that don't use those CSS properties, the normal gatsby-image import is all you need (even for IE11).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that doesn't require previous experience with Gatsby
Projects
None yet
Development

No branches or pull requests