Remove overflow: scroll handling in block flow layout methods
authorjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 31 Jul 2012 03:38:53 +0000 (03:38 +0000)
committerjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 31 Jul 2012 03:38:53 +0000 (03:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=92689

Reviewed by Simon Fraser.

The overflow: scroll scrollbars creation was done at layout time for all RenderBlocks and
descendants. This was not only wrong ('overflow' only changes at style change time) but it
was also introducing some code duplication.

The gist of this change is to share the code by moving it to RenderLayer::updateScrollbarsAfterStyleChange,
this includes the code from bug 69993 to special case list box part.

Covered by existing tests:
- All fast/overflow ones.
- For the list box change:
    fast/forms/select-overflow-scroll-inherited.html
    fast/forms/select-overflow-scroll.html
- For the flexbox:
    css3/flexbox/preferred-widths-orthogonal.html
    css3/flexbox/preferred-widths.html

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::layoutBlock):
* rendering/RenderDeprecatedFlexibleBox.cpp:
(WebCore::RenderDeprecatedFlexibleBox::layoutBlock):
* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::layoutBlock):
* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::layoutBlock):
Removed the common code here.

* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::computePreferredLogicalWidths):
Changed to an ASSERT now that the right scrollbars are created. This is
fine as overflow-x/y are physical coordinates and our access was following that.

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::invalidateScrollbarRect):
Added an early return here if we are not attached yet as RenderLayer::styleChanged
is called at attachment time before we are inserted in the tree. This is fine as the
scrollbars are part of the object which will be painted after the first layout.

(WebCore::overflowRequiresAScrollbar):
(WebCore::overflowDefinesAutomaticScrollbar):
Split the logic in those 2 functions.

(WebCore::RenderLayer::updateScrollbarsAfterLayout):
Updated to use the require / can-have functions. Also added
an early return for list box parts as required by bug 69993.

(WebCore::RenderLayer::updateScrollbarsAfterStyleChange):
Added an early return for list box parts as required by bug 69993,
also removed some unneeded NULL-checks that were added for list box parts.

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

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderGrid.cpp
Source/WebCore/rendering/RenderLayer.cpp

index 3c1010c248d1e90e8c165f9dc5de66bdd4f8f0d1..b6a74237e6276e7cb94517e29c5f64b436cfd928 100644 (file)
@@ -1,3 +1,59 @@
+2012-07-30  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Remove overflow: scroll handling in block flow layout methods
+        https://bugs.webkit.org/show_bug.cgi?id=92689
+
+        Reviewed by Simon Fraser.
+
+        The overflow: scroll scrollbars creation was done at layout time for all RenderBlocks and
+        descendants. This was not only wrong ('overflow' only changes at style change time) but it
+        was also introducing some code duplication.
+
+        The gist of this change is to share the code by moving it to RenderLayer::updateScrollbarsAfterStyleChange,
+        this includes the code from bug 69993 to special case list box part.
+
+        Covered by existing tests:
+        - All fast/overflow ones.
+        - For the list box change:
+            fast/forms/select-overflow-scroll-inherited.html
+            fast/forms/select-overflow-scroll.html
+        - For the flexbox:
+            css3/flexbox/preferred-widths-orthogonal.html
+            css3/flexbox/preferred-widths.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::layoutBlock):
+        * rendering/RenderDeprecatedFlexibleBox.cpp:
+        (WebCore::RenderDeprecatedFlexibleBox::layoutBlock):
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::layoutBlock):
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::layoutBlock):
+        Removed the common code here.
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::computePreferredLogicalWidths):
+        Changed to an ASSERT now that the right scrollbars are created. This is
+        fine as overflow-x/y are physical coordinates and our access was following that.
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::invalidateScrollbarRect):
+        Added an early return here if we are not attached yet as RenderLayer::styleChanged
+        is called at attachment time before we are inserted in the tree. This is fine as the
+        scrollbars are part of the object which will be painted after the first layout.
+
+        (WebCore::overflowRequiresAScrollbar):
+        (WebCore::overflowDefinesAutomaticScrollbar):
+        Split the logic in those 2 functions.
+
+        (WebCore::RenderLayer::updateScrollbarsAfterLayout):
+        Updated to use the require / can-have functions. Also added
+        an early return for list box parts as required by bug 69993.
+
+        (WebCore::RenderLayer::updateScrollbarsAfterStyleChange):
+        Added an early return for list box parts as required by bug 69993,
+        also removed some unneeded NULL-checks that were added for list box parts.
+
 2012-07-30  Vivek Galatage  <vivekgalatage@gmail.com>
 
         fillWithEmptyClients method should also initialize chromeClient with EmptyChromeClient
index 0e4a77a78ccfdb11261b759d3e19ea0a4d8b6511..7e64014e03df1890314342929d48600e7b9ba7ae 100755 (executable)
@@ -1472,14 +1472,6 @@ void RenderBlock::layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeigh
         setPaginationStrut(0);
     }
 
-    // For overflow:scroll blocks, ensure we have both scrollbars in place always.
-    if (scrollsOverflow() && style()->appearance() != ListboxPart) {
-        if (styleToUse->overflowX() == OSCROLL)
-            layer()->setHasHorizontalScrollbar(true);
-        if (styleToUse->overflowY() == OSCROLL)
-            layer()->setHasVerticalScrollbar(true);
-    }
-
     LayoutUnit repaintLogicalTop = ZERO_LAYOUT_UNIT;
     LayoutUnit repaintLogicalBottom = ZERO_LAYOUT_UNIT;
     LayoutUnit maxFloatLogicalBottom = ZERO_LAYOUT_UNIT;
index 4ae42bc7daae9b84c458bce406539f2e47367011..f129991115ce89cc1a5e9ecdee8b6a41a404cb76 100644 (file)
@@ -262,14 +262,6 @@ void RenderDeprecatedFlexibleBox::layoutBlock(bool relayoutChildren, LayoutUnit)
 
     initMaxMarginValues();
 
-    // For overflow:scroll blocks, ensure we have both scrollbars in place always.
-    if (scrollsOverflow()) {
-        if (style()->overflowX() == OSCROLL)
-            layer()->setHasHorizontalScrollbar(true);
-        if (style()->overflowY() == OSCROLL)
-            layer()->setHasVerticalScrollbar(true);
-    }
-
     if (isHorizontal())
         layoutHorizontalBox(relayoutChildren);
     else
index 3b1d4737c66512a9784368c1f9bb0e65f5144677..961a9f31e150b7819a0b6bc61b89245b82e3c45c 100644 (file)
@@ -194,10 +194,10 @@ void RenderFlexibleBox::computePreferredLogicalWidths()
     LayoutUnit scrollbarWidth = 0;
     if (hasOverflowClip()) {
         if (isHorizontalWritingMode() && styleToUse->overflowY() == OSCROLL) {
-            layer()->setHasVerticalScrollbar(true);
+            ASSERT(layer()->hasVerticalScrollbar());
             scrollbarWidth = verticalScrollbarWidth();
         } else if (!isHorizontalWritingMode() && styleToUse->overflowX() == OSCROLL) {
-            layer()->setHasHorizontalScrollbar(true);
+            ASSERT(layer()->hasHorizontalScrollbar());
             scrollbarWidth = horizontalScrollbarHeight();
         }
     }
@@ -248,14 +248,6 @@ void RenderFlexibleBox::layoutBlock(bool relayoutChildren, LayoutUnit)
 
     m_overflow.clear();
 
-    // For overflow:scroll blocks, ensure we have both scrollbars in place always.
-    if (scrollsOverflow()) {
-        if (style()->overflowX() == OSCROLL)
-            layer()->setHasHorizontalScrollbar(true);
-        if (style()->overflowY() == OSCROLL)
-            layer()->setHasVerticalScrollbar(true);
-    }
-
     WTF::Vector<LineContext> lineContexts;
     OrderHashSet orderValues;
     computeMainAxisPreferredSizes(relayoutChildren, orderValues);
index b4d2a0c08b4f6072ec605da6c47242be8bdfa2e0..dcb1861a9e21565010b8089c46b69b0c1c9547b7 100644 (file)
@@ -80,13 +80,6 @@ void RenderGrid::layoutBlock(bool relayoutChildren, LayoutUnit)
 
     m_overflow.clear();
 
-    if (scrollsOverflow()) {
-        if (style()->overflowX() == OSCROLL)
-            layer()->setHasHorizontalScrollbar(true);
-        if (style()->overflowY() == OSCROLL)
-            layer()->setHasVerticalScrollbar(true);
-    }
-
     layoutGridItems();
 
     LayoutUnit oldClientAfterEdge = clientLogicalBottom();
index 74b0f5a67016efaef0950d33b39fc3b1538fa389..787986146443de3ec442a1e888d8d840159b39bf 100644 (file)
@@ -2229,6 +2229,10 @@ void RenderLayer::invalidateScrollbarRect(Scrollbar* scrollbar, const IntRect& r
     IntRect scrollRect = rect;
     RenderBox* box = renderBox();
     ASSERT(box);
+    // If we are not yet inserted into the tree, there is no need to repaint.
+    if (!box->parent())
+        return;
+
     if (scrollbar == m_vBar.get())
         scrollRect.move(verticalScrollbarStart(0, box->width()), box->borderTop());
     else
@@ -2522,13 +2526,17 @@ void RenderLayer::updateScrollbarsAfterLayout()
     RenderBox* box = renderBox();
     ASSERT(box);
 
+    // List box parts handle the scrollbars by themselves so we have nothing to do.
+    if (box->style()->appearance() == ListboxPart)
+        return;
+
     bool hasHorizontalOverflow = this->hasHorizontalOverflow();
     bool hasVerticalOverflow = this->hasVerticalOverflow();
 
     // overflow:scroll should just enable/disable.
-    if (m_hBar && renderer()->style()->overflowX() == OSCROLL)
+    if (renderer()->style()->overflowX() == OSCROLL)
         m_hBar->setEnabled(hasHorizontalOverflow);
-    if (m_vBar && renderer()->style()->overflowY() == OSCROLL)
+    if (renderer()->style()->overflowY() == OSCROLL)
         m_vBar->setEnabled(hasVerticalOverflow);
 
     // overflow:auto may need to lay out again if scrollbars got added/removed.
@@ -4818,33 +4826,45 @@ void RenderLayer::updateStackingContextsAfterStyleChange(const RenderStyle* oldS
     }
 }
 
-static bool overflowCanHaveAScrollbar(EOverflow overflow)
+static bool overflowRequiresScrollbar(EOverflow overflow)
+{
+    return overflow == OSCROLL;
+}
+
+static bool overflowDefinesAutomaticScrollbar(EOverflow overflow)
 {
-    return overflow == OAUTO || overflow == OSCROLL || overflow == OOVERLAY;
+    return overflow == OAUTO || overflow == OOVERLAY;
 }
 
 void RenderLayer::updateScrollbarsAfterStyleChange(const RenderStyle* oldStyle)
 {
     // Overflow are a box concept.
-    if (!renderBox())
+    RenderBox* box = renderBox();
+    if (!box)
         return;
 
-    EOverflow overflowX = renderBox()->style()->overflowX();
-    EOverflow overflowY = renderBox()->style()->overflowY();
-    if (hasHorizontalScrollbar() && !overflowCanHaveAScrollbar(overflowX))
-        setHasHorizontalScrollbar(false);
-    if (hasVerticalScrollbar() && !overflowCanHaveAScrollbar(overflowY))
-        setHasVerticalScrollbar(false);
+    // List box parts handle the scrollbars by themselves so we have nothing to do.
+    if (box->style()->appearance() == ListboxPart)
+        return;
+
+    EOverflow overflowX = box->style()->overflowX();
+    EOverflow overflowY = box->style()->overflowY();
+
+    // To avoid doing a relayout in updateScrollbarsAfterLayout, we try to keep any automatic scrollbar that was already present.
+    bool needsHorizontalScrollbar = (hasHorizontalScrollbar() && overflowDefinesAutomaticScrollbar(overflowX)) || overflowRequiresScrollbar(overflowX);
+    bool needsVerticalScrollbar = (hasVerticalScrollbar() && overflowDefinesAutomaticScrollbar(overflowY)) || overflowRequiresScrollbar(overflowY);
+    setHasHorizontalScrollbar(needsHorizontalScrollbar);
+    setHasVerticalScrollbar(needsVerticalScrollbar);
 
     // With overflow: scroll, scrollbars are always visible but may be disabled.
     // When switching to another value, we need to re-enable them (see bug 11985).
-    if (hasHorizontalScrollbar() && oldStyle->overflowX() == OSCROLL && overflowX != OSCROLL) {
-        ASSERT(overflowCanHaveAScrollbar(overflowX));
+    if (needsHorizontalScrollbar && oldStyle && oldStyle->overflowX() == OSCROLL && overflowX != OSCROLL) {
+        ASSERT(hasHorizontalScrollbar());
         m_hBar->setEnabled(true);
     }
 
-    if (hasVerticalScrollbar() && oldStyle->overflowY() == OSCROLL && overflowY != OSCROLL) {
-        ASSERT(overflowCanHaveAScrollbar(overflowY));
+    if (needsVerticalScrollbar && oldStyle && oldStyle->overflowY() == OSCROLL && overflowY != OSCROLL) {
+        ASSERT(hasVerticalScrollbar());
         m_vBar->setEnabled(true);
     }