Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension. #6159

sammygill · 2022-11-05T00:11:41Z

`036e53a`

Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension.
https://bugs.webkit.org/show_bug.cgi?id=247507
rdar://101979495

Reviewed by Alan Baradlay.

There are certain scenarios where a replaced element may have a
specified aspect ratio, but will not have intrinsic sizes in both
dimensions. One such examples is a SVG with only a specified width
and aspect ratio.

To help facilitate this, the logic in
RenderReplaced::computeAspectRatioInformationForRenderBox has been
separated out since it was doing too much before. It was both computing
the intrinsic size/ratio and constraining the intrinsic size. The logic
for constraining the intrinsic size has been refactored out into a new
method called
RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes.
This method will call

In these scenarios, we must use the specified aspect ratio to do any
sort of sizing since it is not possible to compute it from the
dimensions of the box. computeAspectRatioInformationForRenderBox to get
the intrinsic size/ratio and then constrain it. This new method will
also constrain the sizes in each dimension by the transferred sizes.
This size may be then potentially constrained even further in
RenderReplaced::computeReplacedLogicalWidthRespectingMinMaxWidth (and
height) by any definite min/max sizes in that dimension. There was an
attempt to consolidate all constraining logic into that method, but
that was causing some unexpected results. That may need to be done in
a future patch.

A few of the CSS aspect ratio methods have also been modified to help
with this logic. Since a lot of the logic is shared, these methods
can determine if a replaced element is being used and use the correct
aspect ratio in that case.

A new member variable to RenderReplaced has been added:
m_intrinsicRatioSize. This is used to keep track of the width and height
sizes associated with the aspect ratio. This is because there were some
precision issues that this patch exposed. Certain tests were starting
to fail because the computed width/height from the aspect ratio was
slightly off. To help with this, we can store the intrinsic ratio sizes
until we actually need to use them to compute the width/height. The
goal here is to hopefully reduce the number of floating point operations
that are needed to compute a size. It may be possible to replace all
instances where the computed intrinsicRatio double is being used, but
that is probably a little too risky for this patch. Instead, this patch
only uses it in the call to resolveHeightForRatio within
RenderReplaced::computeReplacedLogicalHeight. It is probably best
to update all uses of intrinsicRatio to m_intrinsicRatioSize in another
patch. To compute the intrinsic ratio size we needed to keep track of
a new variable inside the computeIntrinsicRatioInformation logic and
then update the member variable once it was computed.

Spec references:
https://www.w3.org/TR/CSS22/visudet.html#min-max-widths
https://www.w3.org/TR/CSS22/visudet.html#min-max-heights
https://w3c.github.io/csswg-drafts/css-sizing-4/#aspect-ratio

* LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-039-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-039.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-040-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-040.html: Added.
* Source/WebCore/rendering/RenderBlock.cpp:
(WebCore::RenderBlock::computeChildPreferredLogicalWidths const):
(WebCore::RenderBlock::availableLogicalHeightForPercentageComputation const):
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::constrainLogicalMinMaxSizesByAspectRatio const):
(WebCore::RenderBox::constrainLogicalHeightByMinMax const):
(WebCore::inlineSizeFromAspectRatio):
Two extra arguments have been added to provide some context when
determining the box type to use (content box or border box). These
arguments are the aspect-ratio type being used for the box along with
whether or not the box is a replaced element with an intrinsic ratio.
css-sizing-4 specifies which box type to use depending on whether the
aspect-ratio property is auto, <ratio>, or auto && <ratio>. If the item
is a replaced element, then it should always work with the content box.

(WebCore::RenderBox::computeLogicalHeight const):
(WebCore::RenderBox::availableLogicalHeightUsing const):
(WebCore::RenderBox::computePositionedLogicalHeightUsing const):
(WebCore::RenderBox::computeLogicalWidthFromAspectRatioInternal const):
(WebCore::RenderBox::isRenderReplacedWithIntrinsicRatio const):
(WebCore::RenderBox::resolveAspectRatio const):
A new helper method to determine which aspect ratio to use depending on
the type of the box. Since RenderReplaced objects have slightly
different logic to compute their aspect ratio, this method does the
work of finding on the aspect ratio that needs to be used and performs
the appropriate calls to compute it. If the object is a RenderReplaced
object, then we need the compute its aspect ratio, otherwise we should
be able to get the box's aspect ratio from its style.

(WebCore::RenderBox::computeMinMaxLogicalWidthFromAspectRatio const):
(WebCore::RenderBox::computeMinMaxLogicalHeightFromAspectRatio const):
horizontalBorderAndPaddingExtent and verticalBorderAndPaddingExtent
were replaced with borderAndPaddingLogicalWidth and
borderAndPaddingLogicalHeight since both blockSizeFromAspectRatio and
inlineSizeFromAspectRatio were expecting the inline and block sizes.
Before, these methods were being passed in the values for the physical
directions and were causing certain tests to fail (e.g.
css/css-flexbox/flex-aspect-ratio-img-column-008.html and
css/css-flexbox/image-as-flexitem-size-007v.html).

* Source/WebCore/rendering/RenderBox.h:
(WebCore::RenderBox::blockSizeFromAspectRatio):
* Source/WebCore/rendering/RenderObject.h:
(WebCore::RenderObject::hasIntrinsicAspectRatio const):
This method was made virtual because it was difficult to provide a valid
answer for SVG elements at this high of an abstraction. Since SVG
elements may not have an aspect ratio, we need those elements to
override this method and provide the answer themselves.

* Source/WebCore/rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::computeAspectRatioInformationForRenderBox const):
(WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes const):
(WebCore::RenderReplaced::computeIntrinsicRatioInformation const):
(WebCore::RenderReplaced::computeReplacedLogicalWidth const):
(WebCore::RenderReplaced::computeReplacedLogicalHeight const):
* Source/WebCore/rendering/RenderReplaced.h:
* Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:
(WebCore::LegacyRenderSVGRoot::hasIntrinsicAspectRatio const):
* Source/WebCore/rendering/svg/LegacyRenderSVGRoot.h:
* Source/WebCore/rendering/svg/RenderSVGRoot.cpp:
(WebCore::RenderSVGRoot::hasIntrinsicAspectRatio const):
* Source/WebCore/rendering/svg/RenderSVGRoot.h:

Canonical link: https://commits.webkit.org/257434@main

42e8fd2

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-AS-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-11-05T00:13:05Z

EWS run on previous version of this PR (hash 6367483)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	~~🧪 api-gtk~~
	⏳ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-05T00:19:29Z

EWS run on previous version of this PR (hash 64d52e6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-07T21:28:25Z

EWS run on previous version of this PR (hash 9127040)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk1
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

cathiechen

Hi @sgill26 ! Thanks for working on this!

cathiechen · 2022-11-08T15:16:00Z

Source/WebCore/rendering/RenderBox.h

    {
-        if (boxSizing == BoxSizing::BorderBox)
+        if (boxSizing == BoxSizing::BorderBox && aspectRatioType == AspectRatioType::Ratio && !isReplacedElementWithIntrinsicRatio)


Should we consider auto && [<ratio>] here?

I believe it should be ok here to leave it out. If it auto && ratio, then this check will fail and it will use the ContentBox dimensions.

However, now that I'm looking at the spec again, maybe this should just be boxSizing == BoxSizing::BorderBox && aspectRatioType == AspectRatioType::Ratio? Since the only time we need the BorderBox size is when aspect-ratio is <ratio> and the box-sizing property is set to border-box.

Thoughts?

https://w3c.github.io/csswg-drafts/css-sizing-4/#aspect-ratio

Yes, I agree with you! Should be boxSizing == BoxSizing::BorderBox && aspectRatioType == AspectRatioType::Ratio.

So it looks like the reason I had this additional check is because css-sizing/aspect-ratio/replaced-element-014.html seems to assert that replaced elements should always use the content box sizing. Maybe we can change this argument to take a bool and pass in isRenderReplaced()?

I think that would allow us to get rid of isReplacedElementWithIntrinsicRatio and only add in the necessary logic in the couple places where it is needed.

I couldn't completely get rid of the method, but I renamed is to isRenderReplacedWithAspectRatio. The method is not used as much as it was before, but it still needs to actually compute the intrinsic ratio. I tried to add the intrinsic ratio as a member field and update it, but it wasn't completely straightforward. It will take a little bit of work to determine when we should actually update the intrinsic ratio and then update it accordingly.

I think that work should probably be done outside the scope of this patch and hopefully we could land this without that logic. If you disagree, though, let me know and I can perhaps see what can be done!

cathiechen · 2022-11-08T15:28:15Z

Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp

+{
+    FloatSize intrinsicSize;
+    double intrinsicRatio = 0;
+    computeIntrinsicRatioInformation(intrinsicSize, intrinsicRatio);


Hmm, this looks like a hacking way to me...

Since I moved the logic of computing the transferred min/max sizes and constraining the computed size into computeReplacedLogicalWidthRespectingMinMaxWidth (and height), we don't need the second method to constrain anymore. We just need computeIntrinsicRatioInformation to get the intrinsic ratio and sizes. All of the constraining will then be done in the other method.

This is no longer true since the change was causing too many issues. computeReplacedLogicalWidthRespectingMinMaxWidth is only constraining with definite sizes in the same dimension. renderreplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes is constraining using the transferred min/max sizes.

cathiechen · 2022-11-08T15:28:25Z

Source/WebCore/rendering/svg/RenderSVGRoot.cpp

+{
+    FloatSize intrinsicSize;
+    double intrinsicRatio;
+    computeIntrinsicRatioInformation(intrinsicSize, intrinsicRatio);


cathiechen · 2022-11-08T15:33:33Z

Source/WebCore/rendering/RenderBox.cpp

+bool RenderBox::isReplacedElementWithIntrinsicRatio() const
+{
+    if (auto* replaced = dynamicDowncast<RenderReplaced>(this))
+        return replaced->computeIntrinsicAspectRatio();


Hmm, this looks like a hacking way to me... And this function is called by many places. Do we have an efficient way to figure out if a replaced element has aspect ratio or not?

That's one of the main issues that I have been trying to figure out. Since not all Replaced Elements have an intrinsic ratio, right now they are being computed on demand.

I think ideally we would store both the intrinsic and ratio as member fields. Then we could have a RenderBox::hasAspectRatio method that would check both style().hasAspectRatio() and the replaced element fields when needed.

I wonder can we just use replaced->intrinsicSize().isEmpty() here?

Unfortunately, I don't think that will work for all cases :( For example, a SVG could have a dimension missing but still have an intrinsic ratio. imported/w3c/web-platform-tests/css/css-flexbox/flex-aspect-ratio-img-row-010.html is an example of a test with such behavior

Source/WebCore/rendering/RenderBox.cpp

cathiechen · 2022-11-08T15:47:02Z

Source/WebCore/rendering/RenderBox.cpp

-    ASSERT(style().hasAspectRatio());
+    ASSERT(style().hasAspectRatio() || isReplacedElementWithIntrinsicRatio());
+
+    double aspectRatio = isReplacedElementWithIntrinsicRatio() ? dynamicDowncast<RenderReplaced>(this)->computeIntrinsicAspectRatio() : style().logicalAspectRatio();


Looks like computeIntrinsicAspectRatio() is called twice if it is replacedElement.

This should be fixed now. I added a resolveAspectRatio method to help determine the aspect ratio to use and it should only call computeIntrinsicAspectRatio once if it is replaced.

cathiechen · 2022-11-08T15:51:30Z

Source/WebCore/rendering/RenderBox.cpp

-        if (LayoutUnit inlineMinSize = computeLogicalWidthInFragmentUsing(MinSize, style().logicalMinWidth(), availableWidth(), *containingBlock(), nullptr); inlineMinSize > LayoutUnit())
-            transferredMinSize = blockSizeFromAspectRatio(horizontalBorderAndPaddingExtent(), verticalBorderAndPaddingExtent(), style().logicalAspectRatio(), style().boxSizingForAspectRatio(), inlineMinSize);
+        if (LayoutUnit inlineMinSize = computeLogicalWidthInFragmentUsing(MinSize, style().logicalMinWidth(), containingBlockLogicalWidthForContent(), *containingBlock(), nullptr); inlineMinSize > LayoutUnit())
+            transferredMinSize = blockSizeFromAspectRatio(borderAndPaddingLogicalWidth(), borderAndPaddingLogicalHeight(), aspectRatio, style().boxSizingForAspectRatio(), inlineMinSize, style().aspectRatioType(), isReplacedElementWithIntrinsicRatio());


Just curious, why do we use borderAndPaddingLogicalWidth(), borderAndPaddingLogicalHeight() instead of horizontalBorderAndPaddingExtent(), verticalBorderAndPaddingExtent() here?

It looks like inlineSizeFromAspectRatio and blockSizeFromAspectRatio expect these two values to be the border+padding of the inline and block sizes. I think it was previously returning the border+padding in terms of physical directions and was causing tests with different writing modes to fail (e.g. css/css-flexbox/flex-aspect-ratio-img-column-008.html and css/css-flexbox/image-as-flexitem-size-007v.html)

cathiechen · 2022-11-08T15:58:51Z

Source/WebCore/rendering/RenderReplaced.cpp


+void RenderReplaced::constrainIntrinsicSize(FloatSize& intrinsicSize, double& intrinsicRatio) const


Feels like using constrainedSize is more consistent with RenderReplaced::computeAspectRatioInformationForRenderBox and the callers.

Do you still think this should be changed with all of the changes computeIntrinsicRatio should only provide the intrinsic size now and not do any constraining.

cathiechen · 2022-11-08T16:16:52Z

Source/WebCore/rendering/RenderReplaced.cpp

+                maxSize = std::max(0_lu, maxSize - borderAndPadding);
+            };
+
+            auto [minLogicalWidth, maxLogicalWidth] = computeMinMaxLogicalWidthFromAspectRatio();


Not sure if this is the right place to call computeMinMaxLogicalWidthFromAspectRatio which is calculating the transferred min/max width. I think logically, computeMinMaxLogicalWidthFromAspectRatio should be used in RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth, which is handling min/max size for replaced elements.
You may take a look at how non-replaced elements work, see RenderBox::constrainLogicalMinMaxSizesByAspectRatio.

I moved this logic to RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth. I also modified RenderReplaced::computeReplacedLogicalWidth (and height) to no longer call these methods directly. Instead, these method are just providing a computed width/height, and then the result is being constrained by the caller so that it looks something like computeReplacedLogicalWidthRespectingMinMaxHeight(computeReplacedLogicalWidth()). There was already some code doing this in other parts of the code, too all of these calls should be consistent now.

Once thing we could potentially do is just call computeReplacedLogicalWidth within computeReplacedLogicalWidthRespectingMinMaxWidth, so that it looks a little cleaner but let me know what you think.

This change was causing a lot of issues so I reverted it back to the original version.

webkit-early-warning-system · 2022-11-15T07:43:44Z

EWS run on previous version of this PR (hash c319764)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	❌ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 api-mac	✅ 🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-wk1
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2
	❌ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	❌ 🛠 watch-sim	❌ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-15T17:30:16Z

EWS run on previous version of this PR (hash a46b29a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-15T22:48:43Z

EWS run on previous version of this PR (hash 8817b6a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-16T07:14:07Z

EWS run on previous version of this PR (hash 1eb4716)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	~~🧪 mac-wk2-stress~~

webkit-early-warning-system · 2022-11-16T07:27:33Z

EWS run on previous version of this PR (hash 888f34a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-16T23:51:57Z

EWS run on previous version of this PR (hash 6a2ff5d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-17T06:48:29Z

EWS run on previous version of this PR (hash edf3b47)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-05T23:29:02Z

EWS run on previous version of this PR (hash b271f65)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-06T21:06:29Z

EWS run on current version of this PR (hash 42e8fd2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-AS-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch-sim

…respect min-max constraints in opposite dimension. https://bugs.webkit.org/show_bug.cgi?id=247507 rdar://101979495 Reviewed by Alan Baradlay. There are certain scenarios where a replaced element may have a specified aspect ratio, but will not have intrinsic sizes in both dimensions. One such examples is a SVG with only a specified width and aspect ratio. To help facilitate this, the logic in RenderReplaced::computeAspectRatioInformationForRenderBox has been separated out since it was doing too much before. It was both computing the intrinsic size/ratio and constraining the intrinsic size. The logic for constraining the intrinsic size has been refactored out into a new method called RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes. This method will call In these scenarios, we must use the specified aspect ratio to do any sort of sizing since it is not possible to compute it from the dimensions of the box. computeAspectRatioInformationForRenderBox to get the intrinsic size/ratio and then constrain it. This new method will also constrain the sizes in each dimension by the transferred sizes. This size may be then potentially constrained even further in RenderReplaced::computeReplacedLogicalWidthRespectingMinMaxWidth (and height) by any definite min/max sizes in that dimension. There was an attempt to consolidate all constraining logic into that method, but that was causing some unexpected results. That may need to be done in a future patch. A few of the CSS aspect ratio methods have also been modified to help with this logic. Since a lot of the logic is shared, these methods can determine if a replaced element is being used and use the correct aspect ratio in that case. A new member variable to RenderReplaced has been added: m_intrinsicRatioSize. This is used to keep track of the width and height sizes associated with the aspect ratio. This is because there were some precision issues that this patch exposed. Certain tests were starting to fail because the computed width/height from the aspect ratio was slightly off. To help with this, we can store the intrinsic ratio sizes until we actually need to use them to compute the width/height. The goal here is to hopefully reduce the number of floating point operations that are needed to compute a size. It may be possible to replace all instances where the computed intrinsicRatio double is being used, but that is probably a little too risky for this patch. Instead, this patch only uses it in the call to resolveHeightForRatio within RenderReplaced::computeReplacedLogicalHeight. It is probably best to update all uses of intrinsicRatio to m_intrinsicRatioSize in another patch. To compute the intrinsic ratio size we needed to keep track of a new variable inside the computeIntrinsicRatioInformation logic and then update the member variable once it was computed. Spec references: https://www.w3.org/TR/CSS22/visudet.html#min-max-widths https://www.w3.org/TR/CSS22/visudet.html#min-max-heights https://w3c.github.io/csswg-drafts/css-sizing-4/#aspect-ratio * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-039-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-039.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-040-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/replaced-element-040.html: Added. * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::computeChildPreferredLogicalWidths const): (WebCore::RenderBlock::availableLogicalHeightForPercentageComputation const): * Source/WebCore/rendering/RenderBox.cpp: (WebCore::RenderBox::constrainLogicalMinMaxSizesByAspectRatio const): (WebCore::RenderBox::constrainLogicalHeightByMinMax const): (WebCore::inlineSizeFromAspectRatio): Two extra arguments have been added to provide some context when determining the box type to use (content box or border box). These arguments are the aspect-ratio type being used for the box along with whether or not the box is a replaced element with an intrinsic ratio. css-sizing-4 specifies which box type to use depending on whether the aspect-ratio property is auto, <ratio>, or auto && <ratio>. If the item is a replaced element, then it should always work with the content box. (WebCore::RenderBox::computeLogicalHeight const): (WebCore::RenderBox::availableLogicalHeightUsing const): (WebCore::RenderBox::computePositionedLogicalHeightUsing const): (WebCore::RenderBox::computeLogicalWidthFromAspectRatioInternal const): (WebCore::RenderBox::isRenderReplacedWithIntrinsicRatio const): (WebCore::RenderBox::resolveAspectRatio const): A new helper method to determine which aspect ratio to use depending on the type of the box. Since RenderReplaced objects have slightly different logic to compute their aspect ratio, this method does the work of finding on the aspect ratio that needs to be used and performs the appropriate calls to compute it. If the object is a RenderReplaced object, then we need the compute its aspect ratio, otherwise we should be able to get the box's aspect ratio from its style. (WebCore::RenderBox::computeMinMaxLogicalWidthFromAspectRatio const): (WebCore::RenderBox::computeMinMaxLogicalHeightFromAspectRatio const): horizontalBorderAndPaddingExtent and verticalBorderAndPaddingExtent were replaced with borderAndPaddingLogicalWidth and borderAndPaddingLogicalHeight since both blockSizeFromAspectRatio and inlineSizeFromAspectRatio were expecting the inline and block sizes. Before, these methods were being passed in the values for the physical directions and were causing certain tests to fail (e.g. css/css-flexbox/flex-aspect-ratio-img-column-008.html and css/css-flexbox/image-as-flexitem-size-007v.html). * Source/WebCore/rendering/RenderBox.h: (WebCore::RenderBox::blockSizeFromAspectRatio): * Source/WebCore/rendering/RenderObject.h: (WebCore::RenderObject::hasIntrinsicAspectRatio const): This method was made virtual because it was difficult to provide a valid answer for SVG elements at this high of an abstraction. Since SVG elements may not have an aspect ratio, we need those elements to override this method and provide the answer themselves. * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::RenderReplaced::computeAspectRatioInformationForRenderBox const): (WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes const): (WebCore::RenderReplaced::computeIntrinsicRatioInformation const): (WebCore::RenderReplaced::computeReplacedLogicalWidth const): (WebCore::RenderReplaced::computeReplacedLogicalHeight const): * Source/WebCore/rendering/RenderReplaced.h: * Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp: (WebCore::LegacyRenderSVGRoot::hasIntrinsicAspectRatio const): * Source/WebCore/rendering/svg/LegacyRenderSVGRoot.h: * Source/WebCore/rendering/svg/RenderSVGRoot.cpp: (WebCore::RenderSVGRoot::hasIntrinsicAspectRatio const): * Source/WebCore/rendering/svg/RenderSVGRoot.h: Canonical link: https://commits.webkit.org/257434@main

webkit-commit-queue · 2022-12-06T22:30:51Z

Committed 257434@main (036e53a): https://commits.webkit.org/257434@main

Reviewed commits have been landed. Closing PR #6159 and removing active labels.

sammygill self-assigned this Nov 5, 2022

sammygill added Layout and Rendering For bugs with layout and rendering of Web pages. WebKit Nightly Build labels Nov 5, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 6367483 to 64d52e6 Compare November 5, 2022 00:18

sammygill requested review from rwlbuis and cathiechen November 5, 2022 00:22

sammygill mentioned this pull request Nov 5, 2022

Use intrinsic ratio of replaced elements when computing flex sizes #6155

Merged

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 5, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 7, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 64d52e6 to 9127040 Compare November 7, 2022 21:28

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 7, 2022

cathiechen reviewed Nov 8, 2022

View reviewed changes

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 9127040 to c319764 Compare November 15, 2022 07:39

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from c319764 to a46b29a Compare November 15, 2022 17:24

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from a46b29a to 8817b6a Compare November 15, 2022 22:48

sammygill changed the title ~~Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension.~~ wip Nov 15, 2022

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 16, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 8817b6a to 1eb4716 Compare November 16, 2022 07:13

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 16, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 1eb4716 to 888f34a Compare November 16, 2022 07:27

sammygill changed the title ~~wip~~ Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension. Nov 16, 2022

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 16, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Nov 17, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 6a2ff5d to edf3b47 Compare November 17, 2022 06:48

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 17, 2022

sammygill requested a review from cathiechen November 17, 2022 23:46

xeenon removed the WebKit Nightly Build label Nov 21, 2022

alanbaradlay approved these changes Dec 5, 2022

View reviewed changes

sammygill removed the merging-blocked Applied to prevent a change from being merged label Dec 5, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from edf3b47 to b271f65 Compare December 5, 2022 23:28

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 6, 2022

sammygill force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from b271f65 to 42e8fd2 Compare December 6, 2022 21:04

sammygill added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Dec 6, 2022

webkit-early-warning-system force-pushed the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch from 42e8fd2 to 036e53a Compare December 6, 2022 22:29

webkit-early-warning-system merged commit 036e53a into WebKit:main Dec 6, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 6, 2022

sammygill deleted the eng/Replaced-elements-with-intrinsic-ratio-and-missing-size-dont-respect-min-max-constraints branch April 19, 2024 17:00


		void RenderReplaced::constrainIntrinsicSize(FloatSize& intrinsicSize, double& intrinsicRatio) const

Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension. #6159

Replaced elements with aspect ratio and size in one dimension should respect min-max constraints in opposite dimension. #6159

Conversation

sammygill commented Nov 5, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 5, 2022 • edited

webkit-early-warning-system commented Nov 5, 2022 • edited

webkit-early-warning-system commented Nov 7, 2022 • edited

cathiechen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 15, 2022 • edited

webkit-early-warning-system commented Nov 15, 2022 • edited

webkit-early-warning-system commented Nov 15, 2022 • edited

webkit-early-warning-system commented Nov 16, 2022 • edited

webkit-early-warning-system commented Nov 16, 2022 • edited

webkit-early-warning-system commented Nov 16, 2022 • edited

webkit-early-warning-system commented Nov 17, 2022 • edited

webkit-early-warning-system commented Dec 5, 2022 • edited

webkit-early-warning-system commented Dec 6, 2022 • edited

webkit-commit-queue commented Dec 6, 2022

sammygill commented Nov 5, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 5, 2022 •

edited

webkit-early-warning-system commented Nov 5, 2022 •

edited

webkit-early-warning-system commented Nov 7, 2022 •

edited

webkit-early-warning-system commented Nov 15, 2022 •

edited

webkit-early-warning-system commented Nov 15, 2022 •

edited

webkit-early-warning-system commented Nov 15, 2022 •

edited

webkit-early-warning-system commented Nov 16, 2022 •

edited

webkit-early-warning-system commented Nov 16, 2022 •

edited

webkit-early-warning-system commented Nov 16, 2022 •

edited

webkit-early-warning-system commented Nov 17, 2022 •

edited

webkit-early-warning-system commented Dec 5, 2022 •

edited

webkit-early-warning-system commented Dec 6, 2022 •

edited