Changeset 167597 in webkit

Timestamp:

Apr 21, 2014 11:07:15 AM (10 years ago)

Author:

hyatt@apple.com

Message:

[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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html (added)
• LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/Page.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderMultiColumnSet.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderMultiColumnSet.h (modified) (1 diff)
• Source/WebCore/rendering/RenderView.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderView.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/Page.cpp

@@ -859 +859 @@
 
     RenderView* contentRenderer = mainFrame().contentRenderer();
-    return contentRenderer ? contentRenderer->columnCount(contentRenderer->columnInfo()) : 0;
+    return contentRenderer ? contentRenderer->pageCount() : 0;
 }
 

trunk/Source/WebCore/rendering/RenderMultiColumnSet.cpp

@@ -27 +27 @@
 #include "RenderMultiColumnSet.h"
 
+#include "FrameView.h"
 #include "PaintInfo.h"
 #include "RenderLayer.h"
 #include "RenderMultiColumnFlowThread.h"
 #include "RenderMultiColumnSpannerPlaceholder.h"
+#include "RenderView.h"
 
 namespace WebCore {
@@ -39 +41 @@
     , m_computedColumnWidth(0)
     , m_computedColumnHeight(0)
+    , m_availableColumnHeight(0)
     , m_maxColumnHeight(RenderFlowThread::maxLogicalHeight())
     , m_minSpaceShortage(RenderFlowThread::maxLogicalHeight())
@@ -152 +155 @@
     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.
 }
@@ -328 +344 @@
         m_maxColumnHeight = calculateMaxColumnHeight();
     if (requiresBalancing()) {
-        if (initial)
+        if (initial) {
             m_computedColumnHeight = 0;
+            m_availableColumnHeight = 0;
+        }
     } else
         setAndConstrainColumnHeight(heightAdjustedForSetOffset(multiColumnFlowThread()->columnHeightAvailable()));
@@ -397 +415 @@
 void RenderMultiColumnSet::computeLogicalHeight(LayoutUnit, LayoutUnit logicalTop, LogicalExtentComputedValues& computedValues) const
 {
-    computedValues.m_extent = m_computedColumnHeight;
+    computedValues.m_extent = m_availableColumnHeight;
     computedValues.m_position = logicalTop;
 }

trunk/Source/WebCore/rendering/RenderMultiColumnSet.h

@@ -179 +179 @@
     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.

trunk/Source/WebCore/rendering/RenderView.cpp

@@ -296 +296 @@
         return pageLogicalHeight();
     
-    if (hasColumns() && !style().hasInlineColumnAxis()) {
+    if ((hasColumns() || multiColumnFlowThread()) && !style().hasInlineColumnAxis()) {
         if (int pageLength = frameView().pagination().pageLength)
             return pageLength;
@@ -1258 +1258 @@
 }
 
+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

trunk/Source/WebCore/rendering/RenderView.h

@@ -161 +161 @@
     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; }