Bug 201641 - Default <img> aspect ratio from size attributes
Summary: Default <img> aspect ratio from size attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
: 180580 (view as bug list)
Depends on: 206161 205678 206160
Blocks: 207880
  Show dependency treegraph
 
Reported: 2019-09-10 06:36 PDT by cathiechen
Modified: 2020-03-26 03:33 PDT (History)
21 users (show)

See Also:


Attachments
Patch (1.04 MB, patch)
2019-12-19 01:08 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2019-12-19 01:35 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2019-12-19 01:39 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2019-12-19 01:50 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2019-12-19 07:20 PST, cathiechen
no flags Details | Formatted Diff | Diff
error_images (749 bytes, text/html)
2019-12-30 06:01 PST, cathiechen
no flags Details
Patch (24.35 KB, patch)
2020-01-06 05:31 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (25.55 KB, patch)
2020-01-07 03:43 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (25.55 KB, patch)
2020-01-14 01:46 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (26.21 KB, patch)
2020-01-15 10:11 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (26.14 KB, patch)
2020-01-15 10:28 PST, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2019-09-10 06:36:38 PDT
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
Comment 1 Radar WebKit Bug Importer 2019-09-10 08:48:13 PDT
<rdar://problem/55223958>
Comment 2 Frédéric Wang (:fredw) 2019-10-03 00:18:01 PDT
"Intent to ship" emails have been sent to mozilla and blink developer mailing lists.
Comment 3 Frédéric Wang (:fredw) 2019-11-25 02:57:26 PST
Bug 197739 has an experimental patch for the legacy "intrinsicsize attribute" proposal and can probably be reused here.
Comment 4 Frédéric Wang (:fredw) 2019-11-28 19:06:26 PST
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
Comment 5 Emilio Cobos Álvarez (:emilio) 2019-11-28 20:07:18 PST
That is a completely different thing. That's about an intrinsic-size CSS property, that did something fairly different.
Comment 6 Manuel Rego Casasnovas 2019-11-28 23:54:29 PST
(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.
  "
Comment 7 Frédéric Wang (:fredw) 2019-11-29 03:19:01 PST
@Emilio, Rego: Thanks for clarifying and sorry for the confusion!
Comment 8 cathiechen 2019-12-19 01:08:43 PST
Created attachment 386082 [details]
Patch
Comment 9 cathiechen 2019-12-19 01:35:50 PST
Created attachment 386086 [details]
Patch
Comment 10 cathiechen 2019-12-19 01:39:57 PST
Created attachment 386087 [details]
Patch
Comment 11 cathiechen 2019-12-19 01:50:05 PST
Created attachment 386088 [details]
Patch
Comment 12 Manuel Rego Casasnovas 2019-12-19 04:17:35 PST
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.
Comment 13 Frédéric Wang (:fredw) 2019-12-19 04:42:31 PST
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...
Comment 14 cathiechen 2019-12-19 07:20:32 PST
Created attachment 386100 [details]
Patch
Comment 15 Emilio Cobos Álvarez (:emilio) 2019-12-19 07:50:56 PST
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.
Comment 16 Said Abou-Hallawa 2019-12-19 09:44:03 PST
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().
Comment 17 Maciej Stachowiak 2019-12-19 11:20:47 PST
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.
Comment 18 Maciej Stachowiak 2019-12-19 11:22:59 PST
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.
Comment 19 Emilio Cobos Álvarez (:emilio) 2019-12-19 12:30:36 PST
(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.
Comment 20 Emilio Cobos Álvarez (:emilio) 2019-12-19 12:37:48 PST
(Not observable from the computed style, I mean. It is of course observable)
Comment 21 Simon Fraser (smfr) 2019-12-19 13:15:37 PST
Is there good WPT test coverage for this? Perhaps Cathie's first step should be importing some WPT tests.
Comment 22 Emilio Cobos Álvarez (:emilio) 2019-12-19 16:36:47 PST
(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.
Comment 23 Frédéric Wang (:fredw) 2019-12-19 23:29:49 PST
(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?
Comment 24 Manuel Rego Casasnovas 2019-12-20 00:03:20 PST
(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
Comment 25 cathiechen 2019-12-20 09:32:26 PST
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.
Comment 26 Emilio Cobos Álvarez (:emilio) 2019-12-20 11:28:30 PST
(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.
Comment 27 cathiechen 2019-12-30 06:01:05 PST
Created attachment 386525 [details]
error_images
Comment 28 cathiechen 2019-12-30 06:58:30 PST
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.
Comment 29 cathiechen 2019-12-30 07:45:10 PST
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.
Comment 30 Emilio Cobos Álvarez (:emilio) 2019-12-30 10:29:07 PST
(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...
Comment 31 cathiechen 2020-01-01 07:50:23 PST
(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.
Comment 32 cathiechen 2020-01-01 21:12:31 PST
(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.
Comment 33 cathiechen 2020-01-06 05:22:13 PST
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.
Comment 34 cathiechen 2020-01-06 05:31:13 PST
Created attachment 386834 [details]
Patch
Comment 35 cathiechen 2020-01-07 03:43:45 PST
Created attachment 386958 [details]
Patch
Comment 36 cathiechen 2020-01-07 19:35:11 PST
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.
Comment 37 Frédéric Wang (:fredw) 2020-01-08 02:38:49 PST
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?
Comment 38 cathiechen 2020-01-09 09:48:23 PST
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:)
Comment 39 Frédéric Wang (:fredw) 2020-01-10 00:53:53 PST
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.
Comment 40 cathiechen 2020-01-10 07:15:18 PST
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
Comment 41 cathiechen 2020-01-14 01:46:32 PST
Created attachment 387636 [details]
Patch
Comment 42 Frédéric Wang (:fredw) 2020-01-14 02:32:28 PST
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 ?
Comment 43 cathiechen 2020-01-15 10:03:51 PST
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
Comment 44 cathiechen 2020-01-15 10:11:20 PST
Created attachment 387796 [details]
Patch
Comment 45 cathiechen 2020-01-15 10:28:16 PST
Created attachment 387800 [details]
Patch
Comment 46 WebKit Commit Bot 2020-01-16 02:32:39 PST
Comment on attachment 387800 [details]
Patch

Clearing flags on attachment: 387800

Committed r254669: <https://trac.webkit.org/changeset/254669>
Comment 47 WebKit Commit Bot 2020-01-16 02:32:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 48 Simon Fraser (smfr) 2020-01-22 14:54:59 PST
*** Bug 180580 has been marked as a duplicate of this bug. ***
Comment 49 Simon Fraser (smfr) 2020-02-15 10:43:48 PST
It looks like AspectRatioOfImgFromWidthAndHeightEnabled is still off by default. Should it be on now?
Comment 50 Frédéric Wang (:fredw) 2020-02-17 22:42:03 PST
(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.
Comment 51 Simon Fraser (smfr) 2020-02-18 11:11:03 PST
(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.
Comment 52 Maciej Stachowiak 2020-02-19 03:00:03 PST
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.
Comment 53 Frédéric Wang (:fredw) 2020-02-19 03:12:26 PST
(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...
Comment 54 Antti Koivisto 2020-03-26 03:32:50 PDT
This caused https://bugs.webkit.org/show_bug.cgi?id=209590