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

Incorrect aspect ratios when using gatsby-remark-image #24212

Open
Mike-Dax opened this issue May 19, 2020 · 3 comments · May be fixed by #39008
Open

Incorrect aspect ratios when using gatsby-remark-image #24212

Mike-Dax opened this issue May 19, 2020 · 3 comments · May be fixed by #39008
Labels
help wanted Issue with a clear description that the community can help with. status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) topic: remark/mdx Related to Markdown, remark & MDX ecosystem type: bug An issue or pull request relating to a bug in Gatsby

Comments

@Mike-Dax
Copy link
Contributor

Mike-Dax commented May 19, 2020

Description

Due to floating point precision error, or in some cases carried floating point precision errors, the paddingBottom "don't re-layout" trick sometimes results in an image that's a pixel off in height.

image

When displaying photos this isn't much of a problem, people won't notice, but displaying things like screenshots or graphics, that pixel difference can blur images noticeably.

Here's a difference map, it's just a 1px stretching, nothing amazing:

difference

Instead of using:

const ratio = `${(1 / fluidResult.aspectRatio) * 100}%`

We should probably use this?

const ratio = `${fluidResult.presentationHeight / fluidResult.presentationWidth * 100}%`

In order to maintain precision?

However in my reproduction I just happened to use an image where both versions produce incorrect final image heights, off by a pixel.

It would probably be a breaking change to pass height and width to all the places where this trick is done, regardless?


Gatsby-image itself does this:

object-fit:cover;
object-position:center

Object fit cover

The replaced content is sized to maintain its aspect ratio while filling the element’s entire content box. If the object's aspect ratio does not match the aspect ratio of its box, then the object will be clipped to fit.

This solves the visual blurriness problem, but the image is still off by a pixel in height, cut off.

I think object-fit:cover; should be added to gatsby-remark-image at a minimum, and hopefully some discussion can happen regarding the pixel-off problem.

At the end of the day, these images are supposed to be responsive, if pixel perfect matching is required, the fixed query should be used not fluid. That's fair enough.

I can do PRs for whatever we decide to do.

Steps to reproduce

https://gatsby-blurry-image-repro.netlify.app/markdown

You can swap between the markdown and the regular page with the same screenshot.

https://github.com/Mike-Dax/gatsby-blurry-image-repro

Code available here.

Environment

Environment

  System:
    OS: macOS Mojave 10.14.6
    CPU: (12) x64 Intel(R) Core(TM) i7-8700B CPU @ 3.20GHz
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 12.16.3 - /private/var/folders/dm/pj8g0t455gg16xy2614p_2q40000gn/T/xfs-6128b444/node
    Yarn: 2.0.0-rc.33 - /private/var/folders/dm/pj8g0t455gg16xy2614p_2q40000gn/T/xfs-6128b444/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v12.16.3/bin/npm
  Languages:
    Python: 2.7.10 - /usr/bin/python
  Browsers:
    Chrome: 81.0.4044.138
    Firefox: 76.0.1
    Safari: 13.1

@Mike-Dax Mike-Dax added the type: bug An issue or pull request relating to a bug in Gatsby label May 19, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 19, 2020
@vladar vladar added topic: remark/mdx Related to Markdown, remark & MDX ecosystem help wanted Issue with a clear description that the community can help with. status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 20, 2020
@github-actions
Copy link

github-actions bot commented Jun 9, 2020

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!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jun 9, 2020
@Mike-Dax
Copy link
Contributor Author

Mike-Dax commented Jun 9, 2020

This isn't stale, I'm happy to do a PR up that applies:

object-fit:cover;
object-position:center

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Jun 10, 2020
@LekoArts
Copy link
Contributor

Hi, thanks for the detailed issue!

We should probably use this?
const ratio = ${fluidResult.presentationHeight / fluidResult.presentationWidth * 100}%

If you look how the aspectRatio is calculated and the presentationHeight (both take the height/width of the image):

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sharp/src/index.js#L56-L59

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sharp/src/index.js#L615-L618

So this change could be made but as you said yourself it didn't make a change with the blurriness.

This solves the visual blurriness problem, but the image is still off by a pixel in height, cut off.
I think object-fit:cover; should be added to gatsby-remark-image at a minimum, and hopefully some discussion can happen regarding the pixel-off problem.

I think this is fine to do, having the image cut off by 1 pixel instead of having a blurry image is a good trade-off. As you mentioned with fluid images you run into such problems sometimes on certain screen sizes.

So yeah, please go forward with a PR adding the object-fit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue with a clear description that the community can help with. status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) topic: remark/mdx Related to Markdown, remark & MDX ecosystem type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
3 participants