-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Set width and height attributes as an aspect ratio for images #937
Comments
What exactly needs to be changed on our end? |
I will provide a pull request once we agreed on bugfix vs. feature 🙃 |
I guess this is a bugfix. |
Bugfix |
Would it break something if we released it now? |
It shouldn’t break something in general IMO. There might be some edge cases with using the I’d still wait for the first released browser version to be more certain that this new behavior stays, and so that we can test our implementation. |
I did find a problem. Consider the following setup: Before, the output of this image size was <img src="…" srcset="… 400w, … 800w" sizes="(max-width: 780px) 400px, 800px" alt="" itemprop="image"> Now, the output is <img src="…" srcset="… 400w, … 800w" sizes="(max-width: 780px) 400px, 800px" alt="" itemprop="image" width="400" height="267"> This causes the image to be shown like this: Instead of this: And there seems to be no correct solution for this, because if you turn the values around like this: The Browser will blow up the 400px image to 800px (or less, within its parent) if the viewport is 780px or lower. |
@contaoacademy in your case there should be no problem, regardless of what you actually put into the density/scaling. |
My old setting was "Breite" 320. |
Yes, that causes the problem in conjunction with your |
@fritzmg If you set the width to Can you please also try if |
Not if the What does the specification say about an image with the following markup: <img src="…" srcset="…" sizes="(max-width: 780px) 400px, 800px" width="400" height="200"> https://jsfiddle.net/v5xj9krz/ Should it be displayed with a width of
Seems to have no effect: https://jsfiddle.net/3g4evour/ |
It should work with the correct So I think adding |
Yeah, you are right 😃 Imho this change should be reverted and added to |
I’m not sure if we should revert it. IMO using As the fix is very simple by adding |
I use it all the time and I was unaware that this is not recommended. To be fair, it won't affect any of our installations, as our CSS base includes |
Imho this change causes too many problems for a bugfix release and seems unnecessary with |
@contao/developers What are your opinions on this one? |
I’m OK with both (keeping as is or revert the change) with a slight tendency to keeping it as is. |
I am with @ausi on this one. |
What are |
To tell the browser which resource to load. |
ah, well that was a synonym for "with of the image" to me. 👍 |
Can you elaborate on this? To me it seems like a perfectly valid use-case, and it is supported by the HTML standard as well, is it not? |
The spec writes “The The examples in the spec state: “In this example, a banner image takes up the entire viewport width (using appropriate CSS).” and “These sizes do not necessarily have to match up exactly with the actual image width as specified in the CSS.” I couldn’t find a specific statement that says something like “The
The HTML standard tells browsers to convert If my interpretation of the spec is wrong here I would still favor the solution using |
As discussed on Mubmle: We (I) should adapt the help text for the |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is on my todo list. |
Done in #1807 |
Description ----------- | Q | A | -----------------| --- | Fixed issues | Fixes #937 (comment) The new help text now reads as follows: > The HTML attribute <code>sizes</code> defines the intended layout width of the image, optionally combined with a media query. You can use any CSS length value in this attribute.<br><br>E.g. <code>(max-width: 600px) 100vw, 50vw</code> means the images width is 100% of the viewport for small screens and 50% of the viewport for larger screens.<br><br>And <code>(max-width: 600px) calc(100vw - 20px), 500px</code> means the images width is 20px smaller than the viewport for small screens and 500px for larger screens.<br><br>The sizes attribute shouldn’t be used for styling, use CSS instead. The sizes attribute does not necessarily have to match up exactly with the actual image width as specified in the CSS.<br><br>For more information about the sizes attribute please visit <a href="https://www.w3.org/TR/2016/PR-html51-20160915/semantics-embedded-content.html#element-attrdef-img-sizes" target="_blank" rel="noreferrer noopener">w3.org</a>. Commits ------- 2828003 Improve the sizes attribute help text
Description ----------- | Q | A | -----------------| --- | Fixed issues | Fixes contao/contao#937 (comment) The new help text now reads as follows: > The HTML attribute <code>sizes</code> defines the intended layout width of the image, optionally combined with a media query. You can use any CSS length value in this attribute.<br><br>E.g. <code>(max-width: 600px) 100vw, 50vw</code> means the images width is 100% of the viewport for small screens and 50% of the viewport for larger screens.<br><br>And <code>(max-width: 600px) calc(100vw - 20px), 500px</code> means the images width is 20px smaller than the viewport for small screens and 500px for larger screens.<br><br>The sizes attribute shouldn’t be used for styling, use CSS instead. The sizes attribute does not necessarily have to match up exactly with the actual image width as specified in the CSS.<br><br>For more information about the sizes attribute please visit <a href="https://www.w3.org/TR/2016/PR-html51-20160915/semantics-embedded-content.html#element-attrdef-img-sizes" target="_blank" rel="noreferrer noopener">w3.org</a>. Commits ------- 28280030 Improve the sizes attribute help text
Browsers now seem to change the behavior of how
width
andheight
attributes are handled in combination with responsive CSS, see whatwg/html#4952The new behavior seems to be the way that @fritzmg already suggested five years ago in contao/core#7500 (comment) which means you can use
width
andheight
now to define an aspect ratio for the image.To improve our usage of this feature in Contao we should now always use
width
andheight
attributes as long as all<source>
s have the same aspect ratio.This is a bugfix IMO because the behavior of browsers has changed.
For more details about the changes see https://bugzilla.mozilla.org/show_bug.cgi?id=1547231, https://bugs.chromium.org/p/chromium/issues/detail?id=979891, https://bugs.webkit.org/show_bug.cgi?id=201641, https://twitter.com/tomayac/status/1165902659083280384, https://www.youtube.com/watch?v=4-d_SoCHeWE.
There are also plans to add
width
andheight
support for<source>
tags in the future, see whatwg/html#4968The text was updated successfully, but these errors were encountered: