Bug 201641 – Default <img> aspect ratio from size attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[cathiechen]

After the aspect ratio determined, we can compute the layout size of img/video in advance without waiting for loading.

Details: https://github.com/WICG/intrinsicsize-attribute/issues/16

[Radar WebKit Bug Importer]

<rdar://problem/55223958>

[Frédéric Wang (:fredw)]

"Intent to ship" emails have been sent to mozilla and blink developer mailing lists.

[Frédéric Wang (:fredw)]

Bug 197739 has an experimental patch for the legacy "intrinsicsize attribute" proposal and can probably be reused here.

[Frédéric Wang (:fredw)]

Chromium devs are retracted the intent to ship for now:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/CQVTVOYx1XI/o7Fj1mmBAQAJ

New thread: https://github.com/w3c/csswg-drafts/issues/4531

[Emilio Cobos Álvarez (:emilio)]

That is a completely different thing. That's about an intrinsic-size CSS property, that did something fairly different.

[Manuel Rego Casasnovas]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> That is a completely different thing. That's about an intrinsic-size CSS
> property, that did something fairly different.

Yes, I agree with Emilio here.

This has been fixed and shipped in Firefox and Chromium:
* https://bugzilla.mozilla.org/show_bug.cgi?id=1547231
* https://bugs.chromium.org/p/chromium/issues/detail?id=979891

The spec text can be found at:
https://html.spec.whatwg.org/multipage/rendering.html#attributes-for-embedded-content-and-images
  "
   The intrinsic aspect ratio for an img element img is computed as follows:
     1. If img's current request is available and has an intrinsic aspect ratio, then use that intrinsic aspect ratio.
     2. If img's width and height attribute values, when parsed using the rules for parsing dimension values, are both not an error, not a percentage, and non-zero, then use the ratio resulting from dividing the width attribute value by the height attribute value.
     3. Otherwise, img has no intrinsic aspect ratio.
  "

[Frédéric Wang (:fredw)]

@Emilio, Rego: Thanks for clarifying and sorry for the confusion!

[cathiechen]

Created attachment 386082 [details]
Patch

[cathiechen]

Created attachment 386086 [details]
Patch

[cathiechen]

Created attachment 386087 [details]
Patch

[cathiechen]

Created attachment 386088 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 386088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386088&action=review

> Source/WebCore/page/Settings.yaml:916
> +aspectRatioFromWidthAndHeightEnabled:
> +  initial: false
> +

Do we really need a runtime flag for this?

AFAIK this has already shipped in Chromium and Firefox (https://www.chromestatus.com/feature/5695266130755584)
so this change will just align WebKit with them.

[Frédéric Wang (:fredw)]

Comment on attachment 386088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386088&action=review

>> Source/WebCore/page/Settings.yaml:916
>> +
> 
> Do we really need a runtime flag for this?
> 
> AFAIK this has already shipped in Chromium and Firefox (https://www.chromestatus.com/feature/5695266130755584)
> so this change will just align WebKit with them.

That was also the case for bug 182053 and I was asked to do it under a preference flag. I guess it really depends on whether this is going to cause backward compatibility issue, which is something Apple is concerned about, even when if that improves interoperability. I'll let Apple people comment, but I guess it's safe to keep the flag for now...

[cathiechen]

Created attachment 386100 [details]
Patch

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 386100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386100&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:447
> +        if (!node || !node->hasAttribute(HTMLNames::widthAttr) || !node->hasAttribute(HTMLNames::heightAttr))

What guarantees that this is an <img> and not a <div style="content: url(..)"> for example?

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/canvas-aspect-ratio.html:4
> +<script src="/resources/testharnessreport.js"></script>

What in the spec says that this should be used for <canvas> and <video>? The spec only mentions <img> as far as I can tell.

[Said Abou-Hallawa]

Comment on attachment 386100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386100&action=review

> Source/WebCore/rendering/RenderReplaced.cpp:442
> +    if (intrinsicSize.isEmpty() || !hasAspectRatio()) {

bool hasAspectRatio() const { return isReplaced() && (isImage() || isVideo() || isCanvas()); }

So I think it is wrong to run this calculation if hasAspectRatio() returns false. I think also the following check should be moved to the beginning of this function:

    if (!hasAspectRatio())
        return;

> Source/WebCore/rendering/RenderReplaced.cpp:450
> +        intrinsicSize.setWidth(parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::widthAttr)).valueOr(0));
> +        intrinsicSize.setHeight(parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0));

Why do you need to parse the attributes? Can't you use something like this:

Length width = style().width();
Length height = style().height();

if (width.isFixed() && height.isFixed())
     intrinsicSize = { width.value(), height.value() };

Also do we need to consider the writing mode to in the new width and height calculation? Have a look at RenderBox::intrinsicLogicalWidth().

[Maciej Stachowiak]

Comment on attachment 386100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386100&action=review

> Source/WebCore/ChangeLog:21
> +        * rendering/RenderReplaced.cpp:
> +        (WebCore::RenderReplaced::computeIntrinsicRatioInformation const):

The discussion I saw of this suggested it could be done in pure CSS (using an aspect-ratio CSS property that would be applied to all images via the UA stylesheet) but this does it in C++ code instead. Why is that? The CSS solution seemed so elegant.

[Maciej Stachowiak]

Specifically, the issue. suggests adding this to the UA stylesheet:

img, video {
    aspect-ratio: attr(width) / attr(height);
} 

I think implementing the aspect-ratio property. (if we don't have it already) and adding that to the UA stylesheet would be better than a hardcoded C++ implementation.

Presence of the property is an observable difference so for the sake of interop we should do it the same way as other browsers (assuming they are using this method.

[Emilio Cobos Álvarez (:emilio)]

(In reply to Maciej Stachowiak from comment #18)

What other browsers do is a bit more complicated, and it's not observable.

Firefox does use an internal aspect-ratio CSS property, and Blink uses something similar to this.

The behavior is different from what an eventual proper aspect-ratio CSS property would do, as aspect-ratio would override the intrinsic aspect-ratio of the image, and we found that to be not compatible. See the duplicates of https://bugzilla.mozilla.org/show_bug.cgi?id=1565690.

What Blink and Gecko implement is here: https://html.spec.whatwg.org/#attributes-for-embedded-content-and-images

That reads:

> The intrinsic aspect ratio for an img element img is computed as follows:
>    If img's current request is available and has an intrinsic aspect ratio, then use that intrinsic aspect ratio.
>
>    If img's width and height attribute values, when parsed using the rules for parsing dimension values, are both not an error, not a percentage, and non-zero, then use the ratio resulting from dividing the width attribute value by the height attribute value.
>
>    Otherwise, img has no intrinsic aspect ratio.

Note that it restricts it to <img> elements, not arbitrary elements like this patch. That is intentional, for now. See https://github.com/whatwg/html/issues/4961 and https://github.com/whatwg/html/issues/4968 for known follow-ups to this.

Please don't do what this patch does because it is totally non-conforming as far as I can tell.

[Emilio Cobos Álvarez (:emilio)]

(Not observable from the computed style, I mean. It is of course observable)

[Simon Fraser (smfr)]

Is there good WPT test coverage for this? Perhaps Cathie's first step should be importing some WPT tests.

[Emilio Cobos Álvarez (:emilio)]

(In reply to Simon Fraser (smfr) from comment #21)
> Is there good WPT test coverage for this? Perhaps Cathie's first step should
> be importing some WPT tests.

Yes, there are multiple tests here: http://w3c-test.org/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/ (including tests for it _not_ applying to <video> / <canvas>.

It seems like Cathie imported them, though in the posted patch they're failing...

There's no test for it not applying to <div style="content: url()" width="" height=""></div>, I guess that'd be useful.

[Frédéric Wang (:fredw)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)
> Yes, there are multiple tests here:
> http://w3c-test.org/html/rendering/replaced-elements/attributes-for-embedded-
> content-and-images/ (including tests for it _not_ applying to <video> /
> <canvas>.

@Emilio: I haven't really followed closely the discussion, so sorry for the silly questions... The original intrinsicsize attribute applied to several elements. I saw that Mozilla's intent-to & spec change was only for <img> and you are now mentioning tests that explicitly prevent it for other elements. Do you know why it was decided that way at the end? Google's intent-to thread, mentions both img and video so it seems there is interop issue here.

@Cathie: Did you check how it is currently implemented in Chromium?

[Manuel Rego Casasnovas]

(In reply to Frédéric Wang (:fredw) from comment #23)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #22)
> > Yes, there are multiple tests here:
> > http://w3c-test.org/html/rendering/replaced-elements/attributes-for-embedded-
> > content-and-images/ (including tests for it _not_ applying to <video> /
> > <canvas>.
> 
> @Emilio: I haven't really followed closely the discussion, so sorry for the
> silly questions... The original intrinsicsize attribute applied to several
> elements. I saw that Mozilla's intent-to & spec change was only for <img>
> and you are now mentioning tests that explicitly prevent it for other
> elements. Do you know why it was decided that way at the end? Google's
> intent-to thread, mentions both img and video so it seems there is interop
> issue here.

Despite the title of the intent included video, it was later clarified that only images were affected: https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/blink-dev/GePU9T8UpEc/6dRz0mgDCwAJ

Also there's this issue to extend it to other elements like videos: https://github.com/whatwg/html/issues/4961

So I believe Chromium and Firefox agree on this and are interoperable, check img-aspect-ratio.html and video-aspect-ratio.html at:
https://wpt.fyi/results/html/rendering/replaced-elements/attributes-for-embedded-content-and-images?label=experimental&label=master&aligned

[cathiechen]

Comment on attachment 386100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386100&action=review

Hi Emilio,

Thanks for the review.

>> Source/WebCore/rendering/RenderReplaced.cpp:447
>> +        if (!node || !node->hasAttribute(HTMLNames::widthAttr) || !node->hasAttribute(HTMLNames::heightAttr))
> 
> What guarantees that this is an <img> and not a <div style="content: url(..)"> for example?

Thanks for pointing this out and sorry, I haven't noticed this update. I will add a constraint to this and upload a test case for this to wpt.

>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/canvas-aspect-ratio.html:4
>> +<script src="/resources/testharnessreport.js"></script>
> 
> What in the spec says that this should be used for <canvas> and <video>? The spec only mentions <img> as far as I can tell.

This is imported from wpt, it seems we should update the script.
The default intrinsic size of canvas on WebKit is determined before layout, the value is "attr(width) x attr(height)", but it's not the result of "Canvas width and height attributes are used to infer aspect-ratio".

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25

There's two cases fail.
"assert_ratio(images[3], 1.266);" fails, because the result of getComputedStyle(img).width or height on WebKit will be round to an integer, while on Gecko and Blink it's float. This seems not deeply related aspect ratio.
"assert_equals(getComputedStyle(images[2]).height, "0px");" also fails, it seems there is a compatibility issue here.
Other test cases seem pass in my local env.

[Emilio Cobos Álvarez (:emilio)]

(In reply to cathiechen from comment #25)
> This is imported from wpt, it seems we should update the script.
> The default intrinsic size of canvas on WebKit is determined before layout,
> the value is "attr(width) x attr(height)", but it's not the result of
> "Canvas width and height attributes are used to infer aspect-ratio".

Yes, that sounds fine and what other browsers do. It may be the case that you just never hit the `intrinsicSize.isEmpty()` case for canvas and thus the test just passes.

> There's two cases fail.
> "assert_ratio(images[3], 1.266);" fails, because the result of
> getComputedStyle(img).width or height on WebKit will be round to an integer,
> while on Gecko and Blink it's float. This seems not deeply related aspect
> ratio.
> "assert_equals(getComputedStyle(images[2]).height, "0px");" also fails, it
> seems there is a compatibility issue here.

Fwiw I added that test recently, due to a compat issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1602047

The spec has no concept of "no current request" for images. So I'm glad that it prevented the compat issue with WebKit.

I'm surprised that the <video> passes, but it may also be the case that WebKit gives it an aspect ratio before reaching that code.

(In reply to Frédéric Wang (:fredw) from comment #23)
> @Emilio: I haven't really followed closely the discussion, so sorry for the
> silly questions... The original intrinsicsize attribute applied to several
> elements. I saw that Mozilla's intent-to & spec change was only for <img>
> and you are now mentioning tests that explicitly prevent it for other
> elements. Do you know why it was decided that way at the end? Google's
> intent-to thread, mentions both img and video so it seems there is interop
> issue here.

Yeah, we decided to restrict to <img> because:

 * It doesn't apply to canvas. Canvas' intrinsic size already depends on width and height already.

 * <video> is harder, because the intrinsic ratio may come from e.g., the poster image if no src is playing or what not (this comes from memory, so please take it with a grain of salt). It's not clear which of the multiple involved ratios should have preference, so we punted on it. But I'd be happy to implement that on Gecko provided the html spec is edited and unambiguous, I think it's something worth fixing, but probably a bit less important as most people use aspect ratio boxes using padding for <video> and such already.

[cathiechen]

Created attachment 386525 [details]
error_images

[cathiechen]

Sorry for the slow reply!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)
> (In reply to cathiechen from comment #25)
> > This is imported from wpt, it seems we should update the script.
> > The default intrinsic size of canvas on WebKit is determined before layout,
> > the value is "attr(width) x attr(height)", but it's not the result of
> > "Canvas width and height attributes are used to infer aspect-ratio".
> 
> Yes, that sounds fine and what other browsers do. It may be the case that
> you just never hit the `intrinsicSize.isEmpty()` case for canvas and thus
> the test just passes.
> 

According to https://html.spec.whatwg.org/multipage/canvas.html#attr-canvas-width, width and height attributes control the size of the element's bitmap. IIUC, width and height will infer the intrinsic size of canvas. I check the implement of WebKit and Blink(Sorry, I'm not familiar with Gecko...), the intrinsic size is determined before creating render object for HtmlCanvasElement. If the intrinsic size is determined, width and height attributes won't infer aspect-ratio.


> > "assert_equals(getComputedStyle(images[2]).height, "0px");" also fails, it
> > seems there is a compatibility issue here.
> 
> Fwiw I added that test recently, due to a compat issue:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1602047
> 
> The spec has no concept of "no current request" for images. So I'm glad that
> it prevented the compat issue with WebKit.
> 
Hmmm, it seems the behaviors of error images are quite different. I added a test for this, see "error_images" attachment. 
First, WebKit doesn't distinguish empty source or error source images, while Blink and Gecko do, however the behavior is not same. Not sure if the spec clarify this or not?
Second, how about <img>s with error source, should width and height attributes infer the aspect-ratio? If yes, the behavior <img src="./error.png"> and <img src="./error.png" width=100 height=125> will be different. If no, <img src="./error.png" width=100 height=125> will have content jump issue.

> I'm surprised that the <video> passes, but it may also be the case that
> WebKit gives it an aspect ratio before reaching that code.
On WebKit, video will have an intrinsic size before layout, see `RenderVideo::layout` for detail.

[cathiechen]

https://github.com/web-platform-tests/wpt/pull/20943

Added a test for div with style "content: url('/images/blue.png')".

The patch will be updated after this test landed. And we need to figure out how to handle the error images.

[Emilio Cobos Álvarez (:emilio)]

(In reply to cathiechen from comment #28)
> Hmmm, it seems the behaviors of error images are quite different. I added a
> test for this, see "error_images" attachment. 
> First, WebKit doesn't distinguish empty source or error source images, while
> Blink and Gecko do, however the behavior is not same. Not sure if the spec
> clarify this or not?

Yeah, this is one of the places where Blink doesn't follow anything near close to the spec. Their behavior is even more bizarre if you check quirks vs. non-quirks behavior.

I mailed them a while ago to try to make something sane out of this situation, but no dice. :(

I forwarded the email to you, just in case Igalia can help a bit with this...

[cathiechen]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #30)
> (In reply to cathiechen from comment #28)
> > Hmmm, it seems the behaviors of error images are quite different. I added a
> > test for this, see "error_images" attachment. 
> > First, WebKit doesn't distinguish empty source or error source images, while
> > Blink and Gecko do, however the behavior is not same. Not sure if the spec
> > clarify this or not?
> 
> Yeah, this is one of the places where Blink doesn't follow anything near
> close to the spec. Their behavior is even more bizarre if you check quirks
> vs. non-quirks behavior.
> 
> I mailed them a while ago to try to make something sane out of this
> situation, but no dice. :(
> 
> I forwarded the email to you, just in case Igalia can help a bit with this...


Thanks, I'll take a look.

Regarding the error images, IIUC, the intrinsic size of error images shouldn't be overridden by attribute aspect-ratio either. I added a test for this(https://github.com/web-platform-tests/wpt/pull/20991). Similar to empty src image, but the size of error images are different among browsers, I use another error images for referring.

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #21)
> Is there good WPT test coverage for this? Perhaps Cathie's first step should
> be importing some WPT tests.

Hi Simon,

We added some extra test cases for it, like content style images and error images, as discussed before.
The test cases will be split from this patch and imported by bug 205678.

[cathiechen]

Comment on attachment 386100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386100&action=review

Hi Said, Maciej,

Thanks for the review and sorry for the slow reply.

>> Source/WebCore/ChangeLog:21
>> +        (WebCore::RenderReplaced::computeIntrinsicRatioInformation const):
> 
> The discussion I saw of this suggested it could be done in pure CSS (using an aspect-ratio CSS property that would be applied to all images via the UA stylesheet) but this does it in C++ code instead. Why is that? The CSS solution seemed so elegant.

Like Emilio said, it seems it couldn't be done by pure CSS, for error aspect-ratios and error images.
And it seems aspect-ratio hasn't implemented yet.

>> Source/WebCore/rendering/RenderReplaced.cpp:442
>> +    if (intrinsicSize.isEmpty() || !hasAspectRatio()) {
> 
> bool hasAspectRatio() const { return isReplaced() && (isImage() || isVideo() || isCanvas()); }
> 
> So I think it is wrong to run this calculation if hasAspectRatio() returns false. I think also the following check should be moved to the beginning of this function:
> 
>     if (!hasAspectRatio())
>         return;

Done, thanks for pointing this out!

>> Source/WebCore/rendering/RenderReplaced.cpp:450
>> +        intrinsicSize.setHeight(parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0));
> 
> Why do you need to parse the attributes? Can't you use something like this:
> 
> Length width = style().width();
> Length height = style().height();
> 
> if (width.isFixed() && height.isFixed())
>      intrinsicSize = { width.value(), height.value() };
> 
> Also do we need to consider the writing mode to in the new width and height calculation? Have a look at RenderBox::intrinsicLogicalWidth().

Only HTML attributes width/height will affect the aspect-ratio. style().width() is result of html attributes and css, so we need to parse the HTML attributes here.

Thanks for pointing the writing mode out, I checked the spec again. It seems width/height attributs refer to physical dimension. And intrinsicSize here is also physical, it will be transfered to logical in RenderReplaced::computeAspectRatioInformationForRenderBox.

[cathiechen]

Created attachment 386834 [details]
Patch

[cathiechen]

Created attachment 386958 [details]
Patch

[cathiechen]

Comment on attachment 386958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386958&action=review

Hi,

This patch is ready for review. PTAL, thanks:)

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/canvas-aspect-ratio.html:1
> +<!doctype html><!-- webkit-test-runner [ experimental:AspectRatioFromWidthAndHeightEnabled=true ] -->

canvas-aspect-ratio.html, content-aspect-ratio.html and video-aspect-ratio.html test the attribute width/height shouldn't affect the aspect-ratio of canvas, content style image and video.
So these cases should be passed with or without experimental:AspectRatioFromWidthAndHeightEnabled.

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25

The result of getComputedStyle(img).width or height on WebKit are trimmed to an integer, so the aspect-ratio is 1.25. Gecko and Blink return a float value, so it's 1.266.
Other cases in this file are passed.

[Frédéric Wang (:fredw)]

Comment on attachment 386958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386958&action=review

> Source/WebCore/ChangeLog:8
> +        According to [1], if HTML width and height attribute have valid values, not a percentage, and non-zero,

attributes*

> Source/WebCore/rendering/RenderReplaced.cpp:455
> +        // We shouldn't override the aspect-ratio of empty src <img>.

"src <img>" => I guess you mean when the <img> element has an empty src attribute?

>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
>> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25
> 
> The result of getComputedStyle(img).width or height on WebKit are trimmed to an integer, so the aspect-ratio is 1.25. Gecko and Blink return a float value, so it's 1.266.
> Other cases in this file are passed.

So is it a webkit bug? Or a test bug?

[cathiechen]

Comment on attachment 386958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386958&action=review

Hi Fred,
Thanks for the review!

>> Source/WebCore/ChangeLog:8
>> +        According to [1], if HTML width and height attribute have valid values, not a percentage, and non-zero,
> 
> attributes*

Done

>> Source/WebCore/rendering/RenderReplaced.cpp:455
>> +        // We shouldn't override the aspect-ratio of empty src <img>.
> 
> "src <img>" => I guess you mean when the <img> element has an empty src attribute?

Done, thanks

>>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
>>> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25
>> 
>> The result of getComputedStyle(img).width or height on WebKit are trimmed to an integer, so the aspect-ratio is 1.25. Gecko and Blink return a float value, so it's 1.266.
>> Other cases in this file are passed.
> 
> So is it a webkit bug? Or a test bug?

It seems 1.266 is correct.

I followed the following spec train:
https://www.w3.org/TR/cssom-1/#dom-window-getcomputedstyle
https://www.w3.org/TR/cssom-1/#cssstyledeclaration-declarations
https://www.w3.org/TR/cssom-1/#resolved-value
https://drafts.csswg.org/css-cascade-4/#used-value

IIUC, the value of getComputedStyle(img).width or height is the "Used Value".
And the used value for height is defined here: https://www.w3.org/TR/CSS21/visudet.html#propdef-height
"if 'height' has a computed value of 'auto', and the element has an intrinsic ratio then the used value of 'height' is: (used width) / (intrinsic ratio)".
So this seems could be a float value.
And here are some examples indicated that Used Value could be a float. https://drafts.csswg.org/css-cascade-4/#stages-examples

In this test, we check aspect-ratio by
`assert_approx_equals(parseInt(getComputedStyle(img).width, 10) / parseInt(getComputedStyle(img).height, 10), expected, epsilon);`.
getComputedStyle(img).width is 100px, the intrinsic size of the image is 133x106, so the aspect-ratio is 1.2547. Then getComputedStyle(img).height is 100/1.2547=79.7003. After parseInt, it is 79. SO the value of this javascript is 100/79=1.266.

Hi @Emilio, please correct me if I misunderstood something. Thanks very much:)

[Frédéric Wang (:fredw)]

Comment on attachment 386958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386958&action=review

>>>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
>>>> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25
>>> 
>>> The result of getComputedStyle(img).width or height on WebKit are trimmed to an integer, so the aspect-ratio is 1.25. Gecko and Blink return a float value, so it's 1.266.
>>> Other cases in this file are passed.
>> 
>> So is it a webkit bug? Or a test bug?
> 
> It seems 1.266 is correct.
> 
> I followed the following spec train:
> https://www.w3.org/TR/cssom-1/#dom-window-getcomputedstyle
> https://www.w3.org/TR/cssom-1/#cssstyledeclaration-declarations
> https://www.w3.org/TR/cssom-1/#resolved-value
> https://drafts.csswg.org/css-cascade-4/#used-value
> 
> IIUC, the value of getComputedStyle(img).width or height is the "Used Value".
> And the used value for height is defined here: https://www.w3.org/TR/CSS21/visudet.html#propdef-height
> "if 'height' has a computed value of 'auto', and the element has an intrinsic ratio then the used value of 'height' is: (used width) / (intrinsic ratio)".
> So this seems could be a float value.
> And here are some examples indicated that Used Value could be a float. https://drafts.csswg.org/css-cascade-4/#stages-examples
> 
> In this test, we check aspect-ratio by
> `assert_approx_equals(parseInt(getComputedStyle(img).width, 10) / parseInt(getComputedStyle(img).height, 10), expected, epsilon);`.
> getComputedStyle(img).width is 100px, the intrinsic size of the image is 133x106, so the aspect-ratio is 1.2547. Then getComputedStyle(img).height is 100/1.2547=79.7003. After parseInt, it is 79. SO the value of this javascript is 100/79=1.266.
> 
> Hi @Emilio, please correct me if I misunderstood something. Thanks very much:)

IIUC, the img width is 100px per CSS and height is 100px / aspect ratio = 100px / (133/106) = 79.69924812030075. Then as you said it becomes 79 after parseInt so it becomes 100/79 = 1.265822784810126.

But this seems very confusing since the comment say 1.26 is the original aspect ratio. I believe parseFloat should be used instead. I guess it was ok to use parseInt in Emilio's original tests, but it does not work with this particular part added by Christian.

[cathiechen]

Comment on attachment 386958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386958&action=review

>>>>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3
>>>>> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.266 +/- 0.001 but got 1.25
>>>> 
>>>> The result of getComputedStyle(img).width or height on WebKit are trimmed to an integer, so the aspect-ratio is 1.25. Gecko and Blink return a float value, so it's 1.266.
>>>> Other cases in this file are passed.
>>> 
>>> So is it a webkit bug? Or a test bug?
>> 
>> It seems 1.266 is correct.
>> 
>> I followed the following spec train:
>> https://www.w3.org/TR/cssom-1/#dom-window-getcomputedstyle
>> https://www.w3.org/TR/cssom-1/#cssstyledeclaration-declarations
>> https://www.w3.org/TR/cssom-1/#resolved-value
>> https://drafts.csswg.org/css-cascade-4/#used-value
>> 
>> IIUC, the value of getComputedStyle(img).width or height is the "Used Value".
>> And the used value for height is defined here: https://www.w3.org/TR/CSS21/visudet.html#propdef-height
>> "if 'height' has a computed value of 'auto', and the element has an intrinsic ratio then the used value of 'height' is: (used width) / (intrinsic ratio)".
>> So this seems could be a float value.
>> And here are some examples indicated that Used Value could be a float. https://drafts.csswg.org/css-cascade-4/#stages-examples
>> 
>> In this test, we check aspect-ratio by
>> `assert_approx_equals(parseInt(getComputedStyle(img).width, 10) / parseInt(getComputedStyle(img).height, 10), expected, epsilon);`.
>> getComputedStyle(img).width is 100px, the intrinsic size of the image is 133x106, so the aspect-ratio is 1.2547. Then getComputedStyle(img).height is 100/1.2547=79.7003. After parseInt, it is 79. SO the value of this javascript is 100/79=1.266.
>> 
>> Hi @Emilio, please correct me if I misunderstood something. Thanks very much:)
> 
> IIUC, the img width is 100px per CSS and height is 100px / aspect ratio = 100px / (133/106) = 79.69924812030075. Then as you said it becomes 79 after parseInt so it becomes 100/79 = 1.265822784810126.
> 
> But this seems very confusing since the comment say 1.26 is the original aspect ratio. I believe parseFloat should be used instead. I guess it was ok to use parseInt in Emilio's original tests, but it does not work with this particular part added by Christian.

Done, thanks! Uploaded a PL to WPT. https://github.com/web-platform-tests/wpt/pull/21134

[cathiechen]

Created attachment 387636 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 387636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387636&action=review

This LGTM, but I don't see the same bug for div in https://bugs.webkit.org/show_bug.cgi?id=206161

> Source/WebCore/ChangeLog:10
> +        img element's layout size before loading. The value will be overridden if img is loaded. Also see [2].

Maybe explain the plan for future elements? I think Blink does some non-yet-standard things for video?

> Source/WebKit/Shared/WebPreferences.yaml:1518
> +  humanReadableDescription: "Map HTML attributes width/height to the default aspect ratio of <img>"

Maybe this should be renamed AspectRatioOfImgFromWidthAndHeightEnabled in case we decide to support the same for other elements in the future?

> LayoutTests/imported/w3c/ChangeLog:10
> +        * web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:

Can you please mention that one case still fails because of https://bugs.webkit.org/show_bug.cgi?id=206161 ?

[cathiechen]

Comment on attachment 387636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387636&action=review

Hi Fred,
Thanks for the review:)

>> Source/WebCore/ChangeLog:10
>> +        img element's layout size before loading. The value will be overridden if img is loaded. Also see [2].
> 
> Maybe explain the plan for future elements? I think Blink does some non-yet-standard things for video?

Done. It seems all three browser engines are consistent now. Only <img> element is affected.

>> LayoutTests/imported/w3c/ChangeLog:10
>> +        * web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:
> 
> Can you please mention that one case still fails because of https://bugs.webkit.org/show_bug.cgi?id=206161 ?

Done

[cathiechen]

Created attachment 387796 [details]
Patch

[cathiechen]

Created attachment 387800 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387800 [details]
Patch

Clearing flags on attachment: 387800

Committed r254669: <https://trac.webkit.org/changeset/254669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

*** Bug 180580 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

It looks like AspectRatioOfImgFromWidthAndHeightEnabled is still off by default. Should it be on now?

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #49)
> It looks like AspectRatioOfImgFromWidthAndHeightEnabled is still off by
> default. Should it be on now?

Hi Simon, I think this has happened several times in the past, but WebKit does not have any clear process to implement and ship a new web plaform feature. So the tacit agreement was that contributors implement new features under an experimental feature flag and Apple decides when to enable that flag, once they want to ship the new feature in their products. Should we instead have a more formal process? Anyway, Igalia is happy to enable this, Cathie will upload a follow-up patch.

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #50)
> (In reply to Simon Fraser (smfr) from comment #49)
> > It looks like AspectRatioOfImgFromWidthAndHeightEnabled is still off by
> > default. Should it be on now?
> 
> Hi Simon, I think this has happened several times in the past, but WebKit
> does not have any clear process to implement and ship a new web plaform
> feature. So the tacit agreement was that contributors implement new features
> under an experimental feature flag and Apple decides when to enable that
> flag, once they want to ship the new feature in their products. Should we
> instead have a more formal process? Anyway, Igalia is happy to enable this,
> Cathie will upload a follow-up patch.

Sorry for the lack of clarity here. I think the best option would be to email webkit-dev when you think the feature is ready to be enabled by default.

[Maciej Stachowiak]

Generally it's up to every port to decide, but we don't have a clear process for what the WebKit Open Source Project recommends as the best choice of flags for port owners.

[Frédéric Wang (:fredw)]

(In reply to Maciej Stachowiak from comment #52)
> Generally it's up to every port to decide, but we don't have a clear process
> for what the WebKit Open Source Project recommends as the best choice of
> flags for port owners.

I think there is the point of view of port owner who can have the freedom to enable or disable what they want (including features that are not part of the web platform), but there is also the point of view of web platform implementers who are strongly interested in standards and web interoperability. So I believe it would be useful to have a process to at least agree on how to propose/announce a new feature, implement it and enable the flag in the default/recommended configuration. Anyway, I guess this is a discussion for webkit-dev...

[Antti Koivisto]

This caused https://bugs.webkit.org/show_bug.cgi?id=209590

[Owen Sullivan]

This feature isn't working for me in Safari 14.0 or higher on macOS or iOS. I've tried disabling AspectRatioOfImgFromWidthAndHeightEnabled and turning it back on again, but even a fresh browser with no changes to experimental features has issues with related reflow.

[cathiechen]

(In reply to Owen Sullivan from comment #55)
> This feature isn't working for me in Safari 14.0 or higher on macOS or iOS.
> I've tried disabling AspectRatioOfImgFromWidthAndHeightEnabled and turning
> it back on again, but even a fresh browser with no changes to experimental
> features has issues with related reflow.

Hi Owen,

Can you provide a test case?
It works when I try the wpt test in "Release 121 (Safari 14.2, WebKit 16612.1.4.3)". (web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio.html)

[Owen Sullivan]

(In reply to cathiechen from comment #56)
> (In reply to Owen Sullivan from comment #55)
> > This feature isn't working for me in Safari 14.0 or higher on macOS or iOS.
> > I've tried disabling AspectRatioOfImgFromWidthAndHeightEnabled and turning
> > it back on again, but even a fresh browser with no changes to experimental
> > features has issues with related reflow.
> 
> Hi Owen,
> 
> Can you provide a test case?
> It works when I try the wpt test in "Release 121 (Safari 14.2, WebKit
> 16612.1.4.3)".
> (web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-
> content-and-images/img-aspect-ratio.html)

Here's a codepen that demonstrates my issue: https://codepen.io/sulliops/full/QWdwRxx

Also a video demonstrating the issue: https://gyazo.com/da76046282965f7a32ed3d85cc7423df

If you load the pen in Safari and immediately click on "Nav 4" in the navbar, you'll see that it takes you closer to Nav 3 because Safari is not calculating the aspect ratios of images with defined widths and heights before they are lazy loaded (reflow).

[Simon Fraser (smfr)]

Owen, could you please file a bug on this issue?

[Owen Sullivan]

(In reply to Simon Fraser (smfr) from comment #58)
> Owen, could you please file a bug on this issue?

Bug created at https://bugs.webkit.org/show_bug.cgi?id=224197