Bug 224664 – The implicit aspect-ratio from width and height attributes with float value is not accurate enough

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

[cathiechen]

Test case with float width and height attribute fails in imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio.html

[cathiechen]

Created attachment 426207 [details]
Patch

[cathiechen]

Comment on attachment 426207 [details]
Patch

Hi,
I think this patch is ready for review. Thanks:)

[cathiechen]

Created attachment 426222 [details]
Patch

[Darin Adler]

Comment on attachment 426222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426222&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 4 +/- 0.001 but got 4.166666666666667
> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.2547169811320755 +/- 0.001 but got 1.25

Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please?

Especially wondering if we could construct a test that could show the value of the change, failing before and passing now.

[Rob Buis]

Comment on attachment 426222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426222&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.2547169811320755 +/- 0.001 but got 1.25
> 
> Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please?
> 
> Especially wondering if we could construct a test that could show the value of the change, failing before and passing now.

I believe the test is not well designed, instead of listing all subtests any fail means the test is stopped. So there is progress but due to the test design it is not clear. I think Antti agreed as well that the test is not ideal.

[cathiechen]

Comment on attachment 426222 [details]
Patch

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

Hi Darin and Rob,
Thanks for the review, and sorry for not to explain the patch clearly!

>>> 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.2547169811320755 +/- 0.001 but got 1.25
>> 
>> Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please?
>> 
>> Especially wondering if we could construct a test that could show the value of the change, failing before and passing now.
> 
> I believe the test is not well designed, instead of listing all subtests any fail means the test is stopped. So there is progress but due to the test design it is not clear. I think Antti agreed as well that the test is not ideal.

Like Rob said img-aspect-ratio.html is not well-designed.

The test case with width 0.8 and height 0.2 in img-aspect-ratio.html can reveal the different values.
Before this patch the aspect ratio is 4.166666666666667, so it failed. After this patch, the aspect ratio is 4.
In the code, 0.8 and 0.2 is converted to LayoutUnit, stored as m_intrinsicSize, then use it to calculate logical width and height.
Because LayoutUnit is not accurate to present the float values, so the aspect-ratio in this test is not accurate.
IIUC, the width and height attribute value is not the actual intrinsic size, so it shouldn't impact the intrinsic size.
And if we need intrinsic size to decide the logical width, we should wait for the image downloaded. 

PS: I will add an explanation to the ChangeLog.

[cathiechen]

Created attachment 426335 [details]
Patch

[Darin Adler]

Comment on attachment 426335 [details]
Patch

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

Looks good, but we can do better.

> Source/WebCore/rendering/RenderReplaced.cpp:506
> +        float attributeWidth = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::widthAttr)).valueOr(0);
> +        float attributeHeight = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0);

The parseValidHTMLFloatingPointNumber function returns an Optional<double>. But this code truncates that double to a float, then divides as float, then converts back to double. I suggest using auto here, or double and not involving single-precision float at all.

[cathiechen]

Comment on attachment 426335 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:506
>> +        float attributeHeight = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0);
> 
> The parseValidHTMLFloatingPointNumber function returns an Optional<double>. But this code truncates that double to a float, then divides as float, then converts back to double. I suggest using auto here, or double and not involving single-precision float at all.

Done! Thanks:)

[cathiechen]

Created attachment 426373 [details]
Patch

[EWS]

Committed r276228 (236710@main): <https://commits.webkit.org/236710@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426373 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77110069>