Changeset 124168 in webkit

Timestamp:

Jul 30, 2012 8:38:53 PM (12 years ago)

Author:

jchaffraix@webkit.org

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderBlock.cpp (modified) (1 diff)
• rendering/RenderDeprecatedFlexibleBox.cpp (modified) (1 diff)
• rendering/RenderFlexibleBox.cpp (modified) (2 diffs)
• rendering/RenderGrid.cpp (modified) (1 diff)
• rendering/RenderLayer.cpp (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

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

trunk/Source/WebCore/rendering/RenderBlock.cpp

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

trunk/Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp

@@ -263 +263 @@
     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);

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -195 +195 @@
     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 +248 @@
 
     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;

trunk/Source/WebCore/rendering/RenderGrid.cpp

@@ -80 +80 @@
 
     m_overflow.clear();
-
-    if (scrollsOverflow()) {
-        if (style()->overflowX() == OSCROLL)
-            layer()->setHasHorizontalScrollbar(true);
-        if (style()->overflowY() == OSCROLL)
-            layer()->setHasVerticalScrollbar(true);
-    }
 
     layoutGridItems();

trunk/Source/WebCore/rendering/RenderLayer.cpp

@@ -2230 +2230 @@
     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());
@@ -2523 +2527 @@
     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);
 
@@ -4819 +4827 @@
 }
 
-static bool overflowCanHaveAScrollbar(EOverflow overflow)
-{
-    return overflow == OAUTO || overflow == OSCROLL || overflow == OOVERLAY;
+static bool overflowRequiresScrollbar(EOverflow overflow)
+{
+    return overflow == OSCROLL;
+}
+
+static bool overflowDefinesAutomaticScrollbar(EOverflow overflow)
+{
+    return overflow == OAUTO || overflow == OOVERLAY;
 }
 
@@ -4827 +4840 @@
 {
     // Overflow are a box concept.
-    if (!renderBox())
-        return;
-
-    EOverflow overflowX = renderBox()->style()->overflowX();
-    EOverflow overflowY = renderBox()->style()->overflowY();
-    if (hasHorizontalScrollbar() && !overflowCanHaveAScrollbar(overflowX))
-        setHasHorizontalScrollbar(false);
-    if (hasVerticalScrollbar() && !overflowCanHaveAScrollbar(overflowY))
-        setHasVerticalScrollbar(false);
+    RenderBox* box = renderBox();
+    if (!box)
+        return;
+
+    // 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);
     }