Bug 1364360 part 1. Make Element::GetScrollFrame follow the spec more closely in the quirks mode case. r=ehsan
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 25 May 2017 13:39:44 -0400
changeset 584698 3c315bd6cf00e3f2a7439cd36d4f55be6ae012db
parent 584697 f43ff529c51508085e7d302f2ebe6b50ad59dfe5
child 584699 e670c0dcaa9ff160c053ba0846752be348c83b4b
push id60843
push userbschouten@mozilla.com
push dateThu, 25 May 2017 21:57:22 +0000
reviewersehsan
bugs1364360
milestone55.0a1
Bug 1364360 part 1. Make Element::GetScrollFrame follow the spec more closely in the quirks mode case. r=ehsan
dom/base/Element.cpp
dom/base/nsDocument.cpp
dom/base/nsIDocument.h
layout/reftests/bugs/1364360-1-ref.html
layout/reftests/bugs/1364360-1.html
layout/reftests/bugs/reftest.list
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -678,40 +678,40 @@ Element::GetScrollFrame(nsIFrame **aStyl
     GetPrimaryFrame(aFlushLayout ? FlushType::Layout : FlushType::None);
   if (frame) {
     frame = nsLayoutUtils::GetStyleFrame(frame);
   }
 
   if (aStyledFrame) {
     *aStyledFrame = frame;
   }
-  if (!frame) {
-    return nullptr;
-  }
-
-  // menu frames implement GetScrollTargetFrame but we don't want
-  // to use it here.  Similar for comboboxes.
-  LayoutFrameType type = frame->Type();
-  if (type != LayoutFrameType::Menu &&
-      type != LayoutFrameType::ComboboxControl) {
-    nsIScrollableFrame *scrollFrame = frame->GetScrollTargetFrame();
-    if (scrollFrame)
-      return scrollFrame;
+  if (frame) {
+    // menu frames implement GetScrollTargetFrame but we don't want
+    // to use it here.  Similar for comboboxes.
+    LayoutFrameType type = frame->Type();
+    if (type != LayoutFrameType::Menu &&
+        type != LayoutFrameType::ComboboxControl) {
+      nsIScrollableFrame *scrollFrame = frame->GetScrollTargetFrame();
+      if (scrollFrame) {
+        MOZ_ASSERT(!OwnerDoc()->IsScrollingElement(this),
+                   "How can we have a scrollframe if we're the "
+                   "scrollingElement for our document?");
+        return scrollFrame;
+      }
+    }
   }
 
   nsIDocument* doc = OwnerDoc();
-  bool quirksMode = doc->GetCompatibilityMode() == eCompatibility_NavQuirks;
-  Element* elementWithRootScrollInfo =
-    quirksMode ? doc->GetBodyElement() : doc->GetRootElement();
-  if (this == elementWithRootScrollInfo) {
-    // In quirks mode, the scroll info for the body element should map to the
-    // root scrollable frame.
-    // In strict mode, the scroll info for the root element should map to the
-    // the root scrollable frame.
-    return frame->PresContext()->PresShell()->GetRootScrollFrameAsScrollable();
+  // Note: This IsScrollingElement() call can flush frames, if we're the body of
+  // a quirks mode document.
+  if (OwnerDoc()->IsScrollingElement(this)) {
+    // Our scroll info should map to the root scrollable frame if there is one.
+    if (nsIPresShell* shell = doc->GetShell()) {
+      return shell->GetRootScrollFrameAsScrollable();
+    }
   }
 
   return nullptr;
 }
 
 void
 Element::ScrollIntoView()
 {
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -10540,19 +10540,22 @@ nsIDocument::CaretPositionFromPoint(floa
 NS_IMETHODIMP
 nsDocument::CaretPositionFromPoint(float aX, float aY, nsISupports** aCaretPos)
 {
   NS_ENSURE_ARG_POINTER(aCaretPos);
   *aCaretPos = nsIDocument::CaretPositionFromPoint(aX, aY).take();
   return NS_OK;
 }
 
-static bool
-IsPotentiallyScrollable(HTMLBodyElement* aBody)
-{
+bool
+nsIDocument::IsPotentiallyScrollable(HTMLBodyElement* aBody)
+{
+  // We rely on correct frame information here, so need to flush frames.
+  FlushPendingNotifications(FlushType::Frames);
+
   // An element is potentially scrollable if all of the following conditions are
   // true:
 
   // The element has an associated CSS layout box.
   nsIFrame* bodyFrame = aBody->GetPrimaryFrame();
   if (!bodyFrame) {
     return false;
   }
@@ -10575,29 +10578,50 @@ IsPotentiallyScrollable(HTMLBodyElement*
   }
 
   return true;
 }
 
 Element*
 nsIDocument::GetScrollingElement()
 {
+  // Keep this in sync with IsScrollingElement.
   if (GetCompatibilityMode() == eCompatibility_NavQuirks) {
-    FlushPendingNotifications(FlushType::Layout);
     HTMLBodyElement* body = GetBodyElement();
     if (body && !IsPotentiallyScrollable(body)) {
       return body;
     }
 
     return nullptr;
   }
 
   return GetRootElement();
 }
 
+bool
+nsIDocument::IsScrollingElement(Element* aElement)
+{
+  // Keep this in sync with GetScrollingElement.
+  MOZ_ASSERT(aElement);
+
+  if (GetCompatibilityMode() != eCompatibility_NavQuirks) {
+    return aElement == GetRootElement();
+  }
+
+  HTMLBodyElement* body = GetBodyElement();
+  if (aElement != body) {
+    return false;
+  }
+
+  // Now we know body is non-null, since aElement is not null.  It's the
+  // scrolling element for the document if it itself is not potentially
+  // scrollable.
+  return !IsPotentiallyScrollable(body);
+}
+
 void
 nsIDocument::ObsoleteSheet(nsIURI *aSheetURI, ErrorResult& rv)
 {
   nsresult res = CSSLoader()->ObsoleteSheet(aSheetURI);
   if (NS_FAILED(res)) {
     rv.Throw(res);
   }
 }
--- a/dom/base/nsIDocument.h
+++ b/dom/base/nsIDocument.h
@@ -2772,16 +2772,21 @@ public:
    *           page coordinates.
    * @param aY Vertical point at which to determine the caret position, in
    *           page coordinates.
    */
   already_AddRefed<nsDOMCaretPosition>
     CaretPositionFromPoint(float aX, float aY);
 
   Element* GetScrollingElement();
+  // A way to check whether a given element is what would get returned from
+  // GetScrollingElement.  It can be faster than comparing to the return value
+  // of GetScrollingElement() due to being able to avoid flushes in various
+  // cases.  This method assumes that null is NOT passed.
+  bool IsScrollingElement(Element* aElement);
 
   // QuerySelector and QuerySelectorAll already defined on nsINode
   nsINodeList* GetAnonymousNodes(Element& aElement);
   Element* GetAnonymousElementByAttribute(Element& aElement,
                                           const nsAString& aAttrName,
                                           const nsAString& aAttrValue);
   Element* GetBindingParent(nsINode& aNode);
   void LoadBindingDocument(const nsAString& aURI,
@@ -3023,16 +3028,19 @@ protected:
 
   // Update our frame request callback scheduling state, if needed.  This will
   // schedule or unschedule them, if necessary, and update
   // mFrameRequestCallbacksScheduled.  aOldShell should only be passed when
   // mPresShell is becoming null; in that case it will be used to get hold of
   // the relevant refresh driver.
   void UpdateFrameRequestCallbackSchedulingState(nsIPresShell* aOldShell = nullptr);
 
+  // Helper for GetScrollingElement/IsScrollingElement.
+  bool IsPotentiallyScrollable(mozilla::dom::HTMLBodyElement* aBody);
+
   nsCString mReferrer;
   nsString mLastModified;
 
   nsCOMPtr<nsIURI> mDocumentURI;
   nsCOMPtr<nsIURI> mOriginalURI;
   nsCOMPtr<nsIURI> mChromeXHRDocURI;
   nsCOMPtr<nsIURI> mDocumentBaseURI;
   nsCOMPtr<nsIURI> mChromeXHRDocBaseURI;
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/1364360-1-ref.html
@@ -0,0 +1,15 @@
+<!-- Quirks mode on purpose -->
+<html>
+  <body style="margin: 0; padding: 0; border: none;"></body>
+  <script>
+    var html = document.documentElement;
+    var div = document.createElement("div");
+    div.style.height = "4000px";
+    html.appendChild(div);
+    html.appendChild(document.createTextNode("text"));
+    div = document.createElement("div");
+    div.style.height = "4000px";
+    html.appendChild(div);
+    document.body.scrollTop = 4000;
+  </script>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/1364360-1.html
@@ -0,0 +1,15 @@
+<!-- Quirks mode on purpose -->
+<html>
+  <body style="display:none;"></body>
+  <script>
+    var html = document.documentElement;
+    var div = document.createElement("div");
+    div.style.height = "4000px";
+    html.appendChild(div);
+    html.appendChild(document.createTextNode("text"));
+    div = document.createElement("div");
+    div.style.height = "4000px";
+    html.appendChild(div);
+    document.body.scrollTop = 4000;
+  </script>
+</html>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1999,9 +1999,10 @@ fails-if(stylo) == 1348481-3.html 134848
 == 1358375-1.html 1358375-ref.html
 == 1358375-2.html 1358375-ref.html
 == 1358375-3.html 1358375-ref.html
 == 1364280-1.html 1364280-1-ref.html
 == 1364280-2a.html 1364280-2-ref.html
 == 1364280-2b.html 1364280-2-ref.html
 == 1364280-2c.html 1364280-2-ref.html
 == 1364335.html 1364335-ref.html
+== 1364360-1.html 1364360-1-ref.html
 == 1366144.html 1366144-ref.html