Bug 220970 - Handle zero aspect-ratio width/height
Summary: Handle zero aspect-ratio width/height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 47738
  Show dependency tree / graph
 
Reported: 2021-01-26 03:43 PST by Rob Buis
Modified: 2021-01-27 06:54 PST (History)
11 users (show)

See Also:

Attachments
Patch (1.97 KB, patch)
2021-01-26 03:48 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2021-01-26 06:55 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2021-01-26 12:38 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (6.72 KB, patch)
2021-01-27 03:08 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2021-01-27 03:41 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2021-01-27 05:03 PST, Rob Buis 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-01-26 03:43:12 PST 
Handle zero aspect-ratio width/height
Comment 1 Rob Buis 2021-01-26 03:48:10 PST 
Created attachment 418389 [details]
Patch
Comment 2 Rob Buis 2021-01-26 06:55:44 PST 
Created attachment 418413 [details]
Patch
Comment 3 Rob Buis 2021-01-26 12:38:45 PST 
Created attachment 418464 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2021-01-26 13:17:45 PST 
Comment on attachment 418464 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        as auto, but when serializing keep the original input.

Are there any tests for the serialization?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3475
> +                if (style.aspectRatioWidth() > 0 && style.aspectRatioHeight() > 0)
> +                    return cssValuePool.createIdentifierValue(CSSValueAuto);

I'm not sure I get this, why if width or height is bigger than zero we return auto?
Comment 5 Rob Buis 2021-01-26 13:24:44 PST 
Comment on attachment 418464 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        as auto, but when serializing keep the original input.
> 
> Are there any tests for the serialization?

Yes, it is tested by css/css-sizing/aspect-ratio/parsing/contain-intrinsic-size-computed.html.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3475
>> +                    return cssValuePool.createIdentifierValue(CSSValueAuto);
> 
> I'm not sure I get this, why if width or height is bigger than zero we return auto?

Yes upon reflection  this logic is not very clear and it think it may break. I will add a new enum value tomorrow, this should make the code clearer and not error prone.
Comment 6 Rob Buis 2021-01-27 03:08:38 PST 
Created attachment 418520 [details]
Patch
Comment 7 Rob Buis 2021-01-27 03:41:10 PST 
Created attachment 418524 [details]
Patch
Comment 8 Manuel Rego Casasnovas 2021-01-27 04:07:19 PST 
Comment on attachment 418524 [details]
Patch

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

r=me, let's wait for EWSs output, but the change looks better now. Thanks.

> Source/WebCore/style/StyleBuilderCustom.h:1189
>      builderState.style().setAspectRatio(downcast<CSSPrimitiveValue>(list.item(0))->doubleValue(), downcast<CSSPrimitiveValue>(list.item(1))->doubleValue());

Nit: Please use here width and height.
Comment 9 Rob Buis 2021-01-27 05:03:21 PST 
Created attachment 418531 [details]
Patch
Comment 10 EWS 2021-01-27 06:14:03 PST 
Committed r271948: <https://trac.webkit.org/changeset/271948>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418531 [details].
Comment 11 Radar WebKit Bug Importer 2021-01-27 06:15:14 PST 
<rdar://problem/73659916>