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

sizes needs to be on the source-tag #7997

Merged
merged 3 commits into from
Dec 9, 2022
Merged

Conversation

PatrickG
Copy link
Member

@PatrickG PatrickG commented Dec 8, 2022

The sizes attribute needs to be on the source-tag.
See source vs picture attributes.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Dec 8, 2022

⚠️ No Changeset found

Latest commit: f8c864f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Dec 8, 2022

@PatrickG is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@Rich-Harris
Copy link
Member

Thank you. The sizes themselves were based on an old layout, so I've updated those. For the life of me though, I can't get the browser to actually load the 960px-wide image. It loads the 1440px-wide one every time, even on tiny screens. Any idea what's going on?

@eltigerchino
Copy link
Member

eltigerchino commented Dec 9, 2022

For the life of me though, I can't get the browser to actually load the 960px-wide image. It loads the 1440px-wide one every time, even on tiny screens. Any idea what's going on?

Could it be related to retina screens? This would cause the browser to load the higher res images.

@Rich-Harris
Copy link
Member

That was my thought too, but I tried crazy breakpoints like 3000px and still nothing :/

{#each Object.entries(background.sources) as [format, images]}
<source
sizes="(min-width: 768) 1440px, 960px"
Copy link
Member

@eltigerchino eltigerchino Dec 9, 2022

Choose a reason for hiding this comment

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

- sizes="(min-width: 768) 1440px, 960px"
+ sizes="(min-width: 768px) 1440px, 960px"

After adding the missing px, I've observed that Chrome, Safari, and Firefox all have different behaviours for this (tested on MacOS).

1. Firefox is the only browser that correctly implements this.

  • Respects the sizes attribute.
  • Changes the image when resizing the window.

2. Safari.

  • Respects the sizes attribute.
  • Does not change the image when resizing the window (it only loads the correct image on initial page load).

3. Chrome

  • Does not respect the sizes attribute (always loads the largest image).
  • Removing the sizes attribute enables srcset to work correctly (loads the next largest image on resize).

Would be great if someone else could verify these behaviours too.

Copy link
Member Author

@PatrickG PatrickG Dec 9, 2022

Choose a reason for hiding this comment

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

I can not test on Safari, but it seems Firefox behaves similarly to Chrome for me. It always loads the largest if the sizes attribute is present.

Removing the sizes attribute enables srcset to work correctly (loads the next largest image on resize).

I would not say "correctly", but at least something happens.
Without the sizes attribute, Chrome switches to the large image when the screen width is

  • >= 961px (DPR: 1.0)
  • >= 588px (DPR: 2.0)
  • >= 392px (DPR: 3.0)

while Firefox switches to the large image when the screen width is

  • >= 961px (DPR: 1.0)
  • >= 481px (DPR: 2.0)
  • >= 321px (DPR: 3.0)

with this import ./svelte-kit-machine.webp?w=960;1440&format=avif;webp;png&picture

@PatrickG
Copy link
Member Author

PatrickG commented Dec 9, 2022

I'm not sure, but I think the images in srcset must be ordered from smallest to largest.
It is very strange.
I would suggest removing the sizes attribute. Then at least mobile phones should load the smaller image.

@Rich-Harris
Copy link
Member

Thank you @PatrickG and @s3812497 for the thorough investigation — agree that removing sizes is probably the best solution.

I shouldn't complain — I'm old enough to remember IE6 — but the web platform continues to be a cruel prank. And they wonder why people turn to JavaScript instead of uSinG tHe PlAtfOrM

@Rich-Harris Rich-Harris merged commit 4d2b811 into sveltejs:master Dec 9, 2022
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.

3 participants