Bug 1247074 - When a compositor-based smooth scroll animation is in progress and the scrollframe is reconstructed, restore to the animation destination. r=tnikkel
authorKartikaya Gupta <kgupta@mozilla.com>
Mon, 29 Aug 2016 20:28:40 -0400
changeset 312213 f478daa490df29fd134b42a81a77a11f29f4a6aa
parent 312212 03204f6c0a7a48b27187e4207e733e2abae8d538
child 312214 737088027e6823ba0206d83735a185f80a89bd77
push id31926
push userkgupta@mozilla.com
push dateThu, 01 Sep 2016 15:12:52 +0000
treeherderautoland@f478daa490df [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1247074
milestone51.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 1247074 - When a compositor-based smooth scroll animation is in progress and the scrollframe is reconstructed, restore to the animation destination. r=tnikkel MozReview-Commit-ID: 73juHWNfoQy
layout/generic/nsGfxScrollFrame.cpp
layout/generic/test/mochitest.ini
layout/generic/test/test_scroll_animation_restore.html
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -5887,28 +5887,36 @@ ScrollFrameHelper::SaveState() const
 {
   nsIScrollbarMediator* mediator = do_QueryFrame(GetScrolledFrame());
   if (mediator) {
     // child handles its own scroll state, so don't bother saving state here
     return nullptr;
   }
 
   // Don't store a scroll state if we never have been scrolled or restored
-  // a previous scroll state.
-  if (!mHasBeenScrolled && !mDidHistoryRestore) {
+  // a previous scroll state, and we're not in the middle of a smooth scroll.
+  bool isInSmoothScroll = IsProcessingAsyncScroll() || mLastSmoothScrollOrigin;
+  if (!mHasBeenScrolled && !mDidHistoryRestore && !isInSmoothScroll) {
     return nullptr;
   }
 
   nsPresState* state = new nsPresState();
   // Save mRestorePos instead of our actual current scroll position, if it's
   // valid and we haven't moved since the last update of mLastPos (same check
   // that ScrollToRestoredPosition uses). This ensures if a reframe occurs
   // while we're in the process of loading content to scroll to a restored
-  // position, we'll keep trying after the reframe.
+  // position, we'll keep trying after the reframe. Similarly, if we're in the
+  // middle of a smooth scroll, store the destination so that when we restore
+  // we'll jump straight to the end of the scroll animation, rather than
+  // effectively dropping it. Note that the mRestorePos will override the
+  // smooth scroll destination if both are present.
   nsPoint pt = GetLogicalScrollPosition();
+  if (isInSmoothScroll) {
+    pt = mDestination;
+  }
   if (mRestorePos.y != -1 && pt == mLastPos) {
     pt = mRestorePos;
   }
   state->SetScrollState(pt);
   if (mIsRoot) {
     // Only save resolution properties for root scroll frames
     nsIPresShell* shell = mOuter->PresContext()->PresShell();
     state->SetResolution(shell->GetResolution());
--- a/layout/generic/test/mochitest.ini
+++ b/layout/generic/test/mochitest.ini
@@ -141,8 +141,9 @@ support-files = selection_expanding_xbl.
 [test_selection_preventDefault.html]
 skip-if = buildapp == 'mulet'
 [test_selection_splitText-normalize.html]
 [test_selection_touchevents.html]
 [test_taintedfilters.html]
 support-files = file_taintedfilters_feDisplacementMap-tainted-1.svg file_taintedfilters_feDisplacementMap-tainted-2.svg file_taintedfilters_feDisplacementMap-tainted-3.svg file_taintedfilters_feDisplacementMap-tainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-1.svg file_taintedfilters_feDisplacementMap-untainted-2.svg file_taintedfilters_red-flood-for-feImage-cors.svg file_taintedfilters_red-flood-for-feImage-cors.svg^headers^ file_taintedfilters_red-flood-for-feImage.svg
 [test_scroll_position_restore.html]
 support-files = file_scroll_position_restore.html
+[test_scroll_animation_restore.html]
new file mode 100644
--- /dev/null
+++ b/layout/generic/test/test_scroll_animation_restore.html
@@ -0,0 +1,128 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1247074
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1247074</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/paint_listener.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <style>
+  .outer {
+      direction: ltr;
+      height: 400px;
+      width: 415px;
+      overflow: hidden;
+      position: relative;
+  }
+  .inner {
+      height: 100%;
+      outline: none;
+      overflow-x: hidden;
+      overflow-y: scroll;
+      position: relative;
+      scroll-behavior: smooth;
+  }
+  .outer.contentBefore::before {
+      top: 0;
+      content: '';
+      display: block;
+      height: 2px;
+      position: absolute;
+      width: 100%;
+      z-index: 99;
+  }
+  </style>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1247074">Mozilla Bug 1247074</a>
+<p id="display"></p>
+<div class="outer">
+  <div class="inner">
+   <ol>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+    <li>Some text</li>
+   </ol>
+  </div>
+</div>
+<script>
+SimpleTest.waitForExplicitFinish();
+window.onload = function() {
+  var elm = document.getElementsByClassName('inner')[0];
+
+  // Take control of the refresh driver
+  var utils = SpecialPowers.DOMWindowUtils;
+  utils.advanceTimeAndRefresh(0);
+
+  // Start a smooth scroll and advance a couple of frames so we're in the
+  // middle of the scroll animation
+  elm.scrollTop = 500;
+  utils.advanceTimeAndRefresh(16);
+  utils.advanceTimeAndRefresh(16);
+
+  // Trigger a frame reconstruction
+  elm.parentNode.classList.add('contentBefore');
+
+  // Reach a stable state and verify the scroll position is 500
+  utils.restoreNormalRefresh();
+  waitForAllPaintsFlushed(function() {
+    SimpleTest.is(elm.scrollTop, 500, "Scroll position ended up at 500");
+    SimpleTest.finish();
+  });
+}
+
+</script>
+</body>
+</html>