[New Multicolumn] REGRESSION: RenderMultiColumnSets broken by the RenderRegion -...
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2013 23:23:39 +0000 (23:23 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2013 23:23:39 +0000 (23:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=110239.

Reviewed by Simon Fraser.

Source/WebCore:

Test: fast/multicol/newmulticol/column-rules-fixed-height.html

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::columnRectAt):
Make sure the columnGap() in the old multicolumn code is always expressed as a LayoutUnit. This was the
one place where it was still an int.

* rendering/RenderFlowThread.cpp:
(WebCore::RenderFlowThread::paintFlowThreadPortionInRegion):
Rework the painting of flow thread portions to account for the fact that regions paint at an integral
translation. This means you have to construct clipping around that integral destination. Subpixel layout
regions did not clip correctly as a result of this issue.

* rendering/RenderMultiColumnSet.cpp:
(WebCore::RenderMultiColumnSet::columnRectAt):
Fix the same bug with columnGap() that the old column code has, i.e., one spot where it was an int.

(WebCore::RenderMultiColumnSet::paintObject):
RenderMultiColumnSet should be using paintObject and not paint and it needs to check for visibility
and phases now that it is a RenderBlock subclass.

(WebCore::RenderMultiColumnSet::paintColumnRules):
Fix the bug that Opera guys fixed in the old multi-column code. They didn't patch the new code, so this
takes care of that.

* rendering/RenderMultiColumnSet.h:
(RenderMultiColumnSet):
Change to use paintObject instead of paint.

LayoutTests:

* fast/multicol/newmulticol: Added.
* fast/multicol/newmulticol/column-rules-fixed-height-expected.html: Added.
* fast/multicol/newmulticol/column-rules-fixed-height.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@143395 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height-expected.html [new file with mode: 0644]
LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderFlowThread.cpp
Source/WebCore/rendering/RenderMultiColumnSet.cpp
Source/WebCore/rendering/RenderMultiColumnSet.h

index 3459cff..7262a70 100644 (file)
@@ -1,3 +1,14 @@
+2013-02-19  David Hyatt  <hyatt@apple.com>
+
+        [New Multicolumn] REGRESSION: RenderMultiColumnSets broken by the RenderRegion -> RenderBlock subclassing.
+        https://bugs.webkit.org/show_bug.cgi?id=110239.
+
+        Reviewed by Simon Fraser.
+
+        * fast/multicol/newmulticol: Added.
+        * fast/multicol/newmulticol/column-rules-fixed-height-expected.html: Added.
+        * fast/multicol/newmulticol/column-rules-fixed-height.html: Added.
+
 2013-02-19  Christian Biesinger  <cbiesinger@chromium.org>
 
         Convert residual-style.html test to a reftest (and fix typos)
diff --git a/LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height-expected.html b/LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height-expected.html
new file mode 100644 (file)
index 0000000..ad21844
--- /dev/null
@@ -0,0 +1,8 @@
+<body style="overflow:hidden">
+<div style="-webkit-columns: 3; -webkit-column-rule: 4px solid maroon; padding: 0 10px; border:5px solid black; height:300px;">
+Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Nulla varius enim ac mi. Curabitur sollicitudin felis quis lectus. Quisque adipiscing rhoncus sem. Proin nulla purus, vulputate vel, varius ut, euismod et, nisi. Sed vitae felis vel orci sagittis aliquam. Cras convallis adipiscing sem. Nam nonummy enim. Nullam bibendum lobortis neque. Vestibulum velit orci, tempus euismod, pretium quis, interdum vitae, nulla. Phasellus eget ante et tortor condimentum vestibulum.
+Suspendisse hendrerit quam nec felis. Sed varius turpis vitae pede. Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Proin bibendum justo ac enim. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos hymenaeos. Sed leo nulla, rutrum eu, dictum ut, posuere vel, arcu. Nam libero. Morbi orci. Maecenas pellentesque. Curabitur erat erat, ullamcorper at, gravida vitae, iaculis vitae, elit. Nullam quam. Quisque orci lectus, ullamcorper eu, imperdiet sed, accumsan et, ligula. Duis diam nisl, sagittis a, blandit volutpat, interdum sed, velit. <a href="#">Vestibulum quam.</a>
+Nulla a purus. Phasellus semper semper lectus. Nulla porttitor, dolor dictum scelerisque luctus, velit ipsum lobortis mauris, ac pretium enim nunc vel risus. Proin gravida mi ut sem cursus mattis. Fusce laoreet, nisi quis luctus volutpat, arcu pede tincidunt enim, nec malesuada urna nisl eu enim. Vivamus varius augue ac purus. Vestibulum vestibulum. Phasellus et est vitae ante accumsan rhoncus. Morbi convallis, arcu at hendrerit gravida, sem diam dignissim risus, sed aliquet erat mi ut mi. Nunc cursus lacinia elit.
+Nunc nisi. Quisque at erat. Vestibulum dictum quam vitae nibh. Nunc vitae ante non odio interdum blandit. Curabitur leo quam, fermentum sed, feugiat in, ullamcorper id, nibh. Suspendisse ac turpis. In iaculis sollicitudin dui. <a href="#">Aenean vitae</a> lectus vitae nulla pellentesque sollicitudin. Nunc gravida pharetra lectus. Etiam lacus ligula, placerat ut, dictum vitae, tempus vel, risus. Cras rhoncus. Praesent varius ultricies orci. Donec mattis, neque ut ornare fringilla, ante urna placerat eros, vel commodo nisi tortor ut mauris. Morbi magna dui, sagittis sit amet, tincidunt et, elementum eget, quam. Fusce molestie nisl vitae nisi.
+Vestibulum a sapien. Phasellus ante lacus, vehicula non, cursus a, tempor ut, magna. Suspendisse potenti. Fusce aliquet, odio viverra vulputate dictum, enim odio luctus purus, ut scelerisque quam nulla non est. Donec eros lacus, egestas vitae, lacinia quis, tempor quis, pede. Morbi orci erat, iaculis id, ornare ac, elementum at, sem. Nunc ornare sodales nisi. Morbi interdum commodo nisl. Fusce eget eros non nisi ornare facilisis. Sed placerat, est non posuere posuere, purus sem dignissim libero, a viverra tellus dolor vel lorem. Cras augue. Etiam ultricies consequat odio. Mauris ac libero. Etiam posuere, libero vitae euismod gravida, urna elit imperdiet magna, vel cursus elit felis non mauris. Donec orci erat, porta id, dignissim ut, posuere dictum, leo. Suspendisse scelerisque egestas nulla.
+</div>
diff --git a/LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height.html b/LayoutTests/fast/multicol/newmulticol/column-rules-fixed-height.html
new file mode 100644 (file)
index 0000000..ce112e8
--- /dev/null
@@ -0,0 +1,11 @@
+<script>
+internals.settings.setRegionBasedColumnsEnabled(true)
+</script>
+<body style="overflow:hidden">
+<div style="-webkit-columns: 3; -webkit-column-rule: 4px solid maroon; padding: 0 10px; border:5px solid black; height:300px;">
+Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Nulla varius enim ac mi. Curabitur sollicitudin felis quis lectus. Quisque adipiscing rhoncus sem. Proin nulla purus, vulputate vel, varius ut, euismod et, nisi. Sed vitae felis vel orci sagittis aliquam. Cras convallis adipiscing sem. Nam nonummy enim. Nullam bibendum lobortis neque. Vestibulum velit orci, tempus euismod, pretium quis, interdum vitae, nulla. Phasellus eget ante et tortor condimentum vestibulum.
+Suspendisse hendrerit quam nec felis. Sed varius turpis vitae pede. Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Proin bibendum justo ac enim. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos hymenaeos. Sed leo nulla, rutrum eu, dictum ut, posuere vel, arcu. Nam libero. Morbi orci. Maecenas pellentesque. Curabitur erat erat, ullamcorper at, gravida vitae, iaculis vitae, elit. Nullam quam. Quisque orci lectus, ullamcorper eu, imperdiet sed, accumsan et, ligula. Duis diam nisl, sagittis a, blandit volutpat, interdum sed, velit. <a href="#">Vestibulum quam.</a>
+Nulla a purus. Phasellus semper semper lectus. Nulla porttitor, dolor dictum scelerisque luctus, velit ipsum lobortis mauris, ac pretium enim nunc vel risus. Proin gravida mi ut sem cursus mattis. Fusce laoreet, nisi quis luctus volutpat, arcu pede tincidunt enim, nec malesuada urna nisl eu enim. Vivamus varius augue ac purus. Vestibulum vestibulum. Phasellus et est vitae ante accumsan rhoncus. Morbi convallis, arcu at hendrerit gravida, sem diam dignissim risus, sed aliquet erat mi ut mi. Nunc cursus lacinia elit.
+Nunc nisi. Quisque at erat. Vestibulum dictum quam vitae nibh. Nunc vitae ante non odio interdum blandit. Curabitur leo quam, fermentum sed, feugiat in, ullamcorper id, nibh. Suspendisse ac turpis. In iaculis sollicitudin dui. <a href="#">Aenean vitae</a> lectus vitae nulla pellentesque sollicitudin. Nunc gravida pharetra lectus. Etiam lacus ligula, placerat ut, dictum vitae, tempus vel, risus. Cras rhoncus. Praesent varius ultricies orci. Donec mattis, neque ut ornare fringilla, ante urna placerat eros, vel commodo nisi tortor ut mauris. Morbi magna dui, sagittis sit amet, tincidunt et, elementum eget, quam. Fusce molestie nisl vitae nisi.
+Vestibulum a sapien. Phasellus ante lacus, vehicula non, cursus a, tempor ut, magna. Suspendisse potenti. Fusce aliquet, odio viverra vulputate dictum, enim odio luctus purus, ut scelerisque quam nulla non est. Donec eros lacus, egestas vitae, lacinia quis, tempor quis, pede. Morbi orci erat, iaculis id, ornare ac, elementum at, sem. Nunc ornare sodales nisi. Morbi interdum commodo nisl. Fusce eget eros non nisi ornare facilisis. Sed placerat, est non posuere posuere, purus sem dignissim libero, a viverra tellus dolor vel lorem. Cras augue. Etiam ultricies consequat odio. Mauris ac libero. Etiam posuere, libero vitae euismod gravida, urna elit imperdiet magna, vel cursus elit felis non mauris. Donec orci erat, porta id, dignissim ut, posuere dictum, leo. Suspendisse scelerisque egestas nulla.
+</div>
index 09a61c3..3b9bb90 100644 (file)
@@ -1,3 +1,39 @@
+2013-02-19  David Hyatt  <hyatt@apple.com>
+
+        [New Multicolumn] REGRESSION: RenderMultiColumnSets broken by the RenderRegion -> RenderBlock subclassing.
+        https://bugs.webkit.org/show_bug.cgi?id=110239.
+
+        Reviewed by Simon Fraser.
+
+        Test: fast/multicol/newmulticol/column-rules-fixed-height.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::columnRectAt):
+        Make sure the columnGap() in the old multicolumn code is always expressed as a LayoutUnit. This was the
+        one place where it was still an int.
+
+        * rendering/RenderFlowThread.cpp:
+        (WebCore::RenderFlowThread::paintFlowThreadPortionInRegion):
+        Rework the painting of flow thread portions to account for the fact that regions paint at an integral
+        translation. This means you have to construct clipping around that integral destination. Subpixel layout
+        regions did not clip correctly as a result of this issue.
+
+        * rendering/RenderMultiColumnSet.cpp:
+        (WebCore::RenderMultiColumnSet::columnRectAt):
+        Fix the same bug with columnGap() that the old column code has, i.e., one spot where it was an int.
+
+        (WebCore::RenderMultiColumnSet::paintObject):
+        RenderMultiColumnSet should be using paintObject and not paint and it needs to check for visibility
+        and phases now that it is a RenderBlock subclass.
+
+        (WebCore::RenderMultiColumnSet::paintColumnRules):
+        Fix the bug that Opera guys fixed in the old multi-column code. They didn't patch the new code, so this
+        takes care of that.
+
+        * rendering/RenderMultiColumnSet.h:
+        (RenderMultiColumnSet):
+        Change to use paintObject instead of paint.
+
 2013-02-19  Branimir Lambov  <blambov@google.com>
 
         Fix 'slice' aspect ratio calculation
index 5320ff6..3cdeef7 100644 (file)
@@ -5423,7 +5423,7 @@ LayoutRect RenderBlock::columnRectAt(ColumnInfo* colInfo, unsigned index) const
     LayoutUnit colLogicalHeight = colInfo->columnHeight();
     LayoutUnit colLogicalTop = borderBefore() + paddingBefore();
     LayoutUnit colLogicalLeft = logicalLeftOffsetForContent();
-    int colGap = columnGap();
+    LayoutUnit colGap = columnGap();
     if (colInfo->progressionAxis() == ColumnInfo::InlineAxis) {
         if (style()->isLeftToRightDirection() ^ colInfo->progressionIsReversed())
             colLogicalLeft += index * (colLogicalWidth + colGap);
index 612590d..5a2e71f 100644 (file)
@@ -255,32 +255,39 @@ void RenderFlowThread::paintFlowThreadPortionInRegion(PaintInfo& paintInfo, Rend
     if (!context)
         return;
 
-    // Adjust the clipping rect for the region.
-    // paintOffset contains the offset where the painting should occur
-    // adjusted with the region padding and border.
-    LayoutRect regionClippingRect = computeRegionClippingRect(paintOffset, flowThreadPortionRect, flowThreadPortionOverflowRect);
+    // RenderFlowThread should start painting its content in a position that is offset
+    // from the region rect's current position. The amount of offset is equal to the location of
+    // the flow thread portion in the flow thread's local coordinates.
+    // Note that we have to pixel snap the location at which we're going to paint, since this is necessary
+    // to minimize the amount of incorrect snapping that would otherwise occur.
+    // If we tried to paint by applying a non-integral translation, then all the
+    // layout code that attempted to pixel snap would be incorrect.
+    IntPoint adjustedPaintOffset;
+    LayoutPoint portionLocation;
+    if (style()->isFlippedBlocksWritingMode()) {
+        LayoutRect flippedFlowThreadPortionRect(flowThreadPortionRect);
+        flipForWritingMode(flippedFlowThreadPortionRect);
+        portionLocation = flippedFlowThreadPortionRect.location();
+    } else
+        portionLocation = flowThreadPortionRect.location();
+    adjustedPaintOffset = roundedIntPoint(paintOffset - portionLocation);
+
+    // The clipping rect for the region is set up by assuming the flowThreadPortionRect is going to paint offset from adjustedPaintOffset.
+    // Remember that we pixel snapped and moved the paintOffset and stored the snapped result in adjustedPaintOffset. Now we add back in
+    // the flowThreadPortionRect's location to get the spot where we expect the portion to actually paint. This can be non-integral and
+    // that's ok. We then pixel snap the resulting clipping rect to account for snapping that will occur when the flow thread paints.
+    IntRect regionClippingRect = pixelSnappedIntRect(computeRegionClippingRect(adjustedPaintOffset + portionLocation, flowThreadPortionRect, flowThreadPortionOverflowRect));
 
     PaintInfo info(paintInfo);
-    info.rect.intersect(pixelSnappedIntRect(regionClippingRect));
+    info.rect.intersect(regionClippingRect);
 
     if (!info.rect.isEmpty()) {
         context->save();
 
         context->clip(regionClippingRect);
 
-        // RenderFlowThread should start painting its content in a position that is offset
-        // from the region rect's current position. The amount of offset is equal to the location of
-        // the flow thread portion in the flow thread's local coordinates.
-        IntPoint renderFlowThreadOffset;
-        if (style()->isFlippedBlocksWritingMode()) {
-            LayoutRect flippedFlowThreadPortionRect(flowThreadPortionRect);
-            flipForWritingMode(flippedFlowThreadPortionRect);
-            renderFlowThreadOffset = roundedIntPoint(paintOffset - flippedFlowThreadPortionRect.location());
-        } else
-            renderFlowThreadOffset = roundedIntPoint(paintOffset - flowThreadPortionRect.location());
-
-        context->translate(renderFlowThreadOffset.x(), renderFlowThreadOffset.y());
-        info.rect.moveBy(-renderFlowThreadOffset);
+        context->translate(adjustedPaintOffset.x(), adjustedPaintOffset.y());
+        info.rect.moveBy(-adjustedPaintOffset);
         
         layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects);
 
index 8aaaf62..558b6de 100644 (file)
@@ -119,7 +119,7 @@ LayoutRect RenderMultiColumnSet::columnRectAt(unsigned index) const
     LayoutUnit colLogicalHeight = computedColumnHeight();
     LayoutUnit colLogicalTop = borderBefore() + paddingBefore();
     LayoutUnit colLogicalLeft = borderAndPaddingLogicalLeft();
-    int colGap = columnGap();
+    LayoutUnit colGap = columnGap();
     if (style()->isLeftToRightDirection())
         colLogicalLeft += index * (colLogicalWidth + colGap);
     else
@@ -205,14 +205,21 @@ LayoutRect RenderMultiColumnSet::flowThreadPortionOverflowRect(const LayoutRect&
     return overflowRectForFlowThreadPortion(overflowRect, isFirstRegion() && isFirstColumn, isLastRegion() && isLastColumn);
 }
 
-void RenderMultiColumnSet::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
+void RenderMultiColumnSet::paintObject(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
-    // FIXME: RenderRegions are replaced elements right now and so they only paint in the foreground phase.
+    if (style()->visibility() != VISIBLE)
+        return;
+
+    RenderBlock::paintObject(paintInfo, paintOffset);
+
+    // FIXME: Right now we're only painting in the foreground phase.
     // Columns should technically respect phases and allow for background/float/foreground overlap etc., just like
-    // RenderBlocks do. We can't correct this, however, until RenderRegions are changed to actually be
-    // RenderBlocks. Note this is a pretty minor issue, since the old column implementation clipped columns
+    // RenderBlocks do. Note this is a pretty minor issue, since the old column implementation clipped columns
     // anyway, thus making it impossible for them to overlap one another. It's also really unlikely that the columns
     // would overlap another block.
+    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseSelection))
+        return;
+
     setRegionObjectsRegionStyle();
     paintColumnRules(paintInfo, paintOffset);
     paintColumnContents(paintInfo, paintOffset);
@@ -230,7 +237,7 @@ void RenderMultiColumnSet::paintColumnRules(PaintInfo& paintInfo, const LayoutPo
     EBorderStyle ruleStyle = blockStyle->columnRuleStyle();
     LayoutUnit ruleThickness = blockStyle->columnRuleWidth();
     LayoutUnit colGap = columnGap();
-    bool renderRule = ruleStyle > BHIDDEN && !ruleTransparent && ruleThickness <= colGap;
+    bool renderRule = ruleStyle > BHIDDEN && !ruleTransparent;
     if (!renderRule)
         return;
 
index bd980b5..e841ef9 100644 (file)
@@ -94,7 +94,7 @@ private:
     virtual void updateLogicalHeight() OVERRIDE;
     virtual void computeLogicalHeight(LayoutUnit logicalHeight, LayoutUnit logicalTop, LogicalExtentComputedValues&) const OVERRIDE;
 
-    virtual void paint(PaintInfo&, const LayoutPoint& paintOffset) OVERRIDE;
+    virtual void paintObject(PaintInfo&, const LayoutPoint& paintOffset) OVERRIDE;
     virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation&, const LayoutPoint& accumulatedOffset, HitTestAction) OVERRIDE;
 
     virtual LayoutUnit pageLogicalWidth() const OVERRIDE { return m_computedColumnWidth; }