Bug 231952 - [css-flexbox] Add support for replaced elements with intrinsic ratio and no intrinsic size
Summary: [css-flexbox] Add support for replaced elements with intrinsic ratio and no i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 221474
  Show dependency treegraph
 
Reported: 2021-10-19 07:24 PDT by Sergio Villar Senin
Modified: 2021-11-29 02:01 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.49 KB, patch)
2021-10-19 07:32 PDT, Sergio Villar Senin
svillar: review?
Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2021-10-19 08:24 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (8.23 KB, patch)
2021-10-27 11:20 PDT, Sergio Villar Senin
rego: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2021-10-19 07:24:47 PDT
[css-flexbox] Add support for replaced elements with intrinsic ratio and no intrinsic size
Comment 1 Sergio Villar Senin 2021-10-19 07:32:28 PDT
Created attachment 441722 [details]
Patch
Comment 2 Rob Buis 2021-10-19 07:57:49 PDT
Comment on attachment 441722 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:621
> +static bool isRenderReplacedWithIntrinsicAspectRatio(const RenderBox& child)

I think it is worth trying to make this a method on RenderReplaced instead of static, so we do not need to make computeIntrinsicRatioInformation public on RenderReplaced.h.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:625
> +    // It's common for some replaced elements, such SVGs, to have intrinsic aspect ratios but no intrinsic sizes.

"such as SVGs"
Comment 3 Sergio Villar Senin 2021-10-19 08:24:36 PDT
Created attachment 441729 [details]
Patch
Comment 4 Sergio Villar Senin 2021-10-19 08:25:59 PDT
Comment on attachment 441722 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:621
>> +static bool isRenderReplacedWithIntrinsicAspectRatio(const RenderBox& child)
> 
> I think it is worth trying to make this a method on RenderReplaced instead of static, so we do not need to make computeIntrinsicRatioInformation public on RenderReplaced.h.

Good suggestion. I filed wkb.ug/231955 to tackle this.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:625
>> +    // It's common for some replaced elements, such SVGs, to have intrinsic aspect ratios but no intrinsic sizes.
> 
> "such as SVGs"

OK
Comment 5 Radar WebKit Bug Importer 2021-10-26 07:25:28 PDT
<rdar://problem/84662305>
Comment 6 Sergio Villar Senin 2021-10-27 11:20:56 PDT
Created attachment 442613 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2021-11-25 03:45:03 PST
Comment on attachment 442613 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:898
> +        ratio = downcast<RenderReplaced>(child).computeIntrinsicAspectRatio();

One question, is this always using the intrinsic aspect ratio for replaced elements? Even if you set your own value (e.g. "aspect-ratio: 1;")?
Comment 8 Javier Fernandez 2021-11-25 06:03:28 PST
Comment on attachment 442613 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:904
> +            ASSERT(childIntrinsicSize.height());

Is this ASSERT correct ? How we can guarantee that the child's intrinsic size isn't 0 ? 
For instance, for replaced elements, if shouldApplySizeContainment() is true we would return LayoutSize()
Comment 9 Sergio Villar Senin 2021-11-25 09:52:06 PST
Comment on attachment 442613 [details]
Patch

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

Thanks for taking a look!

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:898
>> +        ratio = downcast<RenderReplaced>(child).computeIntrinsicAspectRatio();
> 
> One question, is this always using the intrinsic aspect ratio for replaced elements? Even if you set your own value (e.g. "aspect-ratio: 1;")?

It considers all the situations. This method just calls RenderReplaced::computeAspectRatioInformationForRenderBox() which already takes care of many different situations, like having/not having intrinsic sizes, using aspect-ratio etc...

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:904
>> +            ASSERT(childIntrinsicSize.height());
> 
> Is this ASSERT correct ? How we can guarantee that the child's intrinsic size isn't 0 ? 
> For instance, for replaced elements, if shouldApplySizeContainment() is true we would return LayoutSize()

Well that ASSERT was there before this patch so I am not adding anything.

Anyway as you ask, it's true that it's far from obvious. This is the rationale, this method computeMainSizeFromAspectRatioUsing() is only called whenever childHasComputableAspectRatio() is true.  That method is implemented as follows.

return child.intrinsicSize().height() || child.style().hasAspectRatio()

So we either have a positive intrinsic height (in which case the ASSERT is true) or hasAspectRatio() is true. If you check hasAspectRatio() implementation it's almost equal to the condition in the if branch, something like this:

return child.style().aspectRatioType() == AspectRatioType::Ratio || (child.style().aspectRatioType() == AspectRatioType::AutoAndRatio;

So we have two possibilities for hasAspectRatio() == true:
a) child.style().aspectRatioType() == AspectRatioType::Ratio. If that's the case then the code should have taken the if branch not the else
b) child.style().aspectRatioType() == AspectRatioType::AutoAndRatio. If that condition is true the only possibility to end up in the else branch is that the intrinsic size is not empty, because otherwise we should have taken the if branch as well.

Summing up, if we take the else branch we know that child.intrinsicSize().height() is true in childHasComputableAspectRatio() or that we are in the above case b) in which case we know that the height is also valid.
Comment 10 Manuel Rego Casasnovas 2021-11-25 12:38:41 PST
Comment on attachment 442613 [details]
Patch

Ok, thanks for the clarifications. r=me
Comment 11 Sergio Villar Senin 2021-11-29 02:01:03 PST
Committed r286206 (244588@main): <https://commits.webkit.org/244588@main>