Bug 218437 – Parse aspect-ratio CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Parse aspect-ratio CSS property according to the specification [1].

[1] https://drafts.csswg.org/css-sizing-4/#aspect-ratio

[Rob Buis]

Created attachment 412905 [details]
Patch

[Rob Buis]

Created attachment 412920 [details]
Patch

[Rob Buis]

Created attachment 413013 [details]
Patch

[Rob Buis]

Created attachment 413017 [details]
Patch

[EWS Watchlist]

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

[Rob Buis]

Created attachment 413034 [details]
Patch

[Rob Buis]

Created attachment 413050 [details]
Patch

[Rob Buis]

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).

[Rob Buis]

Created attachment 413145 [details]
Patch

[Rob Buis]

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.

[Radar WebKit Bug Importer]

<rdar://problem/71187109>

[Darin Adler]

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&.

[Darin Adler]

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.

[Simon Fraser (smfr)]

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.

[Rob Buis]

Created attachment 413686 [details]
Patch

[Rob Buis]

Created attachment 413702 [details]
Patch

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

(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.

[Simon Fraser (smfr)]

I thought we had some rounding code for floating-point values for cssText() but I can't find it.

[Darin Adler]

(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.

[Darin Adler]

(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.

[EWS]

Committed r269641: <https://trac.webkit.org/changeset/269641>

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