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

Set width and height attributes as an aspect ratio for images #937

Closed
ausi opened this issue Nov 6, 2019 · 31 comments
Closed

Set width and height attributes as an aspect ratio for images #937

ausi opened this issue Nov 6, 2019 · 31 comments
Assignees
Labels
Milestone

Comments

@ausi
Copy link
Member

ausi commented Nov 6, 2019

Browsers now seem to change the behavior of how width and height attributes are handled in combination with responsive CSS, see whatwg/html#4952

The 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 and height now to define an aspect ratio for the image.

To improve our usage of this feature in Contao we should now always use width and height 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 and height support for <source> tags in the future, see whatwg/html#4968

@leofeyer
Copy link
Member

leofeyer commented Nov 6, 2019

What exactly needs to be changed on our end?

@ausi
Copy link
Member Author

ausi commented Nov 6, 2019

To improve our usage of this feature in Contao we should now always use width and height attributes as long as all <source>s have the same aspect ratio.

I will provide a pull request once we agreed on bugfix vs. feature 🙃

@leofeyer
Copy link
Member

leofeyer commented Nov 6, 2019

I guess this is a bugfix.

@Toflar
Copy link
Member

Toflar commented Nov 6, 2019

Bugfix

@ausi
Copy link
Member Author

ausi commented Nov 6, 2019

Pull requests: #940 (4.8) #939 (4.8)

We should probably wait until one of the browsers released a stable version with this feature activated. Firefox 71 will be released in december AFAIK.

@leofeyer
Copy link
Member

leofeyer commented Nov 8, 2019

Would it break something if we released it now?

@ausi
Copy link
Member Author

ausi commented Nov 8, 2019

It shouldn’t break something in general IMO. There might be some edge cases with using the sizes attribute, but they should be easy to fix with CSS I think.

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.

leofeyer pushed a commit that referenced this issue Dec 9, 2019
Description
-----------

As discussed in #937

Commits
-------

cdb158c Set img width/height attributes as an aspect ratio
4560027 Move the aspect ratio detection to the picture factory
22b9e15 Fix the coding style (#1076)
@fritzmg
Copy link
Contributor

fritzmg commented Dec 18, 2019

I did find a problem. Consider the following setup:

Screenshot_2019-12-18 Themes › Contao Official Demo › Image sizes › Edit image size ID 1 demo contao org(2)

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:

Screenshot_2019-12-18 Content Elements - Contao Official Demo

Instead of this:

Screenshot_2019-12-18 Content Elements - Contao Official Demo(1)

And there seems to be no correct solution for this, because if you turn the values around like this:

Screenshot_2019-12-18 Themes › Contao Official Demo › Image sizes › Edit image size ID 1 demo contao org(3)

The Browser will blow up the 400px image to 800px (or less, within its parent) if the viewport is 780px or lower.

@contaoacademy
Copy link

I also noticed the same behavior yesterday after the update to 4.8.7.

At the moment I have helped myself as follows:
pixel

@fritzmg
Copy link
Contributor

fritzmg commented Dec 18, 2019

@contaoacademy in your case there should be no problem, regardless of what you actually put into the density/scaling.

@contaoacademy
Copy link

My old setting was "Breite" 320.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 18, 2019

My old setting was "Breite" 320.

Yes, that causes the problem in conjunction with your sizes attribute. The density/scaling is irrelevant.

@leofeyer leofeyer added this to the 4.4 milestone Dec 18, 2019
@ausi
Copy link
Member Author

ausi commented Dec 18, 2019

@fritzmg If you set the width to 400 isn’t it correct if it is shown with that width in the front end? I think you have to set width: 100%; height: auto; for your image in that case.

Can you please also try if width: auto; height: auto; would fix your issue?

@fritzmg
Copy link
Contributor

fritzmg commented Dec 18, 2019

@fritzmg If you set the width to 400 isn’t it correct if it is shown with that width in the front end

Not if the sizes attribute says otherwise, at least that is what I would expect. The width and height attribute are supposed to give the browser the correct aspect ratio information of the image, but the actual size should be determined by a multitude of factors.

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 800px or 400px, inside a viewport that is 781px or more? I expected it to be displayed with a width of 800px as defined in the sizes attribute. But it is actually displayed with a width of 400px.

Can you please also try if width: auto; height: auto; would fix your issue?

Seems to have no effect: https://jsfiddle.net/3g4evour/

@ausi
Copy link
Member Author

ausi commented Dec 19, 2019

Can you please also try if width: auto; height: auto; would fix your issue?

Seems to have no effect: jsfiddle.net/3g4evour

It should work with the correct srcset: https://jsfiddle.net/jmvq1sc8/

So I think adding img { width: auto; height: auto; } to your CSS would be the correct solution for your use case. I’m not sure if we can or should inject this CSS automatically.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 20, 2019

Yeah, you are right 😃

Imho this change should be reverted and added to 4.9 instead. Plus a mention in the UPGRADE.md.

@ausi
Copy link
Member Author

ausi commented Dec 21, 2019

I’m not sure if we should revert it. IMO using sizes to set the width of an image is not recommended and probably doesn’t affect too much installations.

As the fix is very simple by adding img { width: auto; height: auto; } I think the improvement in performance outweighs the small behavior change, don’t you think?

@fritzmg
Copy link
Contributor

fritzmg commented Dec 21, 2019

I’m not sure if we should revert it. IMO using sizes to set the width of an image is not recommended and probably doesn’t affect too much installations.

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 img { width: auto; height: auto; } anyway but still.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 21, 2019

As the fix is very simple by adding img { width: auto; height: auto; } I think the improvement in performance outweighs the small behavior change, don’t you think?

Imho this change causes too many problems for a bugfix release and seems unnecessary with 4.9 right around the corner. At least with 4.9 we can properly advertise the change.

@leofeyer
Copy link
Member

@contao/developers What are your opinions on this one?

@ausi
Copy link
Member Author

ausi commented Dec 22, 2019

I’m OK with both (keeping as is or revert the change) with a slight tendency to keeping it as is.

@leofeyer
Copy link
Member

I’m not sure if we should revert it. IMO using sizes to set the width of an image is not recommended and probably doesn’t affect too much installations.

I am with @ausi on this one.

@leofeyer leofeyer removed this from the 4.4 milestone Feb 3, 2020
@aschempp
Copy link
Member

aschempp commented Feb 7, 2020

I’m not sure if we should revert it. IMO using sizes to set the width of an image is not recommended and probably doesn’t affect too much installations.

What are sizes recommended for then?

@ausi
Copy link
Member Author

ausi commented Feb 8, 2020

What are sizes recommended for then?

To tell the browser which resource to load.

@aschempp
Copy link
Member

ah, well that was a synonym for "with of the image" to me. 👍

@fritzmg
Copy link
Contributor

fritzmg commented Feb 12, 2020

IMO using sizes to set the width of an image is not recommended and probably doesn’t affect too much installations.

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?

@ausi
Copy link
Member Author

ausi commented Feb 12, 2020

The spec writes “The <source-size-value> gives the intended layout width of the image.”

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 sizes attribute shouldn’t be used for styling, use CSS instead.” but I think this can inferred from the examples in the specification.

it is supported by the HTML standard as well, is it not?

The HTML standard tells browsers to convert w descriptors to x descriptors by dividing it by the size from the sizes attribute. Technically this results in the density-corrected intrinsic width being the same as the size from sizes. But I don’t think this means setting the width using sizes is supported, also because it only works with px values and not with a responsive/fluid design.

If my interpretation of the spec is wrong here I would still favor the solution using img { width: auto; height: auto; } because of the performance benefits.

@ausi
Copy link
Member Author

ausi commented Feb 13, 2020

As discussed on Mubmle: We (I) should adapt the help text for the sizes attribute to point out that it should not be used for sizing the image, but only tell the browser which resource it should load.

@stale
Copy link

stale bot commented Jun 9, 2020

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.

@stale stale bot added the stale label Jun 9, 2020
@ausi
Copy link
Member Author

ausi commented Jun 10, 2020

This is on my todo list.

@stale stale bot removed the stale label Jun 10, 2020
@leofeyer leofeyer added this to the 4.4 milestone Jun 10, 2020
@ausi
Copy link
Member Author

ausi commented Jun 10, 2020

We (I) should adapt the help text for the sizes attribute to point out that it should not be used for sizing the image, but only tell the browser which resource it should load.

Done in #1807

@ausi ausi closed this as completed Jun 10, 2020
leofeyer pushed a commit that referenced this issue Jun 11, 2020
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
leofeyer pushed a commit to contao/core-bundle that referenced this issue Jun 11, 2020
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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants