Bug 218437

Summary: Parse aspect-ratio CSS property
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, kondapallykalyan, macpherson, menard, pdr, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 47738    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2020-11-02 05:21:29 PST
Parse aspect-ratio CSS property according to the specification [1].

[1] https://drafts.csswg.org/css-sizing-4/#aspect-ratio
Comment 1 Rob Buis 2020-11-02 05:26:55 PST
Created attachment 412905 [details]
Patch
Comment 2 Rob Buis 2020-11-02 08:05:43 PST
Created attachment 412920 [details]
Patch
Comment 3 Rob Buis 2020-11-03 00:56:42 PST
Created attachment 413013 [details]
Patch
Comment 4 Rob Buis 2020-11-03 01:28:21 PST
Created attachment 413017 [details]
Patch
Comment 5 EWS Watchlist 2020-11-03 01:29:27 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 6 Rob Buis 2020-11-03 03:30:44 PST
Created attachment 413034 [details]
Patch
Comment 7 Rob Buis 2020-11-03 05:28:14 PST
Created attachment 413050 [details]
Patch
Comment 8 Rob Buis 2020-11-03 06:55:53 PST
Comment on attachment 413050 [details]
Patch

This can be reviewed, but ideally https://bugs.webkit.org/show_bug.cgi?id=218384 is reviewed first since then I can rebase and make this patch smaller (remove -webkit-aspect-ratio related changes).
Comment 9 Rob Buis 2020-11-04 01:08:13 PST
Created attachment 413145 [details]
Patch
Comment 10 Rob Buis 2020-11-04 09:09:06 PST
Comment on attachment 413145 [details]
Patch

Latest iteration of the patch removed changes to -webkit-aspect-ratio (https://bugs.webkit.org/show_bug.cgi?id=218384) from previous iterations, so it should be possible to review it as a standalone patch.
Comment 11 Radar WebKit Bug Importer 2020-11-09 05:22:19 PST
<rdar://problem/71187109>
Comment 12 Darin Adler 2020-11-09 14:24:48 PST
Comment on attachment 413145 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3941
> +        return autoValue;

Since the types don’t match here (CSSValue vs. CSSPrimitiveValue), this should be WTFMove(autoValue).

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3943
> +    RefPtr<CSSPrimitiveValue> leftValue = consumeNumber(range, ValueRangeNonNegative);

auto

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3949
> +    RefPtr<CSSPrimitiveValue> rightValue = consumeNumber(range, ValueRangeNonNegative);

auto

> Source/WebCore/rendering/style/RenderStyle.h:1015
> +    void setAspectRatio(FloatSize v) { SET_VAR(m_rareNonInheritedData, aspectRatio, v); }

Maybe const FloatSize&? Not sure if it matters.

> Source/WebCore/style/StyleBuilderCustom.h:1239
> +        const auto& ratioList = downcast<CSSValueList>(list.item(1));

I think this should be auto, not const auto&.
Comment 13 Darin Adler 2020-11-09 14:25:41 PST
Comment on attachment 413145 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3962
> +        return ratioList;

WTFMove here too for the same reason.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3966
> +    return list;

WTFMove here too for the same reason.
Comment 14 Simon Fraser (smfr) 2020-11-09 14:28:27 PST
Comment on attachment 413145 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:65
> +  humanReadableName: "aspect-ratio CSS property"

Let's called this "CSS Aspect Ratio" for better menu sorting.

> Source/WebCore/css/CSSProperties.json:4490
> +        "aspect-ratio": {
> +            "codegen-properties": {
> +                "custom": "All"
> +            },
> +            "status": {
> +                "status": "experimental"
> +            }

Please add a spec URL.

> LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/parsing/contain-intrinsic-size-computed-expected.txt:7
> +FAIL Property aspect-ratio value '1.3' assert_equals: expected "1.3 / 1" but got "1.2999999523162842 / 1"
> +PASS Property aspect-ratio value '1 / 1'
> +PASS Property aspect-ratio value '16 / 9'
> +FAIL Property aspect-ratio value '16.3 / 9.5' assert_equals: expected "16.3 / 9.5" but got "16.299999237060547 / 9.5"

These suggest you need to use the normal rounding rules for computed style.
Comment 15 Rob Buis 2020-11-10 05:25:00 PST
Created attachment 413686 [details]
Patch
Comment 16 Rob Buis 2020-11-10 08:56:36 PST
Created attachment 413702 [details]
Patch
Comment 17 Rob Buis 2020-11-10 09:55:13 PST
Comment on attachment 413145 [details]
Patch

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

>> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:65
>> +  humanReadableName: "aspect-ratio CSS property"
> 
> Let's called this "CSS Aspect Ratio" for better menu sorting.

Done.

>> Source/WebCore/css/CSSProperties.json:4490
>> +            }
> 
> Please add a spec URL.

Done.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3941
>> +        return autoValue;
> 
> Since the types don’t match here (CSSValue vs. CSSPrimitiveValue), this should be WTFMove(autoValue).

Done.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3943
>> +    RefPtr<CSSPrimitiveValue> leftValue = consumeNumber(range, ValueRangeNonNegative);
> 
> auto

Done.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3949
>> +    RefPtr<CSSPrimitiveValue> rightValue = consumeNumber(range, ValueRangeNonNegative);
> 
> auto

Done.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3962
>> +        return ratioList;
> 
> WTFMove here too for the same reason.

Done.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3966
>> +    return list;
> 
> WTFMove here too for the same reason.

Done.

>> Source/WebCore/rendering/style/RenderStyle.h:1015
>> +    void setAspectRatio(FloatSize v) { SET_VAR(m_rareNonInheritedData, aspectRatio, v); }
> 
> Maybe const FloatSize&? Not sure if it matters.

That is better, but I ended up using doubles.

>> Source/WebCore/style/StyleBuilderCustom.h:1239
>> +        const auto& ratioList = downcast<CSSValueList>(list.item(1));
> 
> I think this should be auto, not const auto&.

Done.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/parsing/contain-intrinsic-size-computed-expected.txt:7
>> +FAIL Property aspect-ratio value '16.3 / 9.5' assert_equals: expected "16.3 / 9.5" but got "16.299999237060547 / 9.5"
> 
> These suggest you need to use the normal rounding rules for computed style.

Turns out I need double precision to make this work.
Comment 18 Darin Adler 2020-11-10 10:01:37 PST
Comment on attachment 413145 [details]
Patch

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

>>> LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/parsing/contain-intrinsic-size-computed-expected.txt:7
>>> +FAIL Property aspect-ratio value '16.3 / 9.5' assert_equals: expected "16.3 / 9.5" but got "16.299999237060547 / 9.5"
>> 
>> These suggest you need to use the normal rounding rules for computed style.
> 
> Turns out I need double precision to make this work.

There are actually two different ways to do it. Stay consistently in single precision or stay consistently in double precision. The bad output comes when we compute in single precision, but then serialize by converting to a double precision before doing so, or sometimes if we just make some round trips single to double and back at some point.

All double precision is probably OK/better.
Comment 19 Rob Buis 2020-11-10 10:39:26 PST
(In reply to Darin Adler from comment #18)
> > Turns out I need double precision to make this work.
> 
> There are actually two different ways to do it. Stay consistently in single
> precision or stay consistently in double precision. The bad output comes
> when we compute in single precision, but then serialize by converting to a
> double precision before doing so, or sometimes if we just make some round
> trips single to double and back at some point.
> 
> All double precision is probably OK/better.

Thanks Darin. Out of interest, do you know of a property doing the single precision approach? I looked a bit but could not find anything relevant.
Comment 20 Simon Fraser (smfr) 2020-11-10 11:04:27 PST
I thought we had some rounding code for floating-point values for cssText() but I can't find it.
Comment 21 Darin Adler 2020-11-10 11:05:31 PST
(In reply to Rob Buis from comment #19)
> Out of interest, do you know of a property doing the single
> precision approach? I looked a bit but could not find anything relevant.

I’m not sure if there is any property that does the single precision approach *correctly*. For example, CSSPropertyZoom uses single precision, but I see it converting to double precision so I assume if we constructed an appropriate test we could demonstrate that it serializes incorrectly.
Comment 22 Darin Adler 2020-11-10 11:09:07 PST
(In reply to Simon Fraser (smfr) from comment #20)
> I thought we had some rounding code for floating-point values for cssText()
> but I can't find it.

Maybe we did. But I think it would be very challenging to achieve a correct and consistent result with rounding code. We want to be able to round trip values without introducing errors. Rounding to a specific number of significant digits when serializing can hide the small errors in some cases, but it introduces new problems if code round-trips from the value to the serialized text and the sets it back. Consistent precision and serialization does not have that problem.

So I think it’s best to not think about this as a "rounding" problem.
Comment 23 EWS 2020-11-10 12:40:47 PST
Committed r269641: <https://trac.webkit.org/changeset/269641>

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