Bug 1425107 - Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly. r=mats, a=gchang
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 14 Dec 2017 12:45:37 -0500
changeset 445430 9e7435aeb7bd1cb9825c092db7f7eb938def0945
parent 445429 64d69f61379046ae2f57c15b8bedae5be09720e1
child 445431 13b3ee92ee381296b05ff6a0c66ad153558cf3c0
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, gchang
bugs1425107
milestone58.0
Bug 1425107 - Fix setting scrollTop to 0 on an element with a pending scroll position restore to actually work correctly. r=mats, a=gchang MozReview-Commit-ID: 949eBXmKHlA
layout/generic/nsGfxScrollFrame.cpp
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/cssom-view/scrollTop-display-change-ref.html
testing/web-platform/tests/css/cssom-view/scrollTop-display-change.html
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2267,16 +2267,21 @@ ScrollFrameHelper::GetScrollPositionCSSP
  */
 void
 ScrollFrameHelper::ScrollToWithOrigin(nsPoint aScrollPosition,
                                           nsIScrollableFrame::ScrollMode aMode,
                                           nsAtom *aOrigin,
                                           const nsRect* aRange,
                                           nsIScrollbarMediator::ScrollSnapMode aSnap)
 {
+  if (aOrigin != nsGkAtoms::restore) {
+    // If we're doing a non-restore scroll, we don't want to later
+    // override it by restoring our saved scroll position.
+    mRestorePos.x = mRestorePos.y = -1;
+  }
 
   if (aSnap == nsIScrollableFrame::ENABLE_SNAP) {
     GetSnapPointForDestination(nsIScrollableFrame::DEVICE_PIXELS,
                                mDestination,
                                aScrollPosition);
   }
 
   nsRect scrollRange = GetScrollRangeForClamping();
@@ -4311,16 +4316,18 @@ ScrollFrameHelper::ScrollToRestoredPosit
       }
       nsPoint scrollToPos = mRestorePos;
       if (!IsPhysicalLTR()) {
         // convert from logical to physical scroll position
         scrollToPos.x = mScrollPort.x -
           (mScrollPort.XMost() - scrollToPos.x - mScrolledFrame->GetRect().width);
       }
       AutoWeakFrame weakFrame(mOuter);
+      // It's very important to pass nsGkAtoms::restore here, so
+      // ScrollToWithOrigin won't clear out mRestorePos.
       ScrollToWithOrigin(scrollToPos, nsIScrollableFrame::INSTANT,
                          nsGkAtoms::restore, nullptr);
       if (!weakFrame.IsAlive()) {
         return;
       }
       if (state == LoadingState::Loading || NS_SUBTREE_DIRTY(mOuter)) {
         // If we're trying to do a history scroll restore, then we want to
         // keep trying this until we succeed, because the page can be loading
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -154280,16 +154280,28 @@
       [
        "/css/css3-selectors/selectors-namespace-001-ref.xml",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/cssom-view/scrollTop-display-change.html": [
+    [
+     "/css/cssom-view/scrollTop-display-change.html",
+     [
+      [
+       "/css/cssom-view/scrollTop-display-change-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/filter-effects-1/css-filters-animation-blur.html": [
     [
      "/css/filter-effects-1/css-filters-animation-blur.html",
      [
       [
        "/css/filter-effects-1/css-filters-animation-blur-ref.html",
        "=="
       ]
@@ -254389,16 +254401,21 @@
      {}
     ]
    ],
    "css/css3-selectors/xml-shell.css": [
     [
      {}
     ]
    ],
+   "css/cssom-view/scrollTop-display-change-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/filter-effects-1/OWNERS": [
     [
      {}
     ]
    ],
    "css/filter-effects-1/css-filters-animation-blur-ref.html": [
     [
      {}
@@ -523041,16 +523058,24 @@
   "css/css3-selectors/xml-full.css": [
    "d0f56b84aec227b392ee45085908515d718e85a0",
    "support"
   ],
   "css/css3-selectors/xml-shell.css": [
    "d0f56b84aec227b392ee45085908515d718e85a0",
    "support"
   ],
+  "css/cssom-view/scrollTop-display-change-ref.html": [
+   "bb9079ba597cbcc27604cf8cc5556b4e6e0cda93",
+   "support"
+  ],
+  "css/cssom-view/scrollTop-display-change.html": [
+   "68d33e9669be4db95ea9016a8893212e588189fd",
+   "reftest"
+  ],
   "css/filter-effects-1/OWNERS": [
    "c6caf7a048a7601b044f8d0b2e61a2f0c2dbba4e",
    "support"
   ],
   "css/filter-effects-1/css-filters-animation-blur-ref.html": [
    "e25b4e0eb289af2bab7a396b63ae374d124b42c5",
    "support"
   ],
@@ -578562,17 +578587,17 @@
    "72e4fc91dac6bdd42437b5034d1692a208a90de9",
    "testharness"
   ],
   "service-workers/service-worker/navigation-redirect-to-http.https.html": [
    "12f109101a7d6fcb52ab070077297443fa7ab3eb",
    "testharness"
   ],
   "service-workers/service-worker/navigation-redirect.https.html": [
-   "f8e79356467abba33e8008054c32baabc770fe65",
+   "109f463deeaad2d60d4dab644c782ad633e97a7d",
    "testharness"
   ],
   "service-workers/service-worker/onactivate-script-error.https.html": [
    "bfef14af67c3a21523b5a7283d7cf86ac288f081",
    "testharness"
   ],
   "service-workers/service-worker/oninstall-script-error.https.html": [
    "0497bf37f0e3b55a6a4745cae2ec700b6f963fd3",
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom-view/scrollTop-display-change-ref.html
@@ -0,0 +1,8 @@
+<!doctype html>
+<meta charset=utf-8>
+<div id="scroller" style="height: 100px; overflow: scroll">
+  <div style="height: 1000px">
+    I should be visible.
+  </div>
+  I should not be visible.
+</div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom-view/scrollTop-display-change.html
@@ -0,0 +1,17 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Setting scrollTop to 0 immediately after toggling display from "none" on an element that had nonzero scrollTop before should work.</title>
+<link rel=match href="scrollTop-display-change-ref.html">
+<div id="scroller" style="height: 100px; overflow: scroll">
+  <div style="height: 1000px">
+    I should be visible.
+  </div>
+  I should not be visible.
+</div>
+<script>
+  scroller.scrollTop = 1000;
+  scroller.style.display = "none";
+  var win = scroller.scrollTop; // Force layout flush
+  scroller.style.display = "";
+  scroller.scrollTop = 0;
+</script>