Summary: | Parse aspect-ratio CSS property | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||||||||||||||
Component: | CSS | Assignee: | 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
Rob Buis
2020-11-02 05:21:29 PST
Created attachment 412905 [details]
Patch
Created attachment 412920 [details]
Patch
Created attachment 413013 [details]
Patch
Created attachment 413017 [details]
Patch
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 Created attachment 413034 [details]
Patch
Created attachment 413050 [details]
Patch
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). Created attachment 413145 [details]
Patch
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 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 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 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. Created attachment 413686 [details]
Patch
Created attachment 413702 [details]
Patch
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 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. (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. I thought we had some rounding code for floating-point values for cssText() but I can't find it. (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. (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. Committed r269641: <https://trac.webkit.org/changeset/269641> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413702 [details]. |