Bug 1665796 - Make sure Document::IsScrollingElement and overflow propagation code agree. r=mats
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 25 Oct 2020 10:45:37 +0000
changeset 554351 2e15f7800a8f373c56290ff6e01a2e4a72be936a
parent 554350 a4341527c628b3ba77159a421f70da5adf9f8d54
child 554352 ee8ad6205433e088a716f5f4f4b36f10d2ca5d66
push id37892
push usernbeleuzu@mozilla.com
push dateSun, 25 Oct 2020 21:41:16 +0000
treeherdermozilla-central@61c35792ca70 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1665796, 1635473
milestone84.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1665796 - Make sure Document::IsScrollingElement and overflow propagation code agree. r=mats They got out of sync in bug 1635473. Differential Revision: https://phabricator.services.mozilla.com/D93117
dom/base/Document.cpp
layout/base/nsPresContext.cpp
layout/style/nsStyleStruct.h
testing/web-platform/tests/css/cssom-view/scroll-overflow-clip-quirks-001.html
testing/web-platform/tests/css/cssom-view/scroll-overflow-clip-quirks-002.html
testing/web-platform/tests/css/cssom-view/scrollingElement.html
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -12881,31 +12881,28 @@ bool Document::IsPotentiallyScrollable(H
   // of the following conditions are true:
 
   // The element has an associated CSS layout box.
   nsIFrame* bodyFrame = nsLayoutUtils::GetStyleFrame(aBody);
   if (!bodyFrame) {
     return false;
   }
 
-  // The element's parent element's computed value of the overflow-x or
-  // overflow-y properties is neither visible nor clip.
+  // The element's parent element's computed value of the overflow-x and
+  // overflow-y properties are visible.
   MOZ_ASSERT(aBody->GetParent() == aBody->OwnerDoc()->GetRootElement());
   nsIFrame* parentFrame = nsLayoutUtils::GetStyleFrame(aBody->GetParent());
-  if (parentFrame && !parentFrame->StyleDisplay()->IsScrollableOverflow()) {
+  if (parentFrame &&
+      parentFrame->StyleDisplay()->OverflowIsVisibleInBothAxis()) {
     return false;
   }
 
   // The element's computed value of the overflow-x or overflow-y properties is
-  // neither visible nor clip.
-  if (!bodyFrame->StyleDisplay()->IsScrollableOverflow()) {
-    return false;
-  }
-
-  return true;
+  // not visible.
+  return !bodyFrame->StyleDisplay()->OverflowIsVisibleInBothAxis();
 }
 
 Element* Document::GetScrollingElement() {
   // Keep this in sync with IsScrollingElement.
   if (GetCompatibilityMode() == eCompatibility_NavQuirks) {
     RefPtr<HTMLBodyElement> body = GetBodyElement();
     if (body && !IsPotentiallyScrollable(body)) {
       return body;
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -1115,18 +1115,19 @@ static bool CheckOverflow(const Computed
   const nsStyleDisplay* display = aComputedStyle->StyleDisplay();
 
   // If they will generate no box, just don't.
   if (display->mDisplay == StyleDisplay::None ||
       display->mDisplay == StyleDisplay::Contents) {
     return false;
   }
 
-  if (display->mOverflowX == StyleOverflow::Visible &&
-      display->mOverflowY == StyleOverflow::Visible) {
+  // NOTE(emilio): This check needs to match the one in
+  // Document::IsPotentiallyScrollable.
+  if (display->OverflowIsVisibleInBothAxis()) {
     return false;
   }
 
   *aStyles =
       ScrollStyles(*display, ScrollStyles::MapOverflowToValidScrollStyle);
   return true;
 }
 
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -1512,16 +1512,21 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
 
   bool IsScrollableOverflow() const {
     // Visible and Clip can be combined but not with other values,
     // so checking mOverflowX is enough.
     return mOverflowX != mozilla::StyleOverflow::Visible &&
            mOverflowX != mozilla::StyleOverflow::Clip;
   }
 
+  bool OverflowIsVisibleInBothAxis() const {
+    return mOverflowX == mozilla::StyleOverflow::Visible &&
+           mOverflowY == mozilla::StyleOverflow::Visible;
+  }
+
   bool IsContainPaint() const {
     return (mContain & mozilla::StyleContain::PAINT) &&
            !IsInternalRubyDisplayType() && !IsInternalTableStyleExceptCell();
   }
 
   bool IsContainLayout() const {
     // Note: The spec for layout containment says it should
     // have no effect on non-atomic, inline-level boxes. We
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom-view/scroll-overflow-clip-quirks-001.html
@@ -0,0 +1,23 @@
+<!-- quirks -->
+<title>CSSOM scrollingElement reflects the propagated scroll to viewport correctly</title>
+<meta charset="utf-8">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="help" href="https://drafts.csswg.org/css-overflow/#overflow-propagation">
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#dom-document-scrollingelement">
+<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/5601">
+<style>
+  :root {
+    overflow: clip;
+  }
+  body {
+    overflow: scroll;
+  }
+</style>
+<script>
+test(function() {
+  assert_equals(document.scrollingElement, null);
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom-view/scroll-overflow-clip-quirks-002.html
@@ -0,0 +1,20 @@
+<!-- quirks -->
+<title>CSSOM scrollingElement reflects the propagated scroll to viewport correctly</title>
+<meta charset="utf-8">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="help" href="https://drafts.csswg.org/css-overflow/#overflow-propagation">
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#dom-document-scrollingelement">
+<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/5601">
+<style>
+  body {
+    overflow: clip;
+  }
+</style>
+<script>
+test(function() {
+  assert_equals(document.scrollingElement, document.body);
+});
+</script>
--- a/testing/web-platform/tests/css/cssom-view/scrollingElement.html
+++ b/testing/web-platform/tests/css/cssom-view/scrollingElement.html
@@ -88,22 +88,26 @@ function nonQuirksTest(rootDisplay, body
     nonQuirksFrame.onload = this.step_func_done(function() {
       var nonQuirksDoc = nonQuirksFrame.contentDocument;
       assert_equals(nonQuirksDoc.compatMode, "CSS1Compat", "Should be in standards mode.");
       assert_not_equals(nonQuirksDoc.body, null, "Should have a body element");
 
       nonQuirksDoc.documentElement.style.display = rootDisplay;
       nonQuirksDoc.body.style.display = bodyDisplay;
 
-      // Tests for non-quirks mode document.
       assert_equals(nonQuirksDoc.scrollingElement, nonQuirksDoc.documentElement,
         "scrollingElement in standards mode should be the document element.");
-      nonQuirksDoc.documentElement.style.overflow = "scroll";
-      nonQuirksDoc.body.style.overflow = "scroll";
-      assert_equals(nonQuirksDoc.scrollingElement, nonQuirksDoc.documentElement);
+
+      for (let rootOverflow of ["", "clip", "scroll", "hidden", "visible"]) {
+        for (let bodyOverflow of ["", "clip", "scroll", "hidden", "visible"]) {
+          nonQuirksDoc.documentElement.style.overflow = rootOverflow;
+          nonQuirksDoc.body.style.overflow = bodyOverflow;
+          assert_equals(nonQuirksDoc.scrollingElement, nonQuirksDoc.documentElement);
+        }
+      }
 
       nonQuirksDoc.removeChild(nonQuirksDoc.documentElement);
       assert_equals(nonQuirksDoc.scrollingElement, null);
       nonQuirksDoc.appendChild(nonQuirksDoc.createElement("foobar"));
       assert_equals(nonQuirksDoc.scrollingElement.localName, "foobar");
 
       nonQuirksFrame.remove();
     });