Bug 67202 – Handle positive leading when paginating.

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

[Dave Hyatt]

Technically we should paginate lines based solely off their line top and line bottom with leading included. However there are two issues with always doing so. The first is that overflow can cause lines to overlap, and the second is that negative leading can cause lines to overlap.

Since we're incapable of dealing with overlap until we stop clipping column boxes and allow them to have a form of specialized overflow, we still have to at least factor in overflow and ignore negative leading for now. However we can at least honor positive leading when the lines don't overlap. This bug is about at least having some decent support for paginating when positive leading is involved.

[Dave Hyatt]

Created attachment 105636 [details]
Patch

Comment on attachment 105636 [details]
Patch

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

> Source/WebCore/rendering/RootInlineBox.cpp:251
> +    LayoutUnit maxHeight = max<LayoutUnit>(0, maxAscent + maxDescent);

This is the only part that’s not clear to me. Why is it okay to clamp this to 0 here?

[Dave Hyatt]

(In reply to comment #2)
> (From update of attachment 105636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105636&action=review
> 
> > Source/WebCore/rendering/RootInlineBox.cpp:251
> > +    LayoutUnit maxHeight = max<LayoutUnit>(0, maxAscent + maxDescent);
> 
> This is the only part that’s not clear to me. Why is it okay to clamp this to 0 here?

I think this line is unnecessary. I can't think of any way in which maxAscent + maxDescent can be < 0, since the smallest line-height you can have is 0. It's probably just paranoia, but I'll preserve the timing of the clamp to 0 and make sure the old (possibly negative?) value got into placeBoxes...

[Dave Hyatt]

Fixed in r94084.

[Balazs Kelemen]

This likely caused a regression on Qt (this is the only non-trivial patch on the blame list):
--- /ramdisk/qt-linux-release/build/layout-test-results/css2.1/t090204-display-change-01-b-ao-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/css2.1/t090204-display-change-01-b-ao-actual.txt 
@@ -1,14 +1,14 @@
 layer at (0,0) size 800x600
   RenderView at (0,0) size 800x600
-layer at (0,0) size 800x96
-  RenderBlock {HTML} at (0,0) size 800x96
-    RenderBody {BODY} at (8,16) size 784x72
+layer at (0,0) size 800x113
+  RenderBlock {HTML} at (0,0) size 800x113
+    RenderBody {BODY} at (8,16) size 784x89
       RenderBlock {P} at (0,0) size 784x22
         RenderText {#text} at (0,0) size 285x22
           text run at (0,0) width 285: "There should be no red below, only green."
       RenderBlock (floating) {DIV} at (0,38) size 784x17 [color=#008000] [bgcolor=#FF0000]
         RenderText {#text} at (0,0) size 16x17
           text run at (0,0) width 16: "X"
-      RenderBlock {DIV} at (0,38) size 16x34 [color=#008000] [bgcolor=#FF0000]
+      RenderBlock {DIV} at (0,38) size 16x51 [color=#008000] [bgcolor=#FF0000]
         RenderText {#text} at (0,17) size 16x17
           text run at (0,17) width 16: "X"

[Balazs Kelemen]

Bug for the Qt failure: #67286