Bug 218794 - Support aspect-ratio on non-replaced elements
Summary: Support aspect-ratio on non-replaced elements
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:
 
Reported: 2020-11-11 01:56 PST by Rob Buis
Modified: 2020-12-08 12:30 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.07 KB, patch)
2020-11-11 02:02 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.28 KB, patch)
2020-11-11 07:03 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.28 KB, patch)
2020-11-11 12:22 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2020-11-16 05:20 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.27 KB, patch)
2020-11-18 05:12 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.15 KB, patch)
2020-11-20 02:34 PST, Rob Buis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2020-11-20 06:03 PST, Rob Buis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-11-11 01:56:02 PST
Support aspect-ratio on non-replaced elements
Comment 1 Rob Buis 2020-11-11 02:02:06 PST
Created attachment 413801 [details]
Patch
Comment 2 Rob Buis 2020-11-11 07:03:10 PST
Created attachment 413819 [details]
Patch
Comment 3 Rob Buis 2020-11-11 12:22:51 PST
Created attachment 413853 [details]
Patch
Comment 4 Rob Buis 2020-11-16 05:20:09 PST
Created attachment 414217 [details]
Patch
Comment 5 Simon Fraser (smfr) 2020-11-17 14:06:16 PST
Comment on attachment 414217 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2421
> +static LayoutUnit inlineSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, LayoutUnit aspectRatio, BoxSizing boxSizing, LayoutUnit blockSize)

I think aspectRatio should be floating point.

> Source/WebCore/rendering/style/RenderStyle.h:507
> +    LayoutUnit logicalAspectRatio() const

This return a floating point value, not a LayoutUnit.
Comment 6 Darin Adler 2020-11-17 15:25:16 PST
Comment on attachment 414217 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Compute logical width/height for non-replaced elements
> +        taking aspect ratio into account. This is very basic
> +        support and does not consider things like transferred min/max
> +        block size.

Is it a problem to ship very basic support in web browsers? If so, do we need this behind a feature flag so we can turn it on when it’s more complete?
Comment 7 Radar WebKit Bug Importer 2020-11-18 01:56:17 PST
<rdar://problem/71529770>
Comment 8 Rob Buis 2020-11-18 05:12:16 PST
Created attachment 414440 [details]
Patch
Comment 9 Rob Buis 2020-11-18 06:28:26 PST
Comment on attachment 414217 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2421
>> +static LayoutUnit inlineSizeFromAspectRatio(LayoutUnit borderPaddingInlineSum, LayoutUnit borderPaddingBlockSum, LayoutUnit aspectRatio, BoxSizing boxSizing, LayoutUnit blockSize)
> 
> I think aspectRatio should be floating point.

Done.

>> Source/WebCore/rendering/style/RenderStyle.h:507
>> +    LayoutUnit logicalAspectRatio() const
> 
> This return a floating point value, not a LayoutUnit.

Done.
Comment 10 Rob Buis 2020-11-18 07:44:53 PST
Comment on attachment 414217 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        block size.
> 
> Is it a problem to ship very basic support in web browsers? If so, do we need this behind a feature flag so we can turn it on when it’s more complete?

I feel it is safer to let this mature a bit, at least the majority of aspect ratio tests should pass I think. Maybe it can be turned on in a few weeks if people feel it is safe to do so and there is demand for it.
Comment 11 Rob Buis 2020-11-20 02:34:18 PST
Created attachment 414664 [details]
Patch
Comment 12 Rob Buis 2020-11-20 06:03:24 PST
Created attachment 414671 [details]
Patch
Comment 13 Rob Buis 2020-12-07 14:17:58 PST
ping. Anything left for me to do for this bug? I hope I did not misunderstand Darin's comment 6? :)
Comment 14 Simon Fraser (smfr) 2020-12-08 10:26:27 PST
(In reply to Rob Buis from comment #13)
> ping. Anything left for me to do for this bug? I hope I did not
> misunderstand Darin's comment 6? :)

He was asking whether it's OK to have this shipping in an incomplete form. If not, we should add a feature flag for it.
Comment 15 Rob Buis 2020-12-08 11:04:03 PST
(In reply to Simon Fraser (smfr) from comment #14)
> (In reply to Rob Buis from comment #13)
> > ping. Anything left for me to do for this bug? I hope I did not
> > misunderstand Darin's comment 6? :)
> 
> He was asking whether it's OK to have this shipping in an incomplete form.
> If not, we should add a feature flag for it.

Thanks! Just to be clear, at the moment aspect-ratio is an experimental preference. If I understand correctly, that does not equal to actually shipping?

The feature is defined here:
Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml 

And parsing and handling aspect-ratio depends on that in CSSPropertyParser::parseSingleValue.
Comment 16 Darin Adler 2020-12-08 11:06:18 PST
(In reply to Rob Buis from comment #15)
> Thanks! Just to be clear, at the moment aspect-ratio is an experimental
> preference. If I understand correctly, that does not equal to actually
> shipping?

Great, that answers my question.
Comment 17 EWS 2020-12-08 12:30:22 PST
Committed r270551: <https://trac.webkit.org/changeset/270551>

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