Bug 1379334 - Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar. draft
authorL. David Baron <dbaron@dbaron.org>
Tue, 11 Jul 2017 14:56:24 -0700
changeset 607769 859b3cefd177551aadadd0a6ed98bef561a1e2af
parent 607768 de7260da8e7acc006431c32492e1d6ac272a0e2c
child 607770 833795f91bae9cd01bdf7e4f8cc9a96a6e97f633
push id68105
push userdholbert@mozilla.com
push dateWed, 12 Jul 2017 21:16:09 +0000
bugs1379334, 1308876
milestone56.0a1
Bug 1379334 - Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar. This fixes an incremental layout bug, where the number of times we reflow the frame affects its layout. This is because we make the decisions about the vertical scrollbar before the horizontal scrollbar (and, when making the decision, adjust mHelper.mScrollPort for the size of the scrollbar). Thus, in order to avoid a situation where reflowing the scrollframe once leads us to have no vertical scrollbar, but reflowing it a second time leads us to add that scrollbar, we need to retest for the need for a vertical scrollbar after we add the horizontal one. (It's possible there are some other missing cases here, but this is the one that (a) already existed in the code and (b) is needed to fix the reftest failure on Windows that I got on bug 1308876, in layout/reftests/text-overflow/xulscroll.html . The reftest here shows the bug even without bug 1308876 (though I confirmed that only by loading the test and reference in a nightly build, not in the reftest harness). I did test that, in combination with bug 1308876, the test fails without the patch and passes with the patch. MozReview-Commit-ID: LhMi7LbmB6J
layout/generic/nsGfxScrollFrame.cpp
layout/reftests/scrolling/reftest.list
layout/reftests/scrolling/xul-scrollbar-iterate-ref.html
layout/reftests/scrolling/xul-scrollbar-iterate.html
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -5158,26 +5158,48 @@ nsXULScrollFrame::XULLayout(nsBoxLayoutS
     // if the child is wider that the scroll area
     // and we don't have a scrollbar add one.
     if ((scrolledRect.width > mHelper.mScrollPort.width)
         && styles.mHorizontal == NS_STYLE_OVERFLOW_AUTO) {
 
       if (!mHelper.mHasHorizontalScrollbar) {
           // no scrollbar?
         if (AddHorizontalScrollbar(aState, scrollbarBottom)) {
-          needsLayout = true;
+
+          // if we added a horizontal scrollbar and we did not have a vertical
+          // there is a chance that by adding the horizontal scrollbar we will
+          // suddenly need a vertical scrollbar. Is a special case but it's
+          // important.
+          //
+          // But before we do that we need to relayout, since it's
+          // possible that the contents will flex as a result of adding a
+          // horizontal scrollbar and avoid the need for a vertical
+          // scrollbar.
+          //
+          // So instead of setting needsLayout to true here, do the
+          // layout immediately, and then consider whether to add the
+          // vertical scrollbar (and then maybe layout again).
+          {
+            nsBoxLayoutState resizeState(aState);
+            LayoutScrollArea(resizeState, oldScrollPosition);
+            needsLayout = false;
+          }
+
+          // Refresh scrolledRect because we called LayoutScrollArea.
+          scrolledRect = mHelper.GetScrolledRect();
+
+          if (styles.mVertical == NS_STYLE_OVERFLOW_AUTO &&
+              !mHelper.mHasVerticalScrollbar &&
+              scrolledRect.height > mHelper.mScrollPort.height) {
+            if (AddVerticalScrollbar(aState, scrollbarRight)) {
+              needsLayout = true;
+            }
+          }
         }
 
-        // if we added a horizontal scrollbar and we did not have a vertical
-        // there is a chance that by adding the horizontal scrollbar we will
-        // suddenly need a vertical scrollbar. Is a special case but its
-        // important.
-        //if (!mHasVerticalScrollbar && scrolledRect.height > scrollAreaRect.height - sbSize.height)
-        //  printf("****Gfx Scrollbar Special case hit!!*****\n");
-
       }
     } else {
         // if the area is smaller or equal to and we have a scrollbar then
         // remove it.
       if (mHelper.mHasHorizontalScrollbar) {
         RemoveHorizontalScrollbar(aState, scrollbarBottom);
         needsLayout = true;
       }
--- a/layout/reftests/scrolling/reftest.list
+++ b/layout/reftests/scrolling/reftest.list
@@ -91,8 +91,10 @@ fuzzy-if(asyncPan&&!layersGPUAccelerated
 == propagated-overflow-style-1a.html propagated-overflow-style-1-ref.html
 == propagated-overflow-style-1b.html propagated-overflow-style-1-ref.html
 == propagated-overflow-style-1c.html propagated-overflow-style-1-ref.html
 == propagated-overflow-style-2a.html propagated-overflow-style-2-ref.html
 == propagated-overflow-style-2b.html propagated-overflow-style-2-ref.html
 == propagated-overflow-style-2c.html propagated-overflow-style-2-ref.html
 == propagated-overflow-style-2d.html propagated-overflow-style-2-ref.html
 == propagated-overflow-style-2e.html propagated-overflow-style-2-ref.html
+
+== xul-scrollbar-iterate.html xul-scrollbar-iterate-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/scrolling/xul-scrollbar-iterate-ref.html
@@ -0,0 +1,27 @@
+<!DOCTYPE HTML>
+<title>XUL scrollbar testcase</title>
+<style>
+
+html { overflow: hidden }
+
+div.outer {
+  display: -moz-box;
+  overflow: scroll;
+  width: 200px;
+  height: 100px;
+  background: yellow;
+}
+
+div.inner {
+  width: 300px;
+  height: 100px;
+  min-width: 300px;
+  min-height: 100px;
+  background: aqua;
+}
+</style>
+
+<div class="outer">
+  <div class="inner">
+  </div>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/scrolling/xul-scrollbar-iterate.html
@@ -0,0 +1,27 @@
+<!DOCTYPE HTML>
+<title>XUL scrollbar testcase</title>
+<style>
+
+html { overflow: hidden }
+
+div.outer {
+  display: -moz-box;
+  overflow: auto;
+  width: 200px;
+  height: 100px;
+  background: yellow;
+}
+
+div.inner {
+  width: 300px;
+  height: 100px;
+  min-width: 300px;
+  min-height: 100px;
+  background: aqua;
+}
+</style>
+
+<div class="outer">
+  <div class="inner">
+  </div>
+</div>