Skip to content

Commit

Permalink
(REGRESSION 262277@main) Incorrect float placement when clear is present
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=260202
<rdar://111577468>

Reviewed by Antti Koivisto.

When no float is present at the clear direction it still needs to "clear" the existing floats.

e.g.

<div style="float: left"></div>
<div style="float: left; clear: right;"></div>

(notice we are supposed to clear right, but there's no right float in this context)

This patch ensures that we take existing floats into account for the initial position when there's nothing to clear.

* LayoutTests/fast/block/float/float-left-with-clear-right-incorrect-position-expected.html: Added.
* LayoutTests/fast/block/float/float-left-with-clear-right-incorrect-position.html: Added.
* Source/WebCore/layout/floats/FloatingContext.cpp:
(WebCore::Layout::FloatingContext::positionForFloat const):
(WebCore::Layout::FloatingContext::bottom const):

Canonical link: https://commits.webkit.org/266918@main
  • Loading branch information
alanbaradlay committed Aug 15, 2023
1 parent 30959c8 commit 0c02598
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 26 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<style>
.container {
width: 50px;
}

.box {
width: 50px;
height: 50px;
}
</style>
<div class=container>
<div class=box></div>
<div class=box style="background-color: green"></div>
<div class=box></div>
PASS if green box is visible.
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<style>
.container {
width: 50px;
}

.float {
width: 50px;
height: 50px;
float: left;
}

.sibling {
background-color: green;
}

.clear_box {
clear: right;
background-color: white;
}
</style>
<div class=container>
<div class=float></div>
<div class="float sibling"></div>
<div class="float clear_box"></div>
PASS if green box is visible.
</div>
50 changes: 24 additions & 26 deletions Source/WebCore/layout/floats/FloatingContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -228,34 +228,33 @@ LayoutPoint FloatingContext::positionForFloat(const Box& layoutBox, const BoxGeo

// Find the top most position where the float box fits.
ASSERT(!isEmpty());
auto clearPosition = [&]() -> std::optional<LayoutUnit> {
if (!layoutBox.hasFloatClear())
return { };
// The vertical position candidate needs to clear the existing floats in this context.
switch (clearInFloatingState(layoutBox)) {
case Clear::Left:
return leftBottom();
case Clear::Right:
return rightBottom();
case Clear::Both:
return bottom();
default:
ASSERT_NOT_REACHED();
}
return { };
};

auto absoluteCoordinates = this->absoluteCoordinates(layoutBox, borderBoxTopLeft);
auto absoluteTopLeft = absoluteCoordinates.topLeft;
auto verticalPositionCandidate = absoluteTopLeft.y();
// Incoming float cannot be placed higher than existing floats (margin box of the last float).
// Take the static position (where the box would go if it wasn't floating) and adjust it with the last float.
auto lastFloatAbsoluteTop = floatingState().floats().last().absoluteRectWithMargin().top();
auto lastOrClearedFloatPosition = std::max(clearPosition().value_or(lastFloatAbsoluteTop), lastFloatAbsoluteTop);
if (verticalPositionCandidate - boxGeometry.marginBefore() < lastOrClearedFloatPosition)
verticalPositionCandidate = lastOrClearedFloatPosition + boxGeometry.marginBefore();

if (layoutBox.hasFloatClear()) {
// The vertical position candidate needs to clear the existing floats in this context.
auto floatBottom = [&]() -> std::optional<LayoutUnit> {
switch (clearInFloatingState(layoutBox)) {
case Clear::Left:
return leftBottom();
case Clear::Right:
return rightBottom();
case Clear::Both:
return bottom();
default:
ASSERT_NOT_REACHED();
}
return { };
};
if (auto bottomWithClear = floatBottom())
verticalPositionCandidate = std::max(borderBoxTopLeft.y(), *bottomWithClear) + boxGeometry.marginBefore();
} else {
// Incoming float cannot be placed higher than existing floats (margin box of the last float).
// Take the static position (where the box would go if it wasn't floating) and adjust it with the last float.
auto previousFloatAbsoluteTop = floatingState().floats().last().absoluteRectWithMargin().top();
if (verticalPositionCandidate - boxGeometry.marginBefore() < previousFloatAbsoluteTop)
verticalPositionCandidate = previousFloatAbsoluteTop + boxGeometry.marginBefore();
}
absoluteTopLeft.setY(verticalPositionCandidate);
auto margins = Edges { { boxGeometry.marginStart(), boxGeometry.marginEnd() }, { boxGeometry.marginBefore(), boxGeometry.marginAfter() } };
auto floatBox = FloatAvoider { absoluteTopLeft, boxGeometry.borderBoxWidth(), margins, absoluteCoordinates.containingBlockContentBox, true, isFloatingCandidateLeftPositionedInFloatingState(layoutBox) };
Expand Down Expand Up @@ -357,8 +356,7 @@ std::optional<LayoutUnit> FloatingContext::bottom(Clear type) const
// Cache the value if we end up calling it more frequently (and update it at append/remove).
auto bottom = std::optional<LayoutUnit> { };
for (auto& floatItem : floatingState().floats()) {
if ((type == Clear::Left && !floatItem.isLeftPositioned())
|| (type == Clear::Right && floatItem.isLeftPositioned()))
if ((type == Clear::Left && !floatItem.isLeftPositioned()) || (type == Clear::Right && floatItem.isLeftPositioned()))
continue;
bottom = !bottom ? floatItem.absoluteRectWithMargin().bottom() : std::max(*bottom, floatItem.absoluteRectWithMargin().bottom());
}
Expand Down

0 comments on commit 0c02598

Please sign in to comment.