Bug 101803 – REGRESSION(r121789): Text not wrapping in presence of floating objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

Created attachment 173378 [details]
Reduction

After r121789, the text is not wrapping properly on the attached page and thus overflows its containing block.

This seems caused by a combination of floats and a line layout, where the line is offset a bit from the adjacent float.

[Robert Hogan]

Created attachment 173687 [details]
Patch

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/121789

[Eric Seidel (no email)]

Does this have a perf impact?

[EFL EWS Bot]

Comment on attachment 173687 [details]
Patch

Attachment 173687 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14806693

[Dave Hyatt]

Comment on attachment 173687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173687&action=review

r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:142
> -        return logicalHeight;
> +        return block->lineHeight(true, block->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes);

It's a little weird that you ignore line-box-contain values that might have indicated the block's line-height should be ignored. I don't have a really good solution if that is the case, however, since I'm not sure what we want to do in that situation. We can either leave this alone with a FIXME, or we can decide that if the block is excluded we return 0 instead. I'm leaning towards returning 0.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:145
>      if (lineBox->firstChild() && lineBox->firstChild()->renderer() && lineBox->firstChild()->renderer()->isRenderBlock())
> -        logicalHeight = toRenderBlock(lineBox->firstChild()->renderer())->logicalHeight();
> -    else
> -        logicalHeight = lineBox->height();
> -    return logicalHeight;
> +        return toRenderBlock(lineBox->firstChild()->renderer())->logicalHeight();

This just looks all wrong to me. I don't understand any of it. Neither the original code nor the new code look right to me.

[Dave Hyatt]

I think you should just use the block line height and cut out the inline block checking for now.

Also, you need to handle line-box-contain values that exclude block (just use 0 in that case), and I think if you're not in strict mode, you should just use 0 too, since otherwise you'll mess up the quirks mode cases where images have their descent removed.

[Andrei Bucur]

Created attachment 173709 [details]
A test case with a big inline box

[Robert Hogan]

Created attachment 175025 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175025 [details]
Patch

Attachment 175025 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14894360

New failing tests:
css2.1/20110323/floats-wrap-top-below-inline-001r.htm

[Build Bot]

Comment on attachment 175025 [details]
Patch

Attachment 175025 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14907496

New failing tests:
fast/block/float/floats-offset-inline-block-strict-lineheight.html
css2.1/20110323/floats-wrap-top-below-inline-001r.htm
fast/block/float/floats-offset-image-strict-lineheight.html
fast/block/float/floats-offset-image-strict.html

[Robert Hogan]

Created attachment 175163 [details]
Patch

[Build Bot]

Comment on attachment 175163 [details]
Patch

Attachment 175163 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14901536

New failing tests:
fast/block/float/floats-offset-inline-block-strict-lineheight.html
fast/block/float/floats-offset-image-strict-lineheight.html
fast/block/float/floats-offset-image-strict.html

[Robert Hogan]

Created attachment 175597 [details]
Patch

[Dave Hyatt]

Comment on attachment 175597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175597&action=review

r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:140
> +    if (block->document()->inQuirksMode() && replacedHeight)
> +        return replacedHeight;

I think this replaced stuff needs work. I'd prefer we just get the simplest fix in first. For now, let's just change this to:

if (!block->document()->inNoQuirksMode())
    return 0;

Note that the descent clipping for replaced elements happens even in LimitedQuirksMode, so you need to specifically target NoQuirksMode only.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:950
> +        if (!previousObject && r->m_object->isReplaced()) {
> +            RenderBox* renderBox = toRenderBox(r->m_object);
> +            LayoutUnit lineLogicalHeight = logicalHeightForLine(this, firstLine, renderBox->logicalHeight());
> +            logicalLeft = pixelSnappedLogicalLeftOffsetForLine(logicalHeight(), firstLine, lineLogicalHeight);
> +            logicalRight = pixelSnappedLogicalRightOffsetForLine(logicalHeight(), firstLine, lineLogicalHeight);
> +#if ENABLE(CSS_EXCLUSIONS)
> +            if (exclusionShapeInsideInfo && exclusionShapeInsideInfo->hasSegments()) {
> +                logicalLeft = max<float>(roundToInt(exclusionShapeInsideInfo->segments()[0].logicalLeft), logicalLeft);
> +                logicalRight = min<float>(floorToInt(exclusionShapeInsideInfo->segments()[0].logicalRight), logicalRight);
> +            }
> +#endif
> +            availableLogicalWidth = logicalRight - logicalLeft;
> +        }

Just cut this for now.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2500
> +            if (atStart)
> +                width.updateAvailableWidth(replacedBox->logicalHeight());

Just drop this for now. Let's figure out the replaced element handling in a separate patch.

[Robert Hogan]

Created attachment 176044 [details]
Patch

[Build Bot]

Comment on attachment 176044 [details]
Patch

Attachment 176044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14980895

New failing tests:
css1/formatting_model/floating_elements.html
fast/block/float/010.html
fast/block/float/independent-align-positioning.html

[WebKit Review Bot]

Comment on attachment 176044 [details]
Patch

Attachment 176044 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14985841

New failing tests:
css1/formatting_model/floating_elements.html
fast/text/atsui-small-caps-punctuation-size.html

[Robert Hogan]

Created attachment 176809 [details]
Patch

[Dave Hyatt]

Comment on attachment 176809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176809&action=review

Just one question.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:140
> +    if (block->document()->inQuirksMode() && replacedHeight)
> +        return replacedHeight;

Why only in quirks mode? Lines still grow to accommodate replaced elements in all modes. I'm not really understanding this part.

Note the difference between NoQuirksMode and the other two modes is that in NoQuirksMode, baseline-aligned images still have descent. Even if your heuristic isn't good enough to account for this descent, the replacedHeight is still relevant in all modes.

[Robert Hogan]

(In reply to comment #19)
> (From update of attachment 176809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176809&action=review
> 
> Just one question.
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:140
> > +    if (block->document()->inQuirksMode() && replacedHeight)
> > +        return replacedHeight;
> 
> Why only in quirks mode? Lines still grow to accommodate replaced elements in all modes. I'm not really understanding this part.
> 
> Note the difference between NoQuirksMode and the other two modes is that in NoQuirksMode, baseline-aligned images still have descent. Even if your heuristic isn't good enough to account for this descent, the replacedHeight is still relevant in all modes.

That's right - in the other modes I can just take the maximum of the line-height or replacedHeight. It's only in quirks mode that the descent is ignored completely like this - and I've just realized you asked me to change this to !block->document()->inNoQuirksMode()

[Robert Hogan]

Created attachment 178035 [details]
Patch

[Robert Hogan]

Created attachment 178347 [details]
Patch

[Build Bot]

Comment on attachment 178347 [details]
Patch

Attachment 178347 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15189659

[Robert Hogan]

Created attachment 178348 [details]
Patch

[Build Bot]

Comment on attachment 178348 [details]
Patch

Attachment 178348 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15210495

[Robert Hogan]

Created attachment 178370 [details]
Patch

[Dave Hyatt]

Comment on attachment 178370 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 178370 [details]
Patch

Clearing flags on attachment: 178370

Committed r137331: <http://trac.webkit.org/changeset/137331>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This patch broke the layout of http://www.usspotemkin.com

[Andy Estes]

(In reply to comment #30)
> This patch broke the layout of http://www.usspotemkin.com

Filed https://bugs.webkit.org/show_bug.cgi?id=110910 to track this.