fix(gatsby-image): Fix fluid not respecting maxWidth/maxHeight

[sbardian]

Description

An attempt at fixing gatsby-image fluid not respecting maxHeight #15167 issue. Instead of using the sizes string to pull out max-width/height, I decided to try to add to maxHeight/maxWidth fields to the node and use those in gatsby-image on the gatsby-image-wrapper div. This might be way off base, and may presents other problems I'm not seeing at this point, but thought I would give it a try. This change touches many different pieces of code, and I am in no way confident this is a great/easiest/best way to fix this. :) My manual testing seems to fix the issue though.

Related Issues

closes #15167

[wardpeet]

I'm not 100% sure what to do here. We have presentationWidth & presentationHeight which we can already use for this. Another thing is I'm unsure if baking this in gatsby-image is the right decision.

Problems with this solution is that we need to update our fragments to make it universal to always include presentationWidth & presentationHeight inside fluid fragments, which is probably not a big deal.

const Image = () => {
  const data = useStaticQuery(graphql`
    query {
      placeholderImage: file(relativePath: { eq: "gatsby-astronaut.png" }) {
        childImageSharp {
          fluid(maxHeight: 150) {
            presentationWidth,
            presentationHeight,
            ...GatsbyImageSharpFluid
          }
        }
      }
    }
  `)

  return <Img fluid={data.placeholderImage.childImageSharp.fluid} style={{ width: data.placeholderImage.childImageSharp.fluid.presentationWidth, height: data.placeholderImage.childImageSharp.fluid.presentationHeight }} />
}

[sbardian]

@wardpeet you are right, I didn't even notice those were getting the same values. I can drop the 3 commits dealing with adding maxHeight/maxWidth (4ac71dd, a838a30, c34d5b4) and work from there using presentationHeight and presentationWidth instead. Unless you think we should close this PR and come at this from a totally different angle?

I did some quick testing, including presentationHeight and presentationWidth manually along side ...GatsbyImageSharpFluid, and updating my changes in 74e8f3b to use the presentation values instead of the maxHeight/maxWidth I had added to fluid. It seems to work fine on the gatsby-image-wrapper.

[sbardian]

@wardpeet, I have pushed the changes we talked about. I think I updated the fragments correctly, but I had a heck of a time getting my test project to see / take those updates, so maybe there is somewhere else I'm not aware of that will need to be updated as well?

I was getting some strange errors (didn't seem to be associated with my changes), but they seemed to go away after pulling and rebasing off master again, I think something was broken upstream for awhile, FYI in case they show back up.

[sbardian]

thanks @wardpeet 👍

[pieh]

This changes default behaviour and users will get different results for same code (breaking change) - some people rely on current behaviour. I think it's also easier to to set maxWidth,maxHeight than unset it. Also the maxWidth and maxHeight argument don't apply to presentational dimensions - but rather to size of generated images. Changing that removes "fluid" characteristic of fluid image ;) (above certain threshold)

As a note - please check #10460 for PR that was pretty similar to this one.

[sbardian]

Thought I would give this one last go 😀 sorry @pieh

This still might "break" people who are relying on the current behavior...

Thought here is adding maxWidth and maxHeight to the node, to use in gatsby-sharp-plugin and gatsby-image. Also, adding a container around gatsby-image-wrapper to keep the fluid image behavior. This seems to work as one might expect maxHeight and maxWidth to work.

I see now that you say it, presentationHeight and presentationWidth "max out" at the image dimensions, with this update they respect the maxHeight and maxWidth args passed in the query.

Testing this seems to function pretty well, but I understand it could throw everyone using this for a loop. We can just close it out and go with the documentation with the easy fix 👍 no big.

[sbardian]

one thing I can't seem to get to work, is getting the update fragments in my project. so when testing I have to include maxHeight and maxWidth in my query with ...GatsbyImageSharpFluid. Tried using gatsby clean / running build on gatsby-transformer-sharp then running gatsby develop /etc. Can't seem to get the new fragments to populate in my dev projects cache.

even if all that happens out of this PR is I learn how to update fragments, I consider that a win :)

[sbardian]

I have created a gatsby test site for testing this PR. https://github.com/sbardian/my-default-starter. directions in README for anyone who needs them.

[pieh]

Hey, don't worry about me ;)

I know this is confusing and problematic (hence attempt to address it with this PR), only thing I wanted to convey is that we have to be careful about breaking changes.

I don't have proper answer on how to best solve this. Adding documentation is "safest way out" (in that we avoid making breaking changes, and that's why we opted into this way in that previous PR), but this will still bite people that don't expect our current behaviour and it's not unreasonable to think that meaningful portion of users might never find our documentation for this problem

[sbardian]

Will wait and see what wardpeet or anyone else that might check it out thinks. Give it a test drive and let me know. Thanks!

[sbardian]

@pieh @wardpeet I was looking through the documentation on the manual "fix" for this. I don't think this will be a breaking change. I think people using the documented fix would just end up overriding the new maxHeight and/or maxWidth values I'm setting on the image, when they pass their normalizedProps.styles to the gatsby-image component. So, no breaking change for people using the documentation as far as I can see.

[sbardian]

@pieh @wardpeet looking at the documented fix, it will not always actually work. presentationWidth and presentationHeight will not always equal the maxHeigth and maxWidth args. They max out at the image dimensions. So using the gatsby-astronaut.png from the default starter as an example, if you had the following query:

query MyQuery {
  allImageSharp(filter: {fluid: {src: {regex: "/astronaut/g"}}}) {
    nodes {
      fluid(maxWidth: 2000, maxHeight: 2000) {
        src
        maxWidth
        maxHeight
        presentationWidth
        presentationHeight
      }
    }
  }
}

you will get the following results (using my branch) or the same minus maxHeight/maxWidth using master branch:

{
  "data": {
    "allImageSharp": {
      "nodes": [
        {
          "fluid": {
            "src": "/static/6d91c86c0fde632ba4cd01062fd9ccfa/eb6cb/gatsby-astronaut.png",
            "maxWidth": 2000,
            "maxHeight": 2000,
            "presentationWidth": 800,
            "presentationHeight": 800
          }
        }
      ]
    }
  }
}

So with the fix you would be setting the gatsby-image styles to maxHeight/maxWidth: 800, not the 2000 you requested in your query.

[sbardian]

@gillkyle have you had a chance to review? Thoughts? Anyone, else I should reach out to?

[gillkyle]

Hey @sbardian, all your work on this looks awesome and your research into the problem is really helpful!

I probably don't have as much context as @pieh and @wardpeet (who can probably make the final say better), but I do like the idea of documenting what you've found tricky with this better in the API doc page. I actually wasn't aware of the presentationHeight and presentationWidth in the sharp plugin, if that's actually a use case that solves some problems it'd be worth documenting that somewhere too (doesn't look like it's in the gatsby-plugin-sharp README or the docs AFAIK). Gatsby image has always been a little hard to teach, so deciding what to do under the hood and how much to try and explain through docs is a tricky balance. All of these efforts should help a lot though 🙂

[wardpeet]

Hey @sbardian sorry for taking so long but could you make an example site which has a lot of different image resolutions and see if this actually fixes things for all cases. It's going to be easier to get this through.

Thanks for your patience and sorry for taking so long.

[sbardian]

@wardpeet no problem. I think I fixed up the issue @smashercosmo caught.

[AAverin]

Is this coming any time soon?
At the moment specifying maxHeight for fluid images doesn't size the image to expected maximal height instead seemingly ignoring the property.

[wardpeet]

This looks great, I'm running some tests and hopefully, I can merge this today.

[sbardian]

@wardpeet you might still be working on this, if so ignore me. Pulled down the update and it seems to break the fix. Though I'm not able to run yarn run bootstrap and the project might not be getting loaded into my dev environment correctly? FYI . . .

[wardpeet]

Hey @sbardian, what do you mean it doesn't work anymore? It seems to work fine for me but maybe I'm testing it the wrong way 😬

export const query = graphql`
  query {
    allImageSharp {
      nodes {
        wh: fluid(maxWidth: 800) {
          ...GatsbyImageSharpFluid_withWebp
          ...GatsbyImageSharpFluidLimitPresentationSize
        }
        hw: fluid(maxHeight: 500) {
          ...GatsbyImageSharpFluid_withWebp
          ...GatsbyImageSharpFluidLimitPresentationSize
        }
      }
    }
  }

full page screenshot

[sbardian]

@wardpeet let me pull down the latest commits and see what I get today. I was testing against the project I used before to test here. If you have time pull it down and see if it is still working for you as well maybe? I'll follow up here in a few. Thanks!

[sbardian]

@wardpeet with your update to the fragments I was also not getting maxWidth/maxHeight coming across to my project. But I think that is only because I'm not sure how to get gatsby to update the fragments, when I add them specifically I get the values.

      cropWidth: file(relativePath: { eq: "gatsby-astronaut.png" }) {
        childImageSharp {
          fluid(maxWidth: 200, maxHeight: 600) {
            ...GatsbyImageSharpFluid
            maxHeight
            maxWidth
          }
        }
      }

[wardpeet]

Hey sorry, i changed the PR a bit so we don't change behavior in gatsby-image v2. In v3 we should do stuff differently like we do now.

I've added a new fragment called GatsbyImageSharpFluidLimitPresentationSize that you can add to your sharp query to make this work. See it as an opt-in feature. After the merge, I'll chat with our docs team to make it more obvious on how to use it in our tutorials.

You'll have to do the following:

       childImageSharp {
          fluid(maxWidth: 500) {
            ...GatsbyImageSharpFluid
            ...GatsbyImageSharpFluidLimitPresentationSize
          }
        }

What do you think of this change?

[sbardian]

@wardpeet I see. Looks good. Trying to test it but it thinks the fragment doesn't exist. I'm not sure how to get gatsby to "refresh" the fragments.

[sbardian]

@wardpeet if you are setting the maxHeight/maxWidth to the presentationHeight/presentationWidth I'm not sure this will end up being correct, see this comment of mine on the issue of using presentationHeight/Width as maxHeight/Width. comment. Basically the presentationHeight/Width max out at the images dimensions, so if your image is 800x800 and you set maxWidth to 1500, it will still only be maxWidth 800.

[sbardian]

and this comment also.

[sbardian]

I was finally able to get the fragments to update and working using:

       childImageSharp {
          fluid(maxWidth: 500) {
            ...GatsbyImageSharpFluid
            ...GatsbyImageSharpFluidLimitPresentationSize
          }
        }

and it does appear to be using presentationHeight/Width. So any maxHeight/width will max out at the image dimensions. This is why I had the logic in gatsby-plugin-sharp

  let maxWidth
  let maxHeight
  if (options.maxHeight || options.maxWidth) {
    maxWidth = options.maxWidth
      ? `${options.maxWidth}px`
      : `${options.aspectRatio * options.maxHeight}px`
    maxHeight = options.maxHeight
      ? `${options.maxHeight}px`
      : `${options.aspectRatio * options.maxWidth}px`
  }

and specifically returned maxHeight/maxWidth it as part of the fluid return object.

[wardpeet]

Setting it to presentationWidth/height feels more correct as your the container shouldn't be bigger than image. If you need that, you'll need a bigger image, else you'll end up with quality drop.

If you want to center the image or override the behavior you can still add a wrapper or set the maxWidth/maxHeight yourself on gatsby-image.

[sbardian]

If this makes more sense I'm good with it.

[wardpeet]

I approve this message! Thanks @sbardian for the hard work on this and leading the way to get this merged!

[sbardian]

Thanks @wardpeet !

[sbardian]

@wardpeet sorry to be a downer. . . but it appears using presentationHeight/presentationWidth also broke cropping. So with the following query:

      cropWidth: file(relativePath: { eq: "gatsby-astronaut.png" }) {
        childImageSharp {
          fluid(maxWidth: 200, maxHeight: 600) {
            ...GatsbyImageSharpFluid
            ...GatsbyImageSharpFluidLimitPresentationSize
          }
        }
      }

Instead of getting a skinny tall image cropped to your specifics, you end up with a 200x200 image. One way to fix this is to remove the documentation comment in the example here so no one expects a cropping behavior I guess? Thoughts?

Correction: It may only be affecting one aspect of cropping. See the example "maxWidth: 200, maxHeight: 600. Crop Width" here

[pieh]

The PR was published in:

• gatsby-image@2.4.3
• gatsby-plugin-sharp@2.6.2
• gatsby-transformer-sharp@2.5.2

The cropping problem might need to be looked at :(

[wardpeet]

I will :)

[sbardian]

@wardpeet I think I have a fix for this

by replacing gatsby-plugin-sharp /src/index.js lines 477-484 here with the following:

  // Determine maxHeight and maxWidth when fluid query includes one or the other
  // start presentation values with image height and width
  let presentationWidth = width
  let presentationHeight = height

  if (!options.aspectRatio) {
    options.aspectRatio = width / height
  }

  if (options.maxWidth) {
    // set maxHeight based on maxWidth
    presentationWidth = Math.min(options.maxWidth, width)
    presentationHeight = Math.round(
      options.maxHeight
        ? options.maxHeight
        : options.maxWidth * options.aspectRatio
    )
  } else if (options.maxHeight) {
    // set maxWidth based on maxHeight
    presentationHeight = Math.min(options.maxHeight, height)
    presentationWidth = Math.round(
      options.maxWidth
        ? options.maxWidth
        : options.maxHeight * options.aspectRatio
    )
  } else {
    // prevent errors if fragment GatsbyImageSharpFluidLimitPresentationSize
    // is used without supplying maxHeight or maxWidth
    options.maxWidth = width
    options.maxHeight = height
  }

Also update gatsby-plugin-sharp/src/plugin-options.js by replacing lines 108 with the following:

  if (options.maxWidth !== undefined) {
    options.maxWidth = parseInt(options.maxWidth, 10)
  } else if (options.maxHeight !== undefined) {
    options.maxHeight = parseInt(options.maxHeight, 10)
  }

Basically remove first if condition, no point setting maxWidth if the user does not pass one, we will use the image dimensions.

We get cropping back. The only issue is the image will grow to match a maxWidth or maxHeight query value if given. BUT it will not grow beyond the dimensions of the image itself. So if the image was 200x500 and they had maxHeight: 2000, it will max out at the 500 image height.

I also noticed some odd behavior in the plugin healOptions function. It returns 800 width for images maxWidth if one is not supplied. I would think this should default to the images width dimension? this was fixed with plugin-options.js change above.

Anyway, not sure if you wanted a new PR to help fix this issue or not. Let me know if so. I have also deployed a new demo site here This demo site has updated images and the query descriptions towards the bottom. You can see the queries and demo code here

[wardpeet]

@sbardian thanks for your thorough comment! It seems like calculating presentationWidth & height is a mistake, we should get it from the image itself. I've created #23905 to deal with that.

Basically remove first if condition, no point setting maxWidth if the user does not pass one, we will use the image dimensions.

Sadly changing this default will be a breaking change, I do think in a next version we should make maxWidth or maxHeight mandatory.