Changeset 122489 in webkit

Timestamp:

Jul 12, 2012 11:46:53 AM (12 years ago)

Author:

hyatt@apple.com

Message:

​https://bugs.webkit.org/show_bug.cgi?id=91000
REGRESSION (r122244): Overflow elements don't shrink as much as they should.

Reviewed by Simon Fraser.

This is a fix for a a regression from ​https://bugs.webkit.org/show_bug.cgi?id=90646.

I incorrectly analyzed the issue with Robert Hogan's negative margin patch and fooled myself into putting back
in an incorrect minimum width check from long ago.

What should have happened in the test case I patched is that the overflow element should shrink to 0. The issue
with improving the logical top estimate in the previous patch is it made the clear delta become 0. This in turn
exposed a bug in our clearing algorithm with Robert's changes where you could need a relayout even if you didn't
actually move. This issue only occurs because the floats list is getting changed mid-layout because of negative margins.

The patch changes getClearDelta to call setChildNeedsLayout(true) on children whose widths change even when their
positions do not. In effect this dynamic addition of new floats after you have done a layout on the child already means
that you can need to lay out again despite not actually having to move.

To handle this, the code that does the relayout is now called if the child needs a relayout. This is done even if
the logical top estimate matches the final position.

No new tests required, since the test in fast/block/float is now correctly covering the issue.

• rendering/RenderBlock.cpp:

(WebCore::RenderBlock::layoutBlockChild):
(WebCore::RenderBlock::getClearDelta):

• rendering/RenderBox.cpp:

(WebCore::RenderBox::shrinkLogicalWidthToAvoidFloats):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderBlock.cpp (modified) (4 diffs)
• rendering/RenderBox.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-07-11  David Hyatt  <hyatt@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=91000
+        REGRESSION (r122244): Overflow elements don't shrink as much as they should.
+
+        Reviewed by Simon Fraser.
+
+        This is a fix for a a regression from https://bugs.webkit.org/show_bug.cgi?id=90646.
+
+        I incorrectly analyzed the issue with Robert Hogan's negative margin patch and fooled myself into putting back
+        in an incorrect minimum width check from long ago.
+        
+        What should have happened in the test case I patched is that the overflow element should shrink to 0. The issue 
+        with improving the logical top estimate in the previous patch is it made the clear delta become 0. This in turn
+        exposed a bug in our clearing algorithm with Robert's changes where you could need a relayout even if you didn't
+        actually move. This issue only occurs because the floats list is getting changed mid-layout because of negative margins.
+
+        The patch changes getClearDelta to call setChildNeedsLayout(true) on children whose widths change even when their
+        positions do not. In effect this dynamic addition of new floats after you have done a layout on the child already means
+        that you can need to lay out again despite not actually having to move.
+        
+        To handle this, the code that does the relayout is now called if the child needs a relayout. This is done even if
+        the logical top estimate matches the final position.
+        
+        No new tests required, since the test in fast/block/float is now correctly covering the issue.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::layoutBlockChild):
+        (WebCore::RenderBlock::getClearDelta):
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::shrinkLogicalWidthToAvoidFloats):
+
 2012-07-12  James Weatherall  <wez@chromium.org>
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -2428 +2428 @@
 
     // Now we have a final top position.  See if it really does end up being different from our estimate.
-    if (logicalTopAfterClear != logicalTopEstimate) {
+    // clearFloatsIfNeeded can also mark the child as needing a layout even though we didn't move. This happens
+    // when collapseMargins dynamically adds overhanging floats because of a child with negative margins.
+    if (logicalTopAfterClear != logicalTopEstimate || child->needsLayout()) {
         if (child->shrinkToAvoidFloats()) {
             // The child's width depends on the line width.
@@ -4584 +4586 @@
                 return newLogicalTop - logicalTop;
 
+            RenderRegion* region = regionAtBlockOffset(logicalTopForChild(child));
+            LayoutRect borderBox = child->borderBoxRectInRegion(region, offsetFromLogicalTopOfFirstPage() + logicalTopForChild(child), DoNotCacheRenderBoxRegionInfo);
+            LayoutUnit childLogicalWidthAtOldLogicalTopOffset = isHorizontalWritingMode() ? borderBox.width() : borderBox.height();
+
             // FIXME: None of this is right for perpendicular writing-mode children.
             LayoutUnit childOldLogicalWidth = child->logicalWidth();
@@ -4592 +4598 @@
             child->setLogicalTop(newLogicalTop);
             child->computeLogicalWidth();
-            RenderRegion* region = regionAtBlockOffset(logicalTopForChild(child));
-            LayoutRect borderBox = child->borderBoxRectInRegion(region, offsetFromLogicalTopOfFirstPage() + logicalTopForChild(child), DoNotCacheRenderBoxRegionInfo);
+            region = regionAtBlockOffset(logicalTopForChild(child));
+            borderBox = child->borderBoxRectInRegion(region, offsetFromLogicalTopOfFirstPage() + logicalTopForChild(child), DoNotCacheRenderBoxRegionInfo);
             LayoutUnit childLogicalWidthAtNewLogicalTopOffset = isHorizontalWritingMode() ? borderBox.width() : borderBox.height();
 
@@ -4601 +4607 @@
             child->setMarginRight(childOldMarginRight);
             
-            if (childLogicalWidthAtNewLogicalTopOffset <= availableLogicalWidthAtNewLogicalTopOffset)
+            if (childLogicalWidthAtNewLogicalTopOffset <= availableLogicalWidthAtNewLogicalTopOffset) {
+                // Even though we may not be moving, if the logical width did shrink because of the presence of new floats, then
+                // we need to force a relayout as though we shifted. This happens because of the dynamic addition of overhanging floats
+                // from previous siblings when negative margins exist on a child (see the addOverhangingFloats call at the end of collapseMargins).
+                if (childLogicalWidthAtOldLogicalTopOffset != childLogicalWidthAtNewLogicalTopOffset)
+                    child->setChildNeedsLayout(true, MarkOnlyThis);
                 return newLogicalTop - logicalTop;
+            }
 
             newLogicalTop = nextFloatLogicalBottomBelow(newLogicalTop);

trunk/Source/WebCore/rendering/RenderBox.cpp

@@ -1179 +1179 @@
 
     LayoutUnit result = cb->availableLogicalWidthForLine(logicalTopPosition, false, containingBlockRegion, adjustedPageOffsetForContainingBlock) - childMarginStart - childMarginEnd;
-    result = max(result, minPreferredLogicalWidth()); // Don't shrink below our minimum preferred logical width.
 
     // We need to see if margins on either the start side or the end side can contain the floats in question. If they can,