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

fix(gatsby-image): Fix issue where Safari downloads multiple resolutions #11920

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

tbrannam
Copy link
Contributor

@tbrannam tbrannam commented Feb 20, 2019

Description

Safari browser (Safari 12 / Mac OS) downloads multiple assets for a single Img tag.

The observed behavior is that the attributes of the <img> tag are queued early in the parsing of the document.

Resolved by removing the default (non-webp) <source> and placing those value back into the fallback <img> tag. Additionally the order of the attributes of <img> are important as well. If src attributes precedes srcset in the <img> Safari’s pre-parser will enqueue both images to download.

Finally - added a note regarding a dependency on IntersectionObserver for lazy-loading

Testing Fail-case: https://using-gatsby-image.gatsbyjs.org/prefer-webp/ note that each image has two downloaded image sizes (also note that the markup for the cactus image is referenced twice, thus 4 images are downloaded).

Related Issues

Safari browser (Safari 12 / Mac OS) downloads multiple assets for a single `Img` tag.

The observed behavior is that the attributes of the `img` tag are queued early in the parsing of the document.

Resolved by removing the fallback `<source>` and placing those value back into the `<img>` tag.

Additionally the order of the attributes of `img` are important as well.   If `src` preceeds `srcSet` in the `<img>` Safari’s Pre-parser will enqueue both images to download.

Finally - added a note regarding a dependency on IntersectionObserver for lazy-loading
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Someone else want to verify the problem in Safari and the fix?

@sidharthachatterjee sidharthachatterjee self-assigned this Feb 20, 2019
@sidharthachatterjee sidharthachatterjee changed the title Safari Downloading multiple resolutions fix(gatsby-image): Fix issue where Safari downloads multiple resolutions Feb 20, 2019
@sidharthachatterjee
Copy link
Contributor

@tbrannam Looks good but want to quickly clarify

I can't seem to reproduce this issue on https://using-gatsby-image.gatsbyjs.org/prefer-webp/

Maybe I'm misunderstanding?
screenshot 2019-02-21 02 15 16

@tbrannam
Copy link
Contributor Author

tbrannam commented Feb 20, 2019 via email

@sidharthachatterjee
Copy link
Contributor

Away from computer at the moment.

Safari 12 on macOS Mojave

Can tell you exact minor versions a little later

@tbrannam
Copy link
Contributor Author

tbrannam commented Feb 20, 2019

I'm on High Sierra still but using Safari 12.0.3. I've spot checked Mojave on a colleague's machine and was able to reproduce the same. Additionally we have reproduced this same issue on Safari - iPhone X 12.1.2

screen shot 2019-02-20 at 5 15 47 pm

@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Feb 20, 2019

So strange. I still can't reproduce this. Check out versions in the screenshot.

screenshot 2019-02-21 04 11 14

@tbrannam
Copy link
Contributor Author

This is disappointing - I cannot see a reason why we are getting different results. I have run out of test devices to try to narrow it down.

I've also tried in 'private window' as well as well as bypass cache in the inspector to try to vary the behavior - but they all seemed to give me the same results.

@sidharthachatterjee
Copy link
Contributor

@tbrannam Yeah, really strange.

Let's keep this open and see if the others can reproduce in the mean time. cc @DSchau @pieh

Also, is there value in merging in this anyway irrespective of this particular bug you're seeing?

@pieh
Copy link
Contributor

pieh commented Feb 21, 2019

I reproduced this locally with very weird findings - for some reason I saw 2 different sizes being downloaded ... only if safari window was in certain width range - somewhere between 720 and 760 px wide (below and above that it was just grabbing single "pug" image) - it might happen in more scenarios, but I was happy enough I could reliably reproduce that and didn't search for more. Also verified that the changes in the PR fixes it, but didn't yet look through the code changes here.

@tbrannam
Copy link
Contributor Author

@pieh great callout

<picture>
<source 
 srcSet="/static/3b314577d3b8dd16b97a6b23c068b374/91f71/7b214c41dead6dff0f496c55d52769d2.jpg 180w,
/static/3b314577d3b8dd16b97a6b23c068b374/a4889/7b214c41dead6dff0f496c55d52769d2.jpg 360w,
/static/3b314577d3b8dd16b97a6b23c068b374/044eb/7b214c41dead6dff0f496c55d52769d2.jpg 720w,
/static/3b314577d3b8dd16b97a6b23c068b374/bd259/7b214c41dead6dff0f496c55d52769d2.jpg 1080w,
......" sizes="(max-width: 720px) 100vw, 720px" />
<img src="/static/3b314577d3b8dd16b97a6b23c068b374/044eb/7b214c41dead6dff0f496c55d52769d2.jpg"  ..... />
</picture>

/static/3b314577d3b8dd16b97a6b23c068b374/044eb/7b214c41dead6dff0f496c55d52769d2.jpg is the url for <img src> as well as the of <source srcset>

So the failcase would only occur if the viewport fell outside that breakpoint.

@sidharthachatterjee
Copy link
Contributor

@tbrannam That makes sense. Merging this in.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tbrannam 👍

@sidharthachatterjee sidharthachatterjee merged commit 29e572e into gatsbyjs:master Feb 25, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby-image@2.0.30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants