Bug 1942036 - Don't ignore negative margins for scroll{Width,Height} on overflow:visible frames. r=dshin

Differential Revision: https://phabricator.services.mozilla.com/D234711

Author: emilio

layout/generic/nsBlockFrame.cpp

@@ -270,9 +270,15 @@ static nscolor GetBackplateColor(nsIFrame* aFrame) {
   return NS_ComposeColors(backgroundColor, currentBackgroundColor);
 }
 
-static nsRect GetNormalMarginRect(nsIFrame* aFrame) {
-  auto rect = aFrame->GetMarginRectRelativeToSelf();
-  return rect + aFrame->GetNormalPosition();
+static nsRect GetNormalMarginRect(const nsIFrame& aFrame,
+                                  bool aIncludePositiveMargins = true) {
+  nsMargin m = aFrame.GetUsedMargin().ApplySkipSides(aFrame.GetSkipSides());
+  if (!aIncludePositiveMargins) {
+    m.EnsureAtMost(nsMargin());
+  }
+  auto rect = aFrame.GetRectRelativeToSelf();
+  rect.Inflate(m);
+  return rect + aFrame.GetNormalPosition();
 }
 
 #ifdef DEBUG
@@ -2600,7 +2606,7 @@ nsRect nsBlockFrame::ComputePaddingInflatedScrollableOverflow(
 
 Maybe<nsRect> nsBlockFrame::GetLineFrameInFlowBounds(
     const nsLineBox& aLine, const nsIFrame& aLineChildFrame,
-    bool aConsiderMargins) const {
+    bool aConsiderPositiveMargins) const {
   MOZ_ASSERT(aLineChildFrame.GetParent() == this,
              "Line's frame doesn't belong to this block frame?");
   // Line participants are considered in-flow for content within the line
@@ -2612,9 +2618,7 @@ Maybe<nsRect> nsBlockFrame::GetLineFrameInFlowBounds(
     return Nothing{};
   }
   if (aLine.IsInline()) {
-    auto rect = aConsiderMargins ? aLineChildFrame.GetMarginRectRelativeToSelf()
-                                 : aLineChildFrame.GetRectRelativeToSelf();
-    return Some(rect + aLineChildFrame.GetNormalPosition());
+    return Some(GetNormalMarginRect(aLineChildFrame, aConsiderPositiveMargins));
   }
   const auto wm = GetWritingMode();
   auto rect = aLineChildFrame.GetRectRelativeToSelf();
@@ -2626,14 +2630,19 @@ Maybe<nsRect> nsBlockFrame::GetLineFrameInFlowBounds(
   const auto normalPosition = aLineChildFrame.GetLogicalSize(wm).BSize(wm) == 0
                                   ? linePoint
                                   : aLineChildFrame.GetNormalPosition();
-  if (aConsiderMargins) {
-    // Ensure we use the margin we actually carried out.
+  // Ensure we use the margin we actually carried out.
+  nsMargin margin;
+  if (aConsiderPositiveMargins) {
     auto logicalMargin = aLineChildFrame.GetLogicalUsedMargin(wm);
     logicalMargin.BEnd(wm) = aLine.GetCarriedOutBEndMargin().Get();
-    const auto margin = logicalMargin.GetPhysicalMargin(wm).ApplySkipSides(
+    margin = logicalMargin.GetPhysicalMargin(wm).ApplySkipSides(
+        aLineChildFrame.GetSkipSides());
+  } else {
+    margin = aLineChildFrame.GetUsedMargin().ApplySkipSides(
         aLineChildFrame.GetSkipSides());
-    rect.Inflate(margin);
+    margin.EnsureAtMost(nsMargin());
   }
+  rect.Inflate(margin);
   return Some(rect + normalPosition);
 }
 
@@ -2653,7 +2662,7 @@ void nsBlockFrame::UnionChildOverflow(OverflowAreas& aOverflowAreas,
   // happen only for overflow: visible), so that we don't incorrectly account
   // for margins that otherwise collapse through, see bug 1936156. Note that
   // ::-moz-scrolled-content is always a BFC (see `AnonymousBoxIsBFC`).
-  const bool considerMarginsForInFlowChildBounds =
+  const bool considerPositiveMarginsForInFlowChildBounds =
       isScrolled && HasAnyStateBits(NS_BLOCK_BFC);
 
   // Relying on aOverflowAreas having been set to frame border rect (if
@@ -2686,7 +2695,7 @@ void nsBlockFrame::UnionChildOverflow(OverflowAreas& aOverflowAreas,
       }
 
       if (auto lineFrameBounds = GetLineFrameInFlowBounds(
-              line, *lineFrame, considerMarginsForInFlowChildBounds)) {
+              line, *lineFrame, considerPositiveMarginsForInFlowChildBounds)) {
         inFlowChildBounds = inFlowChildBounds.UnionEdges(*lineFrameBounds);
       }
     }
@@ -2698,9 +2707,8 @@ void nsBlockFrame::UnionChildOverflow(OverflowAreas& aOverflowAreas,
         if (inkOverflowOnly || !isScrolled) {
           continue;
         }
-        auto rect = considerMarginsForInFlowChildBounds
-                        ? GetNormalMarginRect(f)
-                        : f->GetRectRelativeToSelf() + f->GetNormalPosition();
+        auto rect = GetNormalMarginRect(
+            *f, considerPositiveMarginsForInFlowChildBounds);
         inFlowChildBounds = inFlowChildBounds.UnionEdges(rect);
       }
     }
@@ -5774,12 +5782,11 @@ bool nsBlockFrame::PlaceLine(BlockReflowState& aState,
     lineOverflowAreas.UnionWith(aLine->GetOverflowAreas());
     aLine->SetOverflowAreas(lineOverflowAreas);
     if (Style()->GetPseudoType() == PseudoStyleType::scrolledContent) {
-      auto itr = aLine->Floats().begin();
+      Span<const nsIFrame* const> floats(aLine->Floats());
       // Guaranteed to have at least 1 element since `HasFloats()` is true.
-      auto floatRect = GetNormalMarginRect(*itr);
-      ++itr;
-      for (; itr != aLine->Floats().end(); ++itr) {
-        floatRect = floatRect.UnionEdges(GetNormalMarginRect(*itr));
+      auto floatRect = GetNormalMarginRect(*floats[0]);
+      for (const nsIFrame* f : floats.From(1)) {
+        floatRect = floatRect.UnionEdges(GetNormalMarginRect(*f));
       }
       auto inFlowBounds = aLine->GetInFlowChildBounds();
       aLine->SetInFlowChildBounds(

layout/generic/nsBlockFrame.h

@@ -265,9 +265,9 @@ class nsBlockFrame : public nsContainerFrame {
 
   nsRect ComputePaddingInflatedScrollableOverflow(
       const nsRect& aInFlowChildBounds) const;
-  Maybe<nsRect> GetLineFrameInFlowBounds(const nsLineBox& aLine,
-                                         const nsIFrame& aLineChildFrame,
-                                         bool aConsiderMargins = true) const;
+  Maybe<nsRect> GetLineFrameInFlowBounds(
+      const nsLineBox& aLine, const nsIFrame& aLineChildFrame,
+      bool aConsiderPositiveMargins = true) const;
 
   template <typename LineIteratorType>
   Maybe<nscoord> GetBaselineBOffset(LineIteratorType aStart,

testing/web-platform/tests/css/cssom-view/scrollWidthHeight-overflow-visible-negative-margins.html

@@ -0,0 +1,60 @@
+<!doctype html>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#dom-element-scrollwidth">
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#dom-element-scrollheight">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1936156">
+<title>scroll{Width,Height} shouldn't account for collapsed margins, in order not to report unnecessary overflow</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  #target {
+    width: 20px;
+    flex-direction: column;
+  }
+  #target > div {
+    background-color: green;
+    margin: -5px -7px;
+  }
+
+  #target > div > div {
+    height: 20px;
+    width: 20px;
+  }
+</style>
+<div id="target">
+  <div><div></div></div>
+  <div><div></div></div>
+  <div><div></div></div>
+  <div><div></div></div>
+</div>
+<script>
+let target = document.getElementById("target");
+// "clip" is not really scrollable, but should match as well.
+for (let overflow of ["visible", "hidden", "auto", "scroll", "clip"]) {
+  // First one creates no overflow, last one does create overflow.
+  // TODO(emilio): Figure out why the overflow padding and some of the grid cases
+  // are not interoperable across platforms / engines. Maybe scrollbar-width dependent?
+  for (let padding of ["10px 20px" /* , "2px" */]) {
+    for (let border of ["0", "3px solid"]) {
+      for (let display of ["flex", "block", "flow-root", "inline-block", "inline-flex", /*  "grid", "inline-grid" */]) {
+        test(function() {
+          let [expectedOverflowY, expectedOverflowX] = padding == "10px 20px" ? [0, 0] : [3, 5];
+          target.style.overflow = overflow;
+          target.style.display = display;
+          target.style.border = border;
+          target.style.padding = padding;
+          let sh = target.scrollHeight;
+          let sw = target.scrollWidth;
+          let ch = target.clientHeight;
+          let cw = target.clientWidth;
+          assert_equals(sh, ch + expectedOverflowY, `scrollHeight expecting ${expectedOverflowY}px of overflow`);
+          assert_equals(sw, cw + expectedOverflowX, `scrollWidth expecting ${expectedOverflowX}px of overflow`);
+        }, `scroll{Width,Height} with negative margins with overflow: ${overflow}, padding: ${padding}, border: ${border}, display: ${display}`);
+      }
+    }
+  }
+}
+</script>