Bug 1293242 - Part 2 - Propagate resized child size up to parent scrollable frame. r=dbaron draft
authorNeil Deakin <neil@mozilla.com>
Thu, 11 May 2017 17:34:40 +0100
changeset 576354 fbdc439317253b3f6a7b39dcfe52ac787ddc166c
parent 576353 85a9c00abd73a2ed010a9ca3a04fbe3a91d86add
child 628168 5c96c5e916514a171d0cce294781b4a7d86d0c73
push id58330
push userpaolo.mozmail@amadzone.org
push dateThu, 11 May 2017 17:11:12 +0000
reviewersdbaron
bugs1293242, 393970
milestone55.0a1
Bug 1293242 - Part 2 - Propagate resized child size up to parent scrollable frame. r=dbaron Currently XUL flexbox frames expand the size of themselves when a child is resized during layout (typically due to text that now wraps). This happens near the end of nsSprocketLayout::XULLayout. When this happens and the frame is the direct child of a scrollable frame, propagate this expanded size change up to the parent scrollable frame. This fixes issues with the transition in the downloads panel that contains a number of wrapping blocks inside a scrollable container that doesn't expand to the full height of the contents. The test_bug393970.xul asserts more on Linux, but the layout of the grid in that test now looks correct, whereas before it was cut off. MozReview-Commit-ID: 8B7XNJYxopS
layout/generic/nsGfxScrollFrame.cpp
layout/generic/nsGfxScrollFrame.h
layout/generic/nsIScrollableFrame.h
layout/reftests/xul/description-inside-overflowhidden-1-ref.xul
layout/reftests/xul/description-inside-overflowhidden-1.xul
layout/reftests/xul/description-inside-overflowhidden-2-ref.xul
layout/reftests/xul/description-inside-overflowhidden-2.xul
layout/reftests/xul/description-inside-overflowhidden-3-ref.xul
layout/reftests/xul/description-inside-overflowhidden-3.xul
layout/reftests/xul/reftest.list
layout/xul/nsSprocketLayout.cpp
layout/xul/test/test_bug393970.xul
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -1161,16 +1161,18 @@ NS_IMPL_FRAMEARENA_HELPERS(nsXULScrollFr
 nsXULScrollFrame::nsXULScrollFrame(nsStyleContext* aContext,
                                    bool aIsRoot,
                                    bool aClipAllDescendants)
   : nsBoxFrame(aContext, LayoutFrameType::Scroll, aIsRoot)
   , mHelper(ALLOW_THIS_IN_INITIALIZER_LIST(this), aIsRoot)
 {
   SetXULLayoutManager(nullptr);
   mHelper.mClipAllDescendants = aClipAllDescendants;
+  mChildExpandedHorizontally = false;
+  mChildExpandedVertically = false;
 }
 
 void
 nsXULScrollFrame::ScrollbarActivityStarted() const
 {
   if (mHelper.mScrollbarActivity) {
     mHelper.mScrollbarActivity->ActivityStarted();
   }
@@ -4924,16 +4926,27 @@ nsXULScrollFrame::LayoutScrollArea(nsBox
     // REVIEW: Have we accounted for both?
     ClampAndSetBounds(aState, childRect, aScrollPosition, true);
   }
 
   aState.SetLayoutFlags(oldflags);
 
 }
 
+void
+nsXULScrollFrame::ScrolledFrameResized(nsBoxLayoutState& aState,
+                                       nsIFrame* aFrame, nsSize aSize,
+                                       bool aHorizontal, bool aVertical)
+{
+  MOZ_ASSERT(aFrame == mHelper.mScrolledFrame);
+
+  mChildExpandedHorizontally = aHorizontal;
+  mChildExpandedVertically = aVertical;
+}
+
 void ScrollFrameHelper::PostOverflowEvent()
 {
   if (mAsyncScrollPortEvent.IsPending()) {
     return;
   }
 
   // Keep this in sync with FireScrollPortEvent().
   nsSize scrollportSize = mScrollPort.Size();
@@ -5095,19 +5108,77 @@ nsXULScrollFrame::XULLayout(nsBoxLayoutS
      mHelper.mHasVerticalScrollbar = true;
 
   if (mHelper.mHasHorizontalScrollbar)
      AddHorizontalScrollbar(aState, scrollbarBottom);
 
   if (mHelper.mHasVerticalScrollbar)
      AddVerticalScrollbar(aState, scrollbarRight);
 
+  mChildExpandedHorizontally = false;
+  mChildExpandedVertically = false;
+
   // layout our the scroll area
   LayoutScrollArea(aState, oldScrollPosition);
 
+  // If mChildExpandedHorizontally or mChildExpandedVertically is set, then
+  // the scrolled frame resized during layout, likely due to containing some
+  // text that needed to be wrapped. In this case get the scrolled frame's
+  // new size and expand the scroll frame to match.
+  nsRect finalRect;
+  nsRect finalPortRect;
+  if (mChildExpandedHorizontally || mChildExpandedVertically) {
+    // If an explicit width or height was assigned then just use that size
+    // instead of expanding in that direction.
+    bool widthSet, heightSet;
+    nsSize prefSizeCSS(NS_INTRINSICSIZE, NS_INTRINSICSIZE);
+    nsIFrame::AddXULPrefSize(this, prefSizeCSS, widthSet, heightSet);
+
+    nsSize scrolledSize = mHelper.mScrolledFrame->GetRect().Size();
+    nsMargin borderPadding;
+    GetXULBorderAndPadding(borderPadding);
+
+    nsSize scrollbarExpandSize;
+
+    nsRect newRect = GetRect();
+    if (!widthSet && mChildExpandedHorizontally) {
+      // Use the scrolled width plus our border and padding plus any visible scrollbar.
+      nsSize scrollbarSize;
+      if (mHelper.mHasVerticalScrollbar && mHelper.mVScrollbarBox) {
+        scrollbarSize = mHelper.mVScrollbarBox->GetXULPrefSize(aState);
+        nsBox::AddMargin(mHelper.mVScrollbarBox, scrollbarSize);
+        scrollbarExpandSize.width = scrollbarSize.width;
+      }
+
+      newRect.width = scrolledSize.width + borderPadding.LeftRight() + scrollbarSize.width;
+    }
+    if (!heightSet && mChildExpandedVertically) {
+      // Use the scrolled height plus our border and padding plus any visible scrollbar.
+      nsSize scrollbarSize;
+      if (mHelper.mHasHorizontalScrollbar && mHelper.mHScrollbarBox) {
+        scrollbarSize = mHelper.mHScrollbarBox->GetXULPrefSize(aState);
+        nsBox::AddMargin(mHelper.mHScrollbarBox, scrollbarSize);
+        scrollbarExpandSize.height = scrollbarSize.height;
+      }
+
+      newRect.height = scrolledSize.height + borderPadding.TopBottom() + scrollbarSize.height;
+    }
+
+    SetXULBounds(aState, newRect);
+
+    // Now set the scrollport size to match the newly assigned size.
+    GetXULClientRect(clientRect);
+    if (mChildExpandedHorizontally) {
+      mHelper.mScrollPort.width = clientRect.width - scrollbarExpandSize.width;
+    }
+    if (mChildExpandedVertically) {
+      mHelper.mScrollPort.height = clientRect.height - scrollbarExpandSize.height;
+    }
+  }
+
   // now look at the content area and see if we need scrollbars or not
   bool needsLayout = false;
 
   // if we have 'auto' scrollbars look at the vertical case
   if (styles.mVertical != NS_STYLE_OVERFLOW_SCROLL) {
     // These are only good until the call to LayoutScrollArea.
     nsRect scrolledRect = mHelper.GetScrolledRect();
 
--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@@ -1045,16 +1045,22 @@ public:
   // Update the style on our scrolled frame.
   virtual void DoUpdateStyleOfOwnedAnonBoxes(mozilla::ServoStyleSet& aStyleSet,
                                              nsStyleChangeList& aChangeList,
                                              nsChangeHint aHintForThisFrame) override {
     UpdateStyleOfChildAnonBox(mHelper.GetScrolledFrame(), aStyleSet,
                               aChangeList, aHintForThisFrame);
   }
 
+  virtual void ScrolledFrameResized(nsBoxLayoutState& aState, nsIFrame* aFrame,
+                                    nsSize aSize, bool aHorizontal, bool aVertical) override {
+    // This should never be called for HTML scroll frames.
+    MOZ_ASSERT(false);
+  };
+
 #ifdef DEBUG_FRAME_DUMP
   virtual nsresult GetFrameName(nsAString& aResult) const override;
 #endif
 
 #ifdef ACCESSIBILITY
   virtual mozilla::a11y::AccType AccessibleType() override;
 #endif
 
@@ -1489,16 +1495,19 @@ public:
 
   virtual void DoUpdateStyleOfOwnedAnonBoxes(mozilla::ServoStyleSet& aStyleSet,
                                              nsStyleChangeList& aChangeList,
                                              nsChangeHint aHintForThisFrame) override {
     UpdateStyleOfChildAnonBox(mHelper.GetScrolledFrame(), aStyleSet,
                               aChangeList, aHintForThisFrame);
   }
 
+   virtual void ScrolledFrameResized(nsBoxLayoutState& aState, nsIFrame* aFrame,
+                                     nsSize aSize, bool aHorizontal, bool aVertical) override;
+
 #ifdef DEBUG_FRAME_DUMP
   virtual nsresult GetFrameName(nsAString& aResult) const override;
 #endif
 
 protected:
   nsXULScrollFrame(nsStyleContext* aContext, bool aIsRoot,
                    bool aClipAllDescendants);
 
@@ -1511,14 +1520,17 @@ protected:
      * edge, then subtract the current width to find the physical position.
      */
     if (!mHelper.IsPhysicalLTR()) {
       aRect.x = mHelper.mScrollPort.XMost() - aScrollPosition.x - aRect.width;
     }
     mHelper.mScrolledFrame->SetXULBounds(aState, aRect, aRemoveOverflowAreas);
   }
 
+  bool mChildExpandedHorizontally;
+  bool mChildExpandedVertically;
+
 private:
   friend class mozilla::ScrollFrameHelper;
   ScrollFrameHelper mHelper;
 };
 
 #endif /* nsGfxScrollFrame_h___ */
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@@ -487,11 +487,19 @@ public:
   virtual void AsyncScrollbarDragRejected() = 0;
 
   /**
    * Returns whether this scroll frame is the root scroll frame of the document
    * that it is in. Note that some documents don't have root scroll frames at
    * all (ie XUL documents) even though they may contain other scroll frames.
    */
   virtual bool IsRootScrollFrameOfDocument() const = 0;
+
+  /**
+   * Called by a child XUL flexbox to indicate that it had to grow due to a
+   * child block growing. This should increase the size of the scrolled frame.
+   * This should be ignored by other frame types.
+   */
+  virtual void ScrolledFrameResized(nsBoxLayoutState& aState, nsIFrame* aFrame,
+                                    nsSize aSize, bool aHorizontal, bool aVertical) = 0;
 };
 
 #endif
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-1-ref.xul
@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height not set in CSS, no scrollframe. -->
+<vbox style="max-width: 150px;">
+  <description style="width: 150px;"><span style="width: 149px;">Line 1</span> Line 2</description>
+</vbox>
+
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-1.xul
@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height not set in CSS, the scrollframe should expand. -->
+<vbox style="max-width: 150px; overflow: hidden;">
+  <description style="width: 150px;"><span style="width: 149px;">Line 1</span> Line 2</description>
+</vbox>
+
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-2-ref.xul
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height set in CSS, the scrollframe should not expand. -->
+<vbox style="max-width: 150px; background: green; height: 7px; overflow: hidden;">
+  <!-- This does not wrap, so it does not trigger scrollframe expansion code. -->
+  <description style="width: 150px;" value="Line 1" />
+</vbox>
+
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-2.xul
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height set in CSS, the scrollframe should not expand. -->
+<vbox style="max-width: 150px; background: green; height: 7px; overflow: hidden;">
+  <!-- This wraps, so it triggers scrollframe expansion code. -->
+  <description style="width: 150px;"><span style="width: 149px;">Line 1</span> Line 2</description>
+</vbox>
+
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-3-ref.xul
@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height constrained indirectly, the scrollframe should not expand. -->
+<vbox style="height: 7px;">
+  <vbox flex="1" style="max-width: 150px; background: green; overflow: hidden;">
+    <!-- This does not wrap, so it does not trigger scrollframe expansion code. -->
+    <description style="width: 150px;" value="Line 1" />
+  </vbox>
+</vbox>
+
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/description-inside-overflowhidden-3.xul
@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<!-- Height constrained indirectly, the scrollframe should not expand. -->
+<vbox style="height: 7px;">
+  <vbox flex="1" style="max-width: 150px; background: green; overflow: hidden;">
+    <!-- This wraps, so it triggers scrollframe expansion code. -->
+    <description style="width: 150px;"><span style="width: 149px;">Line 1</span> Line 2</description>
+  </vbox>
+</vbox>
+
+</window>
--- a/layout/reftests/xul/reftest.list
+++ b/layout/reftests/xul/reftest.list
@@ -67,8 +67,13 @@ fuzzy-if(webrender,16,20) == object-posi
 # Tests for rendering SVG images in a XUL <treecell>:
 # XXXdholbert: These are marked as "random" right now, since they might not
 # render the images they trying to test in time for the reftest snapshot, per
 # bug 1218954.
 skip == treecell-image-svg-1a.xul treecell-image-svg-1-ref.xul # bug 1218954
 skip == treecell-image-svg-1b.xul treecell-image-svg-1-ref.xul # bug 1218954
 
 == treechildren-padding-percent-1.xul treechildren-padding-percent-1-ref.xul
+
+# These tests verify the size of overflow: hidden elements with a wrapping block inside.
+== description-inside-overflowhidden-1.xul description-inside-overflowhidden-1-ref.xul
+== description-inside-overflowhidden-2.xul description-inside-overflowhidden-2-ref.xul
+== description-inside-overflowhidden-3.xul description-inside-overflowhidden-3-ref.xul
--- a/layout/xul/nsSprocketLayout.cpp
+++ b/layout/xul/nsSprocketLayout.cpp
@@ -604,16 +604,25 @@ nsSprocketLayout::XULLayout(nsIFrame* aB
       nsRect bounds(aBox->GetRect());
       if (tmpClientRect.width > originalSize.width)
         bounds.width = tmpClientRect.width;
 
       if (tmpClientRect.height > originalSize.height)
         bounds.height = tmpClientRect.height;
 
       aBox->SetXULBounds(aState, bounds);
+
+      // If inside a scrollframe, increase the size of the scrollframe
+      // to the same size as well.
+      nsIScrollableFrame* scrollableParent = do_QueryFrame(aBox->GetParent());
+      if (scrollableParent) {
+        scrollableParent->ScrolledFrameResized(aState, aBox, bounds.Size(),
+                                               tmpClientRect.width > originalSize.width,
+                                               tmpClientRect.height > originalSize.height);
+      }
     }
   }
 
   // Because our size grew, we now have to readjust because of box packing.  Repack
   // in order to update our x and y to the correct values.
   HandleBoxPack(aBox, frameState, x, y, originalClientRect, clientRect);
 
   // Compare against our original x and y and only worry about adjusting the children if
--- a/layout/xul/test/test_bug393970.xul
+++ b/layout/xul/test/test_bug393970.xul
@@ -46,17 +46,17 @@ https://bugzilla.mozilla.org/show_bug.cg
     </grid>
   </hbox>
 
   <!-- test code goes here -->
   <script type="application/javascript"><![CDATA[
     /** Test for Bug 393970 **/
 
     if (navigator.platform.startsWith("Linux")) {
-      SimpleTest.expectAssertions(0, 24);
+      SimpleTest.expectAssertions(0, 48);
     }
 
     var tests = [
       'overflow-x: hidden; overflow-y: hidden;',
       'overflow-x: scroll; overflow-y: hidden;',
       'overflow-x: hidden; overflow-y: scroll;',
       'overflow-x: scroll; overflow-y: scroll;',
     ];