Skip to content

Commit

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

    REGRESSION (259663@main): lowes.com: Product image is blank
    https://bugs.webkit.org/show_bug.cgi?id=254603
    rdar://106926883

    Reviewed by Alan Baradlay.

    The product image on Lowes had a structure that is very similar
    to the following that results in the product image not rendering
    correctly (having a width and height of 0). I made some slight
    modifications to make it easier to digest, but the following is
    indicative of the issue that is causing the image to show up as blank.

    body {
      height: 50px;
    }
    img {
      position: relative;
      max-width: 100%;
      max-height: 100%;
    }
    .outer-flexbox {
      display: flex;
      width: 100%;
      height: 100%;
    }
    .outer-flexbox-item {
      position: relative;
      min-width: 100%;
    }
    .inner-flexbox {
      position: absolute;
      display: flex;
      inset: 0px;
    }
    </style>

    <div class="outer-flexbox">
        <div class="outer-flexbox-item">
            <div class="inner-flexbox">
                <div>
                    <img src="/css/support/60x60-green.png">
                </div>
            </div>
        </div>
    </div>

    The problem arises when we layout the outer-flexbox and eventually
    recurse into the image to compute its preferred width. During this
    process, we attempt to compute the max-height by resolving the percentage value, but we end up
    incorrectly computing a max-height of 0. This max-height computation is
    done when we reach RenderBox::computeLogicalHeightUsing and end up
    calling RenderBox::computeReplacedLogicalHeightUsing with a heightType
    of MaxSize. computeReplacedLogicalHeightUsing will calculate this height
    differently depending on the LengthType of the passed in height and in the
    case of this scenario we fall into the LengthType::Percent case for
    max-height. Since this code is unable to resolve this height (due to the
    fact its containing block depends on the size of its content), it
    returns a value of 0. This 0 value ends up affecting not only the size
    of the image in both the width and height dimensions, but also affects
    the flex item of the inner flexbox and the size of the inner flexbox as
    part of a flex item for the outer flexbox.

    The solution here is to modify computeLogicalHeightUsing to check if
    the min/max height would compute to 0/none depending on the heightType.
    Ideally, the caller should not have to do this (like how it is done in
    RenderBox::computeReplacedLogicalHeightRespectingMinMaxHeight), and
    this would be done handled in computeReplacedLogicalHeightUsing, but
    a couple of call sites make this type of change tricky. It is
    particularly when computeReplacedLogicalHeightUsing with a heightType of
    MainOrPreferredSize since these call sites expect the function to return
    a definite value, so it is not clear what would be the correct logic if
    it instead returned an empty optional.

    To accomplish this, we can use the existing helper function
    replacedMinMaxLogicalHeightComputesAsNone, which returns true in this
    example to indicate that the used value of max-height should be treated
    as none. This was actually already being used in the other call site:
    computeReplacedLogicalHeightRespectingMinMaxHeight. To make it more
    difficult to call this function when replacedMinMaxLogicalHeightComputesAsNone
    returns false, I added an assert to trigger in that problematic scenario.

    * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/grid-item-image-percentage-min-height-computes-as-0-expected.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/grid-item-image-percentage-min-height-computes-as-0.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/nested-flexbox-image-percentage-max-height-computes-as-none-expected.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/nested-flexbox-image-percentage-max-height-computes-as-none.html: Added.
    * Source/WebCore/rendering/RenderBox.cpp:
    (WebCore::RenderBox::computeLogicalHeightUsing const):
    (WebCore::RenderBox::computeReplacedLogicalHeightUsing const):

    Canonical link: https://commits.webkit.org/262342@main
  • Loading branch information
@sammygill @aperezdc
sammygill authored and aperezdc committed Apr 25, 2023
1 parent 3c5c6c1 commit 2b76d74
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 2 deletions.
10 changes: 10 additions & 0 deletions ...rm-tests/css/css-sizing/grid-item-image-percentage-min-height-computes-as-0-expected.html
View file
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<style>
</style>
</head>
<body>
<img src="/css/support/60x60-green.png">
</body>
</html>
28 changes: 28 additions & 0 deletions ...eb-platform-tests/css/css-sizing/grid-item-image-percentage-min-height-computes-as-0.html
View file
@@ -0,0 +1,28 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css2/#propdef-min-height">
<link rel="match" href="grid-item-image-percentage-min-height-computes-as-0-ref.html">
<meta name="assert" content="Image percentage min-height cannot be resolved and used value should be 0">
<style>
.grid {
display: grid;
}
.grid-item {
display: grid;
}
.grid-item img {
height: var(--grid-item-height, auto);
min-height: 100%;
}
</style>
</head>
<body>
<div class="grid">
<div class="grid-item">
<img src="/css/support/60x60-green.png">
</div>
</div>
</body>
</html>
10 changes: 10 additions & 0 deletions .../css/css-sizing/nested-flexbox-image-percentage-max-height-computes-as-none-expected.html
View file
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<style>
</style>
</head>
<body>
<img src="/css/support/60x60-green.png">
</body>
</html>
47 changes: 47 additions & 0 deletions ...orm-tests/css/css-sizing/nested-flexbox-image-percentage-max-height-computes-as-none.html
View file
@@ -0,0 +1,47 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css2/#propdef-max-height">
<link rel="match" href="nested-flexbox-image-percentage-max-height-computes-as-none-ref.html">
<meta name="assert" content="Image percentage max-height cannot be resolved and used value should be none">
<style>
body {
height: 100px;
}
svg {
position: relative;
max-width: 100%;
max-height: 100%;
contain: size;
contain-intrinsic-size: 60px 60px;
aspect-ratio: 1/1;
}
.outer-flexbox {
display: flex;
width: 100%;
height: 100%;
}
.outer-flexbox-item {
position: relative;
min-width: 100%;
}
.inner-flexbox {
position: absolute;
display: flex;
inset: 0px;
}
</style>
</head>
<body>
<div class="outer-flexbox">
<div class="outer-flexbox-item">
<div class="inner-flexbox">
<div>
<svg viewBox="0 0 1 1" style="background: green"></svg>
</div>
</div>
</div>
</div>
</body>
</html>
14 changes: 12 additions & 2 deletions Source/WebCore/rendering/RenderBox.cpp
View file
Expand Up @@ -3250,8 +3250,11 @@ LayoutUnit RenderBox::computeLogicalHeightWithoutLayout() const

std::optional<LayoutUnit> RenderBox::computeLogicalHeightUsing(SizeType heightType, const Length& height, std::optional<LayoutUnit> intrinsicContentHeight) const
{
if (is<RenderReplaced>(this))
return computeReplacedLogicalHeightUsing(heightType, height) + borderAndPaddingLogicalHeight();
if (is<RenderReplaced>(this)) {
if ((heightType == MinSize || heightType == MaxSize) && !replacedMinMaxLogicalHeightComputesAsNone(heightType))
return computeReplacedLogicalHeightUsing(heightType, height) + borderAndPaddingLogicalHeight();
return std::nullopt;
}
if (std::optional<LayoutUnit> logicalHeight = computeContentAndScrollbarLogicalHeightUsing(heightType, height, intrinsicContentHeight))
return adjustBorderBoxLogicalHeightForBoxSizing(logicalHeight.value());
return std::nullopt;
Expand Down Expand Up @@ -3566,6 +3569,13 @@ LayoutUnit RenderBox::computeReplacedLogicalHeightRespectingMinMaxHeight(LayoutU
LayoutUnit RenderBox::computeReplacedLogicalHeightUsing(SizeType heightType, Length logicalHeight) const
{
ASSERT(heightType == MinSize || heightType == MainOrPreferredSize || !logicalHeight.isAuto());
#if ASSERT_ENABLED
// This function should get called with MinSize/MaxSize only if replacedMinMaxLogicalHeightComputesAsNone
// returns false, otherwise we should not try to compute those values as they may be incorrect. The caller
// should make sure this condition holds before calling this function
if (heightType == MinSize || heightType == MaxSize)
ASSERT(!replacedMinMaxLogicalHeightComputesAsNone(heightType));
#endif
if (heightType == MinSize && logicalHeight.isAuto())
return adjustContentBoxLogicalHeightForBoxSizing(std::optional<LayoutUnit>(0));

Expand Down

0 comments on commit 2b76d74

Please sign in to comment.