Bug 1536157 - Make sure we repaint after accepting a visual scroll update. r=kats a=pascalc
authorBotond Ballo <botond@mozilla.com>
Fri, 22 Mar 2019 05:20:31 +0000
changeset 525825 f36792ee434476ad27f215563b2fda30f6b7298b
parent 525824 e0b94caff888c14d80461bfa414a90ac11515184
child 525826 2a43c0f29c287054b941988970db92a64e5519be
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats, pascalc
bugs1536157
milestone67.0
Bug 1536157 - Make sure we repaint after accepting a visual scroll update. r=kats a=pascalc A possible alternative would be to have the main thread already paint a displayport at the target position of a requested visual update as part of the same transaction that requests the update. There are a couple of reasons we may not want to do that: 1) APZ could reject the requested visual update under certain conditions, e.g. if there is a higher-priority layout update. 2) It would break the property that the displayport in the main thread's scroll metadata is relative to the scroll offset in said metadata. Various places assume this and untangling that seems tricky. This does mean that if the main thread requests a visual update to "far away" (outside the existing displayport), we can get temporary checkerboarding before the content at the target position is painted. However, it's straightforward for callers to work around that, by changing the layout scroll offset _and_ scheduling a visual update if they wish to visual-scroll far away. Differential Revision: https://phabricator.services.mozilla.com/D23902
gfx/layers/RepaintRequest.h
gfx/layers/apz/src/AsyncPanZoomController.cpp
layout/base/nsIPresShell.h
--- a/gfx/layers/RepaintRequest.h
+++ b/gfx/layers/RepaintRequest.h
@@ -27,18 +27,22 @@ namespace layers {
 
 struct RepaintRequest {
   friend struct IPC::ParamTraits<mozilla::layers::RepaintRequest>;
 
  public:
   // clang-format off
   MOZ_DEFINE_ENUM_WITH_BASE_AT_CLASS_SCOPE(
     ScrollOffsetUpdateType, uint8_t, (
-      eNone,             // The default; the scroll offset was not updated.
-      eUserAction        // The scroll offset was updated by APZ.
+        eNone,             // The default; the scroll offset was not updated.
+        eUserAction,       // The scroll offset was updated by APZ in response
+                           // to user action.
+        eVisualUpdate      // The scroll offset was updated by APZ in response
+                           // to a visual scroll update request from the
+                           // main thread.
   ));
   // clang-format on
 
   RepaintRequest()
       : mScrollId(ScrollableLayerGuid::NULL_SCROLL_ID),
         mPresShellResolution(1),
         mCompositionBounds(0, 0, 0, 0),
         mCumulativeResolution(),
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -4635,17 +4635,27 @@ void AsyncPanZoomController::NotifyLayer
     // |if (scrollOffsetUpdated)| branch above.
     Metrics().RecalculateLayoutViewportOffset();
     mCompositedLayoutViewport = Metrics().GetLayoutViewport();
     mCompositedScrollOffset = Metrics().GetScrollOffset();
     mExpectedGeckoMetrics = aLayerMetrics;
     if (!mAnimation || !mAnimation->HandleScrollOffsetUpdate(Nothing())) {
       CancelAnimation();
     }
+    // The main thread did not actually paint a displayport at the target
+    // visual offset, so we need to ask it to repaint. We need to set the
+    // contentRepaintType to something other than eNone, otherwise the main
+    // thread will short-circuit the repaint request.
+    // Don't do this for eRestore visual updates as a repaint coming from APZ
+    // breaks the scroll offset restoration mechanism.
     needContentRepaint = true;
+    if (aLayerMetrics.GetVisualScrollUpdateType() ==
+        FrameMetrics::eMainThread) {
+      contentRepaintType = RepaintUpdateType::eVisualUpdate;
+    }
     ScheduleComposite();
   }
 
   if (viewportUpdated) {
     // While we want to accept the main thread's layout viewport _size_,
     // its position may be out of date in light of async scrolling, to
     // adjust it if necessary to make sure it continues to enclose the
     // visual viewport.
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -1684,16 +1684,20 @@ class nsIPresShell : public nsStubDocume
     FrameMetrics::ScrollOffsetUpdateType mUpdateType;
   };
 
   // Ask APZ in the next transaction to scroll to the given visual viewport
   // offset (relative to the document).
   // Use this sparingly, as it will clobber JS-driven scrolling that happens
   // in the same frame. This is mostly intended to be used in special
   // situations like "first paint" or session restore.
+  // If scrolling "far away", i.e. not just within the existing layout
+  // viewport, it's recommended to use both nsIScrollableFrame.ScrollTo*()
+  // (via window.scrollTo if calling from JS) *and* this function; otherwise,
+  // temporary checkerboarding may result.
   // Please request APZ review if adding a new call site.
   void SetPendingVisualScrollUpdate(
       const nsPoint& aVisualViewportOffset,
       FrameMetrics::ScrollOffsetUpdateType aUpdateType);
   void ClearPendingVisualScrollUpdate() {
     mPendingVisualScrollUpdate = mozilla::Nothing();
   }
   const mozilla::Maybe<VisualScrollUpdate>& GetPendingVisualScrollUpdate()