Skip to content

Commit

Permalink
Cherry-pick 260851@main (e6a3195). https://bugs.webkit.org/show_bug.c…
Browse files Browse the repository at this point in the history 
…gi?id=252589

    RenderReplaced::computeAspectRatioInformationForRenderBox does not need to call into RenderBox::computeReplacedLogicalWidth
    https://bugs.webkit.org/show_bug.cgi?id=252589
    rdar://105489410

    Reviewed by Alan Baradlay.

    The call to RenderBox::computeReplacedLogicalWidth from RenderReplaced::computeAspectRatioInformationForRenderBox
    when the object was a video with a default object size was extraneous
    and can be safely removed without changing any functionality. It could
    also result in a stack overflow in certain conditions. At this
    point we know the intrinsic sizes of the object anyway since it is a
    video with a default object size (300px x 150px). This is
    because:

    1.) RenderBox::computeReplacedLogicalWidth would call into computeReplacedLogicalWidthUsing(MainOrPreferredSize, style().logicalWidth())
        com compute the width value to use
    2.) The caller (RenderReplaced::computeReplacedLogicalWidth in this case)
        will only use the width value of style().logicalWidth().isAuto()
        returns true.
    3.) If this is true RenderBox::computeReplacedLogicalWidth would have
        returned the intrinsic width but constrained to min/max sizes. However,
        the caller constrains the intrinsic width anyway.
    4.) If this is false (style().logicalWidth().isAuto() returns false),
        then the caller (RenderReplaced::computeReplacedLogicalWidth) will
        use the intrinsic width anyway by calling into intrinsicLogicalWidth.

    We can also take advantage of the existing logic to constrain the
    intrinsic sizes in each dimension by transferred constraints from the
    opposite one.

    Here is an example of when this extra call could cause a stack overflow:

    <style>
    video {
      aspect-ratio: 1;
      container-type: inline-size;
      inset: 0 auto;
      min-width: min-content;
      position: fixed;
    }
    </style>
    <video></video>

    WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const
    WebCore::RenderBox::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const
    WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes(WebCore::RenderBox*, WebCore::FloatSize&, WebCore::FloatSize&) const
    WebCore::RenderReplaced::computeReplacedLogicalHeight(std::__1::optional<WebCore::LayoutUnit>) const
    WebCore::RenderImage::computeReplacedLogicalHeight(std::__1::optional<WebCore::LayoutUnit>) const
    WebCore::RenderBox::computePositionedLogicalHeightReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const
    WebCore::RenderBox::computePositionedLogicalHeight(WebCore::RenderBox::LogicalExtentComputedValues&) const
    WebCore::RenderBox::computeLogicalHeight(WebCore::LayoutUnit, WebCore::LayoutUnit) const
    WebCore::RenderBox::computeLogicalWidthFromAspectRatioInternal() const
    WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, WebCore::LayoutUnit, WebCore::LayoutUnit) const
    WebCore::RenderBox::computeReplacedLogicalWidthUsing(WebCore::SizeType, WebCore::Length) const
    WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const
    WebCore::RenderVideo::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const
    WebCore::RenderBox::computePositionedLogicalWidthReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const
    WebCore::RenderBox::computePositionedLogicalWidth(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const
    WebCore::RenderBox::computeLogicalWidthInFragment(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const
    WebCore::RenderBox::updateLogicalWidth()
    WebCore::RenderReplaced::layout()
    WebCore::RenderImage::layout()
    WebCore::RenderMedia::layout()
    WebCore::RenderVideo::layout()

    After we have computed the width for the video, we call RenderBox::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth
    to restrict it within any min and max constraints. This eventually calls into RenderBox::computeIntrinsicLogicalWidthUsing to compute the
    min-width using MinContent as the width type.

    The container-type: inline-size; and inset: 0 auto; properties cause RenderBox::shouldComputeLogicalWidthFromAspectRatio to return true
    because the call to shouldComputeLogicalWidthFromAspectRatioAndInsets returns true. This last method returns true because:
    1.) Applying the inline size containment causes hasConstrainedWidth to be false when it would normally be true from the video’s intrinsic width.
    2.) hasConstrainedHeight gets set to false because inset causes the logical top and property values to get set to a fixed value
    3.) The final call to style.logicalHeight().isAuto() returns true

    Due to these series of events we end up calling computeLogicalWidthFromAspectRatioInternal inside RenderBox::computeIntrinsicLogicalWidthUsing.
    This eventually recurses its way back into RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth through the reset of the stack trace.

    * LayoutTests/TestExpectations:
    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html: Added.
    This test currently crashes due to a different reason (webkit.org/b/252594),
    but is still useful to this patch as it was causing the stack
    overflow.

    * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-intrinsic-width-height.html:
    One of the test cases was incorrectly changed in a previous patch and
    is being restored to its previous version. The height for this test
    should be 150px and not 300px because it falls under the case:

    If 'height' and 'width' both have computed values of 'auto' and the
    element also has an intrinsic height, then that intrinsic height is
    the used value of 'height'.
    https://www.w3.org/TR/CSS22/visudet.html#inline-replaced-height

    * LayoutTests/platform/gtk/TestExpectations:
    * LayoutTests/platform/wpe/TestExpectations:
    * Source/WebCore/rendering/RenderReplaced.cpp:
    (WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes const):

    Canonical link: https://commits.webkit.org/260851@main
  • Loading branch information
@sammygill @aperezdc
sammygill authored and aperezdc committed Apr 10, 2023
1 parent 2804afc commit 85145ca
Show file tree
Hide file tree
Showing 10 changed files with 103 additions and 21 deletions.
1 change: 1 addition & 0 deletions LayoutTests/TestExpectations
View file
Expand Up @@ -827,6 +827,7 @@ imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/propagate-t
imported/w3c/web-platform-tests/html/rendering/widgets/the-select-element/option-checked-styling.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/legend-painting-order.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/input-image-inline-alt.html [ ImageOnlyFailure ]
webkit.org/b/252594 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html [ Skip ]
imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/cross-domain-iframe.sub.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/tall-cross-domain-iframe-in-scrolled.sub.html [ ImageOnlyFailure ]
imported/w3c/web-platform-tests/html/rendering/the-details-element/details-page-break-after-1-print.html [ ImageOnlyFailure ]
Expand Down
10 changes: 10 additions & 0 deletions ...ded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html
View file
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
</head>
<body>
<p>Test passes if there is a filled green square.</p>
<div style="width:100px; height: 100px; background-color: green;"></div>
</body>
</html>
22 changes: 22 additions & 0 deletions ...for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html
View file
@@ -0,0 +1,22 @@
<!DOCTYPE html>
<html>
<head>
<meta name="assert" content="Video that has no intrinsic size should have height constrained by max-width">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css-sizing-4/#aspect-ratio-size-transfers">
<link author="Sammy Gill" href="mailto:sammy.gill@apple.com">
<link rel="match" href="/css/reference/ref-filled-green-100px-square.xht">
<style>
video {
background-color: green;
width: auto;
height: auto;
max-width: 100px;
}
</style>
</head>
<!-- aspect-ratio should be set to: auto 100 / 100-->
<body>
<p>Test passes if there is a filled green square.</p>
<video width="100" height="100" src="/css/support/60x60-green.png">
</body>
</html>
10 changes: 10 additions & 0 deletions ...ded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html
View file
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
</head>
<body>
<p>Test passes if there is a filled green square.</p>
<div style="width:100px; height: 100px; background-color: green;"></div>
</body>
</html>
22 changes: 22 additions & 0 deletions ...for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html
View file
@@ -0,0 +1,22 @@
<!DOCTYPE html>
<html>
<head>
<meta name="assert" content="Video that has no intrinsic size should have width constrained by max-height">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css-sizing-4/#aspect-ratio-size-transfers">
<link author="Sammy Gill" href="mailto:sammy.gill@apple.com">
<link rel="match" href="/css/reference/ref-filled-green-100px-square.xht">
<style>
video {
background-color: green;
width: auto;
height: auto;
max-height: 100px;
}
</style>
</head>
<!-- aspect-ratio should be set to: auto 100 / 100-->
<body>
<p>Test passes if there is a filled green square.</p>
<video width="100" height="100" src="/css/support/60x60-green.png">
</body>
</html>
19 changes: 19 additions & 0 deletions ...ts/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html
View file
@@ -0,0 +1,19 @@
<!DOCTYPE html>
<html>
<head>
<link author="Sammy Gill" href="mailto:sammy.gill@apple.com">
<meta name="assert" content="Applying inline-size containment to the video should not result in a debug assert being triggered">
<style>
video {
aspect-ratio: 1;
container-type: inline-size;
inset: 0 auto;
min-width: min-content;
position: fixed;
}
</style>
</head>
<body>
<video></video>
</body>
</html>
2 changes: 1 addition & 1 deletion ...ced-elements/attributes-for-embedded-content-and-images/video-intrinsic-width-height.html
View file
Expand Up @@ -25,7 +25,7 @@
<!-- A width:height ratio other than 2:1 and overriding the specified style must be used to
verify that width/height does not influence intrinsic ratio -->
<video title="both width/height attributes and style"
data-expected-width="300" data-expected-height="300"
data-expected-width="300" data-expected-height="150"
width="100" height="100" style="width: auto; height: auto"></video>
<script>
Array.prototype.forEach.call(document.querySelectorAll('video'), function(video)
Expand Down
2 changes: 2 additions & 0 deletions LayoutTests/platform/gtk/TestExpectations
View file
Expand Up @@ -1927,6 +1927,8 @@ imported/w3c/web-platform-tests/xhr/preserve-ua-header-on-redirect.htm [ Failure

imported/w3c/web-platform-tests/xhr/send-authentication-basic-setrequestheader-existing-session.htm [ Failure ]

webkit.org/b/252953 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html [ ImageOnlyFailure ]
webkit.org/b/252953 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html [ ImageOnlyFailure ]
# Ftp code is disabled in gtk port
http/tests/misc/ftp-eplf-directory.py [ Skip ]

Expand Down
3 changes: 3 additions & 0 deletions LayoutTests/platform/wpe/TestExpectations
View file
Expand Up @@ -863,6 +863,9 @@ webkit.org/b/227061 imported/w3c/web-platform-tests/css/css-ui/outline-negative-
webkit.org/b/224645 imported/w3c/web-platform-tests/css/css-text/word-break/word-break-break-all-inline-006.html [ ImageOnlyFailure Pass ]
webkit.org/b/224645 imported/w3c/web-platform-tests/css/css-text/word-break/word-break-break-all-inline-009.html [ ImageOnlyFailure Pass ]

webkit.org/b/252954 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html [ ImageOnlyFailure ]
webkit.org/b/252954 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html [ ImageOnlyFailure ]

#////////////////////////////////////////////////////////////////////////////////////////
# 3. UNRESOLVED TESTS
#////////////////////////////////////////////////////////////////////////////////////////
Expand Down
33 changes: 13 additions & 20 deletions Source/WebCore/rendering/RenderReplaced.cpp
View file
Expand Up @@ -440,26 +440,19 @@ void RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes(Re
// having to shrink in order to preserve the aspect ratio. Because we compute these values independently along
// each axis, the final returned size may in fact not preserve the aspect ratio.
if (!intrinsicRatio.isZero() && style().logicalWidth().isAuto() && style().logicalHeight().isAuto()) {
if (isVideoWithDefaultObjectSize(this)) {
LayoutUnit width = RenderBox::computeReplacedLogicalWidth();
intrinsicSize.setWidth(width);
LayoutUnit height = blockSizeFromAspectRatio(horizontalBorderAndPaddingExtent(), verticalBorderAndPaddingExtent(), intrinsicRatio.aspectRatioDouble(), BoxSizing::ContentBox, adjustBorderBoxLogicalWidthForBoxSizing(width, LengthType::Fixed), style().aspectRatioType(), true);
intrinsicSize.setHeight(height.round() - verticalBorderAndPaddingExtent());
} else {
auto removeBorderAndPaddingFromMinMaxSizes = [](LayoutUnit& minSize, LayoutUnit &maxSize, LayoutUnit borderAndPadding) {
minSize = std::max(0_lu, minSize - borderAndPadding);
maxSize = std::max(0_lu, maxSize - borderAndPadding);
};

auto [minLogicalWidth, maxLogicalWidth] = computeMinMaxLogicalWidthFromAspectRatio();
removeBorderAndPaddingFromMinMaxSizes(minLogicalWidth, maxLogicalWidth, borderAndPaddingLogicalWidth());

auto [minLogicalHeight, maxLogicalHeight] = computeMinMaxLogicalHeightFromAspectRatio();
removeBorderAndPaddingFromMinMaxSizes(minLogicalHeight, maxLogicalHeight, borderAndPaddingLogicalHeight());

intrinsicSize.setWidth(std::clamp(LayoutUnit { intrinsicSize.width() }, minLogicalWidth, maxLogicalWidth));
intrinsicSize.setHeight(std::clamp(LayoutUnit { intrinsicSize.height() }, minLogicalHeight, maxLogicalHeight));
}
auto removeBorderAndPaddingFromMinMaxSizes = [](LayoutUnit& minSize, LayoutUnit &maxSize, LayoutUnit borderAndPadding) {
minSize = std::max(0_lu, minSize - borderAndPadding);
maxSize = std::max(0_lu, maxSize - borderAndPadding);
};

auto [minLogicalWidth, maxLogicalWidth] = computeMinMaxLogicalWidthFromAspectRatio();
removeBorderAndPaddingFromMinMaxSizes(minLogicalWidth, maxLogicalWidth, borderAndPaddingLogicalWidth());

auto [minLogicalHeight, maxLogicalHeight] = computeMinMaxLogicalHeightFromAspectRatio();
removeBorderAndPaddingFromMinMaxSizes(minLogicalHeight, maxLogicalHeight, borderAndPaddingLogicalHeight());

intrinsicSize.setWidth(std::clamp(LayoutUnit { intrinsicSize.width() }, minLogicalWidth, maxLogicalWidth));
intrinsicSize.setHeight(std::clamp(LayoutUnit { intrinsicSize.height() }, minLogicalHeight, maxLogicalHeight));
}
}

Expand Down

0 comments on commit 85145ca

Please sign in to comment.