Skip to content

Commit

Permalink
Cherry-pick 264579@main (9d3c89b). <rdar://108742128>
Browse files Browse the repository at this point in the history
    [IFC][Float] Incorrectly placed float boxes may generate series of empty lines
    https://bugs.webkit.org/show_bug.cgi?id=257261
    <rdar://108742128>

    Reviewed by Antti Koivisto.

    When intrusive floats prevent us from placing any content at the current vertical position,
    the candidate position for the next line is computed by looking at such intrusive floats.
    e.g.

      Two float boxes with the inline content of "foobar".

      _______    _______
     |       |  |       |
     | left  |  | right |
     |_______|  |       |
                |_______|

     1. "foobar" does not fit at y: 0 (overlaps "left").
     2. we find 2 intrusive floats at y: 0
     3. vertical position for next line is at the bottom of "left1"

    This is rather simple, but if float placement bugs produce some vertical gaps between floats
    e.g.

      _______    _______
     |       |  |       |
     | left1 |  | right |
     |_______|  |       |
                |_______|

      _______
     |       |
     | left2 |
     |_______|

     (note that left2 is supposed to be vertically adjacent to left1)

     Now if we run line layout:

     1. "foobar" does not fit at y: 0 -> vertical position for next line is at the bottom of "left1"
     2. "foobar" still does not fit (assume it overlaps "right") -> position for next line is at the bottom of "right"
     3. now assume that "foobar" is tall and it does not fit between the bottom of the "right" and the top of this incorrectly placed "left2"

      _______    _______
     |       |  |       |
     | left1 |  | right |
     |_______|  |       |
                |_______|
        ____
       |
       |___
      _|_____
     | |     |
     | |     |
     |_______|

     running "let's find the position for next line by avoiding intrusive floats" logic in InlineFormattingGeometry::logicalTopForNextLine()
     finds no intrusive float at the bottom of the "right" float (that's what #2 computed as candidate position).
     This is an unexpected state (we assert) and in order not to get stuck on the same vertical position we advance by 1px for the next line hoping we would be able to place "foobar" there.
     While it helps to avoid forever looping, if the gap between the bottom of the "right" and the top of the "left2" is
     wide we may end up producing thousands of empty lines until we reach the top of the "left2" float box and finally get out of this unexpected state.

    In this patch, instead of advancing by 1px, we jump right to the bottom of the "left2" float box (bottom of all the floats in this floating context) and continue from there.

    * Source/WebCore/layout/formattingContexts/inline/InlineFormattingGeometry.cpp:
    (WebCore::Layout::InlineFormattingGeometry::logicalTopForNextLine const): move float handling to a helper (intrusiveFloatBottom) and return
    the max of floatingContext.bottom() and lineLogicalRect.bottom().

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

Canonical link: https://commits.webkit.org/259548.800@safari-7615-branch
Identifier: 259548.798@safari-7615.3.6.11-branch
  • Loading branch information
alanbaradlay authored and MyahCobbs committed Jun 5, 2023
1 parent fabc82e commit 6c9af88
Showing 1 changed file with 27 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ InlineFormattingGeometry::InlineFormattingGeometry(const InlineFormattingContext

InlineLayoutUnit InlineFormattingGeometry::logicalTopForNextLine(const LineBuilder::LineContent& lineContent, const InlineRect& lineLogicalRect, const FloatingContext& floatingContext) const
{
if (!lineContent.inlineItemRange.isEmpty()) {
auto didManageToPlaceInlineContentOrFloat = !lineContent.inlineItemRange.isEmpty();
if (didManageToPlaceInlineContentOrFloat) {
// Normally the next line's logical top is the previous line's logical bottom, but when the line ends
// with the clear property set, the next line needs to clear the existing floats.
if (lineContent.runs.isEmpty())
Expand All @@ -59,25 +60,32 @@ InlineLayoutUnit InlineFormattingGeometry::logicalTopForNextLine(const LineBuild
return lineLogicalRect.bottom();
return std::max(lineLogicalRect.bottom(), InlineLayoutUnit(positionWithClearance->position));
}
// Floats must have prevented us placing any content on the line.
// Move the next line below the intrusive float(s).
ASSERT(lineContent.runs.isEmpty() || lineContent.runs[0].isLineSpanningInlineBoxStart());
ASSERT(lineContent.hasIntrusiveFloat);
// FIXME: Moving the bottom position by the initial line height may be too much meaning that we miss vertical positions where atomic inline content could very well fit.
// The spec is unclear of how much we should move down at this point and while 1px should be the most precise it's also rather expensive.
auto inflatedLineRectBottom = lineLogicalRect.top() + formattingContext().root().style().computedLineHeight();
auto floatConstraints = floatingContext.constraints(toLayoutUnit(lineLogicalRect.top()), toLayoutUnit(inflatedLineRectBottom), FloatingContext::MayBeAboveLastFloat::Yes);
if (floatConstraints.left && floatConstraints.right) {
// In case of left and right constraints, we need to pick the one that's closer to the current line.
return std::min(floatConstraints.left->y, floatConstraints.right->y);
}
if (floatConstraints.left)
return floatConstraints.left->y;
if (floatConstraints.right)
return floatConstraints.right->y;
ASSERT_NOT_REACHED();

auto intrusiveFloatBottom = [&]() -> InlineLayoutUnit {
// Floats must have prevented us placing any content on the line.
// Move the next line below the intrusive float(s).
ASSERT(lineContent.runs.isEmpty() || lineContent.runs[0].isLineSpanningInlineBoxStart());
ASSERT(lineContent.hasIntrusiveFloat);
// FIXME: Moving the bottom position by the initial line height may be too much meaning that we miss vertical positions where atomic inline content could very well fit.
// The spec is unclear of how much we should move down at this point and while 1px should be the most precise it's also rather expensive.
auto inflatedLineRectBottom = lineLogicalRect.top() + formattingContext().root().style().computedLineHeight();
auto floatConstraints = floatingContext.constraints(toLayoutUnit(lineLogicalRect.top()), toLayoutUnit(inflatedLineRectBottom), FloatingContext::MayBeAboveLastFloat::Yes);
if (floatConstraints.left && floatConstraints.right) {
// In case of left and right constraints, we need to pick the one that's closer to the current line.
return std::min(floatConstraints.left->y, floatConstraints.right->y);
}
if (floatConstraints.left)
return floatConstraints.left->y;
if (floatConstraints.right)
return floatConstraints.right->y;
// If we didn't manage to place a content on this vertical position due to intrusive floats, we have to have
// at least one float here. However due to bugs in float positioning, we may end up with gaps between floats in vertical direction.
// In such cases let's just go with the bottom most float we have here.
ASSERT_NOT_REACHED();
return std::max<InlineLayoutUnit>(lineLogicalRect.bottom(), floatingContext.bottom().value_or(0_lu));
};
// Do not get stuck on the same vertical position.
return nextafter(lineLogicalRect.bottom(), std::numeric_limits<float>::max());
return std::max(intrusiveFloatBottom(), nextafter(lineLogicalRect.bottom(), std::numeric_limits<float>::max()));
}

ContentWidthAndMargin InlineFormattingGeometry::inlineBlockContentWidthAndMargin(const Box& formattingContextRoot, const HorizontalConstraints& horizontalConstraints, const OverriddenHorizontalValues& overriddenHorizontalValues) const
Expand Down

0 comments on commit 6c9af88

Please sign in to comment.