1670151 - image width should be aspect ratio * height when width: max-content

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[dgrogan (chrome)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36

Steps to reproduce:

<img src="https://placehold.it/60x60" style="height: 200px; width: max-content;">

Actual results:

width of the image is 60px

Expected results:

width of the image should be 200px

This is based on

https://drafts.csswg.org/css-sizing-3/#sizing-values ->
https://drafts.csswg.org/css-sizing-3/#min-content ->
https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes ->
https://www.w3.org/TR/CSS2/visudet.html#float-replaced-width ->
https://www.w3.org/TR/CSS2/visudet.html#inline-replaced-width ->

"""
if 'width' has a computed value of 'auto', 'height' has some other computed value, and the element does have an intrinsic ratio; then the used value of 'width' is:

(used height) * (intrinsic ratio)
"""

Chrome has the same bug
https://bugs.chromium.org/p/chromium/issues/detail?id=1135287

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Assuming typo in the title per the contents of comment 0.

if 'width' has a computed value of 'auto', 'height' has some other computed value, and the element does have an intrinsic ratio; then the used value of 'width' is...

Hmm, but if height is a percentage doesn't that mean that we need to do layout before computing intrinsic sizes? That seems wrong.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I guess we deal with it somehow. I seem to recall we try to figure out percentages etc as we do this. Ok, the key here is that https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes says that:

The max-content size of a box in each axis is the size it would have if it was a float given an auto size in that axis.

And auto is defined to behave like that (though it's not very precise because it talks about computed values, and "given an auto size" isn't also very clear)... But I guess the intention of the spec is clear enough and it should behave exactly as data:text/html,<div style="float: left"><img src="https://placehold.it/60x60" style="height: 200px;"></div>.

Comment 3

[dgrogan (chrome)]

There's a very similar case that Blink and Firefox incorrectly display identically. Both engines give this a 300x50 size but it should be 50x50.

<img src="https://placekitten.com/300/300" style="max-height: 50px; width: max-content;">

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1670151 - Handle {min|max|fit}-content well for aspect-ratio on replaced elements.

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1670151 - Fix image-min-max-content-intrinsic-size-change-00*.html to avoid using definite block-size.

If we have definite block-size and a fintie intrinsic ratio, we compute
the min-content/max-content by these two value, instead of using the
natural size of the image. We have to update these wpts.

Comment 6

[Boris Chiou [:boris]]

Will send review requests after landing Bug 1646100.

Comment 7

[Boris Chiou [:boris]]

Still thinking about what should we do about this bug. Given the comment from Tab #5023, if we use the preferred aspect-ratio (i.e. the natural ratio here) and the block size to compute the min-content/max-content inline size of the replaced elements, we are missing the ability to use the natural size of the image in one axis. Perhaps we should wait for the discussion of CSSWG because they may introduce new keywords to represent the natural size for replaced elements.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1670151 Part 1 - Rename ComputeInlineSizeFromAspectRatio() to ComputeISizeValueFromAspectRatio(). r?#layout

This renaming is to match the existing ComputeISizeValue() and
ComputeBSizeValue() APIs. Also, rename the related local variables for brevity
and improve the comments.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1670151 Part 2 - Use nsIFrame::GetAspectRatio() to query aspect-ratio rather than obtaining it from style. r?#layout

GetAspectRatio() considers both the aspect-ratio property and the intrinsic
aspect-ratio of replaced elements.

Changing just nsIFrame::ComputeISizeValueFromAspectRatio() fixed the intrinsic
size for replaced elements when there is a definite height. However, it doesn't
pass any tests on wpt, so I wrote intrinsic-size-017.html to
intrinsic-size-019.html to cover this.

Modifying nsLayoutUtils::IntrinsicForAxis() fixed the intrinsic size
contribution for replaced elements. We already have the logic to use inline size
from aspect ratio in AddIntrinsicSizeOffset() and GetIntrinsicCoord(). We
just need to compute it when sizes in the inline axis have intrinsic keywords.
intrinsic-size-020.html to intrinsic-size-025.html cover this.

grid-auto-min-sizing-min-content-min-size-{001,002}-ref.html are also modified
to reflect this new behavior. The modified subtests are:

• 001.html: Test 1, 2, 3, 6, 8, 9
• 002.html: Test 1, 2, 3, 6, 7, 9

In these modified subtests, before this patch, our rendering was all different
from Google Chrome's. After this patch, our behavior aligns with Google Chrome,
except for Test 7 and 9 in 002.html.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cdbcc5902b3
Part 1 - Rename ComputeInlineSizeFromAspectRatio() to ComputeISizeValueFromAspectRatio(). r=layout-reviewers,boris
https://hg.mozilla.org/integration/autoland/rev/cb56506b10a7
Part 2 - Use nsIFrame::GetAspectRatio() to query aspect-ratio rather than obtaining it from style. r=dholbert

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46643 for changes under testing/web-platform/tests

Comment 12

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/6cdbcc5902b3
https://hg.mozilla.org/mozilla-central/rev/cb56506b10a7

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot