[New Multicolumn] Pagination mode messed up with non-inline axis and reversed direction.
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Apr 2014 18:07:15 +0000 (18:07 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Apr 2014 18:07:15 +0000 (18:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=131811

Reviewed by Dean Jackson.

Source/WebCore:
Added fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html

With block axis pagination mode, it is possible to set a column height that is not the same
as the available fill height for a block. The new multi-column code had the assumption that
the column height was the same as the amount of fill room you had available. This is not
the case.

To correct the issue, I added a member variable to RenderMultiColumnSet that stores the
available column height as a separate variable from the computed column height. This allows
the pagination API to specify a different column height that is not the same as the view's
content height.

Even though it isn't involved in the solution, I also patched pageOrViewLogicalHeight on
RenderView to work with the new column code as well.

To address the layout test failures (that caused the previous rollout), I made sure to
initialize m_availableHeight to 0 when m_computedColumnHeight also gets reset to 0.

The assertion is not something I could reproduce on any machine, but I can see the problem.
I patched Page's pageCount method to not have column code directly in Page.cpp,
and to make a new pageCount() method on RenderView that Page calls
into. This method is now patched to handle the new column code as well as the old. I have
no real way of testing this method though, since I can't reproduce the assertion that the
bots were experiencing.

* page/Page.cpp:
(WebCore::Page::pageCount):
* rendering/RenderMultiColumnSet.cpp:
(WebCore::RenderMultiColumnSet::RenderMultiColumnSet):
(WebCore::RenderMultiColumnSet::setAndConstrainColumnHeight):
(WebCore::RenderMultiColumnSet::prepareForLayout):
(WebCore::RenderMultiColumnSet::computeLogicalHeight):
* rendering/RenderMultiColumnSet.h:
* rendering/RenderView.cpp:
(WebCore::RenderView::pageOrViewLogicalHeight):
(WebCore::RenderView::pageCount):
* rendering/RenderView.h:

LayoutTests:
* fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html: Added.
* fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html [new file with mode: 0644]
LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/Page.cpp
Source/WebCore/rendering/RenderMultiColumnSet.cpp
Source/WebCore/rendering/RenderMultiColumnSet.h
Source/WebCore/rendering/RenderView.cpp
Source/WebCore/rendering/RenderView.h

index 88e1f99..c0f73fe 100644 (file)
@@ -1,3 +1,13 @@
+2014-04-21  David Hyatt  <hyatt@apple.com>
+
+        [New Multicolumn] Pagination mode messed up with non-inline axis and reversed direction.
+        https://bugs.webkit.org/show_bug.cgi?id=131811
+
+        Reviewed by Dean Jackson.
+
+        * fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html: Added.
+        * fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html: Added.
+
 2014-04-21  Alexey Proskuryakov  <ap@apple.com>
 
         REGRESSION (r167530): ASSERT(m_selfTime <= m_totalTime) on tests that run after certain Inspector tests
diff --git a/LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html b/LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html
new file mode 100644 (file)
index 0000000..2df0e8b
--- /dev/null
@@ -0,0 +1,44 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        body {
+            -webkit-writing-mode: horizontal-tb;
+        }
+        .box {
+            height: 50px;
+            width: 50px;
+            background-color: blue;
+        }
+        .box.changed {
+            background-color: green;
+        }
+    </style>
+    <script>
+        if (window.internals)
+            internals.setPagination("BottomToTopPaginated", 20, 180);
+    </script>
+</head>
+<body>
+
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>1 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>2 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>3 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>4 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>5 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>6 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>7 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>8 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+
+
+</body>
+</html>
diff --git a/LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html b/LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html
new file mode 100644 (file)
index 0000000..08e3ac4
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        body {
+            -webkit-writing-mode: horizontal-tb;
+        }
+        .box {
+            height: 50px;
+            width: 50px;
+            background-color: blue;
+        }
+        .box.changed {
+            background-color: green;
+        }
+    </style>
+    <script>
+        if (window.internals) {
+            internals.settings.setRegionBasedColumnsEnabled(true);
+            internals.setPagination("BottomToTopPaginated", 20, 180);
+        }
+    </script>
+</head>
+<body>
+
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>1 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>2 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>3 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>4 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>5 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>6 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>7 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+<div class="box" onclick="this.classList.toggle('changed')"></div>
+<p>8 Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. </p>
+
+
+</body>
+</html>
index 207e3f9..e1ed68c 100644 (file)
@@ -1,3 +1,48 @@
+2014-04-21  David Hyatt  <hyatt@apple.com>
+
+        [New Multicolumn] Pagination mode messed up with non-inline axis and reversed direction.
+        https://bugs.webkit.org/show_bug.cgi?id=131811
+
+        Reviewed by Dean Jackson.
+
+        Added fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html
+        
+        With block axis pagination mode, it is possible to set a column height that is not the same
+        as the available fill height for a block. The new multi-column code had the assumption that
+        the column height was the same as the amount of fill room you had available. This is not
+        the case.
+        
+        To correct the issue, I added a member variable to RenderMultiColumnSet that stores the
+        available column height as a separate variable from the computed column height. This allows
+        the pagination API to specify a different column height that is not the same as the view's
+        content height.
+
+        Even though it isn't involved in the solution, I also patched pageOrViewLogicalHeight on
+        RenderView to work with the new column code as well.
+
+        To address the layout test failures (that caused the previous rollout), I made sure to
+        initialize m_availableHeight to 0 when m_computedColumnHeight also gets reset to 0.
+        
+        The assertion is not something I could reproduce on any machine, but I can see the problem.
+        I patched Page's pageCount method to not have column code directly in Page.cpp, 
+        and to make a new pageCount() method on RenderView that Page calls
+        into. This method is now patched to handle the new column code as well as the old. I have
+        no real way of testing this method though, since I can't reproduce the assertion that the
+        bots were experiencing.
+
+        * page/Page.cpp:
+        (WebCore::Page::pageCount):
+        * rendering/RenderMultiColumnSet.cpp:
+        (WebCore::RenderMultiColumnSet::RenderMultiColumnSet):
+        (WebCore::RenderMultiColumnSet::setAndConstrainColumnHeight):
+        (WebCore::RenderMultiColumnSet::prepareForLayout):
+        (WebCore::RenderMultiColumnSet::computeLogicalHeight):
+        * rendering/RenderMultiColumnSet.h:
+        * rendering/RenderView.cpp:
+        (WebCore::RenderView::pageOrViewLogicalHeight):
+        (WebCore::RenderView::pageCount):
+        * rendering/RenderView.h:
+
 2014-04-18  Dean Jackson  <dino@apple.com>
 
         [Media] Clean up localized strings in controls
index 7c859d6..33f0478 100644 (file)
@@ -858,7 +858,7 @@ unsigned Page::pageCount() const
         document->updateLayoutIgnorePendingStylesheets();
 
     RenderView* contentRenderer = mainFrame().contentRenderer();
-    return contentRenderer ? contentRenderer->columnCount(contentRenderer->columnInfo()) : 0;
+    return contentRenderer ? contentRenderer->pageCount() : 0;
 }
 
 void Page::setIsInWindow(bool isInWindow)
index 46cb64b..eee737c 100644 (file)
 #include "config.h"
 #include "RenderMultiColumnSet.h"
 
+#include "FrameView.h"
 #include "PaintInfo.h"
 #include "RenderLayer.h"
 #include "RenderMultiColumnFlowThread.h"
 #include "RenderMultiColumnSpannerPlaceholder.h"
+#include "RenderView.h"
 
 namespace WebCore {
 
@@ -38,6 +40,7 @@ RenderMultiColumnSet::RenderMultiColumnSet(RenderFlowThread& flowThread, PassRef
     , m_computedColumnCount(1)
     , m_computedColumnWidth(0)
     , m_computedColumnHeight(0)
+    , m_availableColumnHeight(0)
     , m_maxColumnHeight(RenderFlowThread::maxLogicalHeight())
     , m_minSpaceShortage(RenderFlowThread::maxLogicalHeight())
     , m_minimumColumnHeight(0)
@@ -151,6 +154,19 @@ void RenderMultiColumnSet::setAndConstrainColumnHeight(LayoutUnit newHeight)
     m_computedColumnHeight = newHeight;
     if (m_computedColumnHeight > m_maxColumnHeight)
         m_computedColumnHeight = m_maxColumnHeight;
+    
+    // FIXME: The available column height is not the same as the constrained height specified
+    // by the pagination API. The column set in this case is allowed to be bigger than the
+    // height of a single column. We cache available column height in order to use it
+    // in computeLogicalHeight later. This is pretty gross, and maybe there's a better way
+    // to formalize the idea of clamped column heights without having a view dependency
+    // here.
+    m_availableColumnHeight = m_computedColumnHeight;
+    if (multiColumnFlowThread() && !multiColumnFlowThread()->progressionIsInline() && parent()->isRenderView()) {
+        int pageLength = view().frameView().pagination().pageLength;
+        if (pageLength)
+            m_computedColumnHeight = pageLength;
+    }
     // FIXME: the height may also be affected by the enclosing pagination context, if any.
 }
 
@@ -327,8 +343,10 @@ void RenderMultiColumnSet::prepareForLayout(bool initial)
     if (initial)
         m_maxColumnHeight = calculateMaxColumnHeight();
     if (requiresBalancing()) {
-        if (initial)
+        if (initial) {
             m_computedColumnHeight = 0;
+            m_availableColumnHeight = 0;
+        }
     } else
         setAndConstrainColumnHeight(heightAdjustedForSetOffset(multiColumnFlowThread()->columnHeightAvailable()));
 
@@ -396,7 +414,7 @@ void RenderMultiColumnSet::layout()
 
 void RenderMultiColumnSet::computeLogicalHeight(LayoutUnit, LayoutUnit logicalTop, LogicalExtentComputedValues& computedValues) const
 {
-    computedValues.m_extent = m_computedColumnHeight;
+    computedValues.m_extent = m_availableColumnHeight;
     computedValues.m_position = logicalTop;
 }
 
index 331ea6c..956761c 100644 (file)
@@ -178,6 +178,7 @@ private:
     unsigned m_computedColumnCount; // Used column count (the resulting 'N' from the pseudo-algorithm in the multicol spec)
     LayoutUnit m_computedColumnWidth; // Used column width (the resulting 'W' from the pseudo-algorithm in the multicol spec)
     LayoutUnit m_computedColumnHeight;
+    LayoutUnit m_availableColumnHeight;
     
     // The following variables are used when balancing the column set.
     LayoutUnit m_maxColumnHeight; // Maximum column height allowed.
index cf22d58..2ea86a1 100644 (file)
@@ -295,7 +295,7 @@ LayoutUnit RenderView::pageOrViewLogicalHeight() const
     if (document().printing())
         return pageLogicalHeight();
     
-    if (hasColumns() && !style().hasInlineColumnAxis()) {
+    if ((hasColumns() || multiColumnFlowThread()) && !style().hasInlineColumnAxis()) {
         if (int pageLength = frameView().pagination().pageLength)
             return pageLength;
     }
@@ -1257,4 +1257,18 @@ unsigned RenderView::pageNumberForBlockProgressionOffset(int offset) const
     return columnNumber;
 }
 
+unsigned RenderView::pageCount() const
+{
+    const Pagination& pagination = frameView().frame().page()->pagination();
+    if (pagination.mode == Pagination::Unpaginated)
+        return 0;
+    
+    if (hasColumns())
+        return columnCount(columnInfo());
+    if (multiColumnFlowThread())
+        return multiColumnFlowThread()->columnCount();
+
+    return 0;
+}
+
 } // namespace WebCore
index a0ffbdb..4ba7f19 100644 (file)
@@ -160,6 +160,8 @@ public:
     // function and should not be mistaken for a general page number API.
     unsigned pageNumberForBlockProgressionOffset(int offset) const;
 
+    unsigned pageCount() const;
+
     // FIXME: These functions are deprecated. No code should be added that uses these.
     int bestTruncatedAt() const { return m_legacyPrinting.m_bestTruncatedAt; }
     void setBestTruncatedAt(int y, RenderBoxModelObject* forRenderer, bool forcedBreak = false);