-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update Picture.astro so it no longer passes height and width attributes #4797
Conversation
the image variable of getPicture contains a width and height property, which we usually require. In this case, the image is wrapped in a picture tag and the img tag itself should not have a width and height property as this will break the responsiveness of the image provided by the picture tag.
🦋 Changeset detectedLatest commit: 1db96b1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Won't not having a width and height on the image cause a layout shift when the image loads in? |
You are correct; this will cause a layout shift. I have no idea how to prevent this if you want to use the picture tag (or img tag, for that matter) for selecting responsive image sizes. When keeping the width/height tags, you could only replace the image with better quality, like a 2x for retina screens. As far as I know, the only way to prevent the layout shift would be by using js to set the correct width/height for the current breakpoint (which we are trying to avoid)... One of the most detailed articles I could find is https://stackoverflow.blog/2022/03/28/picture-perfect-images-with-the-modern-element/ which sadly does not handle the layout shift issue when not providing a width/height. Quite an exciting conundrum... |
Yeah that's definitely unfortunate...I can't find another example of an I hadn't noticed this one in our use of I just merged in main to see if tests pass, this should be able to go out with the next release that adds WASM support |
This can cause layout shift but it's still manageable with <Picture src={import('./hero.png') width={800} height={400} />
<style>
picture :global(img) {
@apply aspect-[2/1];
}
</style> |
Google is giving me warnings now that the img doesnt have a size anymore on https://web.dev/measure/. Anyone else experiencing this? |
@bronze You are warned about the possible layout shifts when using the img without width and height, which is already discussed above. Currently, it is not possible to optimize for responsiveness and cls at the same time. |
I wonder what “responsiveness” issues you were running into. @smeevil It’s currently not possible to do art direction with the Picture component. The only reason the component outputs multiple You shouldn’t use All images variants the components serve will have the same aspect ratio, the one you request via the Once art direction is possible the attributes should be output with the corresponding https://developer.mozilla.org/en-US/docs/Web/HTML/Element/source |
@tony-sull could you please consider reverting this change? |
@tony-sull This is a breaking change for our page. Img needs the hight and width for fix image sizes. 90% of our images are fixed size and not responsive (but different resolutions, for different devices). Please revert this. See also Smashing Magazine wrote about this: https://www.smashingmagazine.com/2020/03/setting-height-width-images-important-again/ |
I'm open to reverting this one, gotta say I'm at a bit of a loss for solid specs on whether width/height should be included in a picture's We had to make a couple fixes on astro.build when this update was released, so I definitely know the pain being hit here! I'm wondering if the |
When doing art direction you’d want to add the width and height attributes to each individual Example from the MDN article you shared might look like so with attributes added. The source for smaller viewports outputs a 4:3 image and the one for large viewports outputs in 16:9. <picture>
<source media="(max-width: 799px)" width="800" height="600" srcset="elva-480w-close-portrait.jpg" />
<source media="(min-width: 800px)" width="800" height="450" srcset="elva-800w.jpg" />
<img src="elva-800w.jpg" alt="Chris standing up holding his daughter Elva" />
</picture> Support to add the attributes to Specs: https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element The thing is that Astro’s Picture component doesn’t have a way to do art direction. So until this is a thing it’d probably be fine to just add width and height to the Regarding CSS I would actually appreciate if the components wouldn’t output any styles or classes. But I don’t know if that’s feasible or practical. |
@tony-sull I just found another resource where they recommend adding width and height attributes to the |
According to the spec, the width and height on an img tag are just the intrinsic size of that image, it does not tell the user agent, how to render the image. Usually the UAs just derive the aspectRatio from it, if not otherwise specified. @smeevil does not state, what the actual problem is. The referenced article explicitly states, that it is best practice to have width and height on your img and it does not say, you shouldn't on picturized images. As far as I could find, all major browsers will handle responsive images just fine, even if the dimensions on the img are given. As @carlcs states, it's now also possible to set width and height on the source element. This is already supported by all major browsers except Firefox, but at the end it just overrides the dimensions from the img tag. Hence, I would urgently request to undo that change, until we have some good proof, that it is bad practice to add dimensions to responsive images. Currently, it renders the Picture component useless and I have to implement my own based on the getPicture method. Maybe @smeevil could implement his own very naive wrapper around the Picture component, that removes those attributes. Adding them is much harder, because they are a result of the transformation. |
Due to Github PR withastro/astro#4797, the Picture component removes width and height from the embedded img. That causes massive layout shifts. The patched component reverts that change until it is reverted in the Astro repo.
Any plans to revert this? I'm facing CLS when using the Picture element since width and height aren't being included automatically. |
Just following up to see if there are any plans to revert this, as it is causing CLS. Also, they way to the fix the issue with the image not being responsive due to the set width and height is to set an override in CSS. The width and Height on the image is used by the browser to draw out the space before the image is loaded. Adding something like this, will then allow the image to scale as needed.
|
Changes
The image variable of getPicture contains a width and height property, which we usually require. In this case, the image is wrapped in a picture tag, and the img tag itself should not have a width and height property as this will break the responsiveness of the image provided by the picture tag.
Testing
The picture component now renders the img tag without the width and height attributes.
I'm not sure how you would like to adjust the test files and if this is the correct way you would like to solve this issue.
Docs
This has no impact on the docs as everything stated about the element is correct.