NTP: Prevent conflict of dismiss animations and scroll position handling.

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
index 7eb2a27ed875284272da9bf2893a06450383fa65..9e002bfc030abfdc8a65ba22b06c5177e92e91e1 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
@@ -58,7 +58,7 @@ import org.chromium.ui.base.DeviceFormFactor;
 public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
     private static final String TAG = "NewTabPageView";
 
-    private static final long SNAP_SCROLL_DELAY_MS = 30;
+    private static final long SCROLL_RUNNABLE_DELAY_MS = 30;
 
     /**
      * Experiment parameter for the maximum number of tile suggestion rows to show.
@@ -101,6 +101,7 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
     private TileGroup mTileGroup;
     private UiConfig mUiConfig;
     private Runnable mSnapScrollRunnable;
+    private Runnable mUpdateSearchBoxOnScrollRunnable;
     private boolean mFirstShow = true;
     private boolean mSearchProviderHasLogo = true;
     private boolean mPendingSnapScroll;
@@ -196,12 +197,35 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
 
         mRecyclerView.setItemAnimator(new DefaultItemAnimator() {
             @Override
+            public boolean animateMove(ViewHolder holder, int fromX, int fromY, int toX, int toY) {
+                // If |mNewTabPageLayout| is animated by the RecyclerView because an item below it

[Bernhard Bauer, 2017/03/27 17:36:54]

Stupid question, why doesn't it work to call updateSearchBoxOnScroll in here if
the animated item is the NewTabPageLayout?

[Michael van Ouwerkerk, 2017/03/28 10:44:15]

Because this method (animateMove) is called only once for each item than needs
to be moved, in order to schedule its animation. There are no intermediate
updates that would allow us to respond with causing jank, and we only see the
end scroll state here.

+                // was dismissed, avoid also manipulating its vertical offset in our scroll handling
+                // at the same time. The onScrolled() method is called when an item is dismissed and
+                // the item at the top of the viewport is repositioned.
+                if (holder.itemView == mNewTabPageLayout) setUrlFocusAnimationsDisabled(true);
+
+                // Cancel any pending scroll update handling, a new one will be scheduled in
+                // onAnimationFinished().
+                mRecyclerView.removeCallbacks(mUpdateSearchBoxOnScrollRunnable);
+
+                return super.animateMove(holder, fromX, fromY, toX, toY);
+            }
+
+            @Override
             public void onAnimationFinished(ViewHolder viewHolder) {
                 super.onAnimationFinished(viewHolder);
-                // When removing sections, because the animations are all translations, the
-                // scroll events don't fire and we can get in the situation where the toolbar
-                // buttons disappear.
-                updateSearchBoxOnScroll();
+
+                // When an item is dismissed, the items at the top of the viewport might not move,
+                // and onScrolled() might not be called. We can get in the situation where the
+                // toolbar buttons disappear, so schedule an update for it. This can be cancelled
+                // from animateMove() in case |mNewTabPageLayout| will be moved. We don't know that
+                // from here, as the RecyclerView will animate multiple items when one is dismissed,
+                // and some will "finish" synchronously if they are already in the correct place,
+                // before other moves have even been scheduled.
+                if (viewHolder.itemView == mNewTabPageLayout) setUrlFocusAnimationsDisabled(false);
+                mRecyclerView.removeCallbacks(mUpdateSearchBoxOnScrollRunnable);
+                mRecyclerView.postDelayed(
+                        mUpdateSearchBoxOnScrollRunnable, SCROLL_RUNNABLE_DELAY_MS);

[Bernhard Bauer, 2017/03/27 17:36:54]

Is there a reason to use that particular delay (which is for snap scrolling)?
Would 0 work?

[Michael van Ouwerkerk, 2017/03/28 10:44:15]

As it turns out, there's no need to use postDelayed() here, simply post() is
sufficient.

             }
         });
 
@@ -231,6 +255,7 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
         mNoSearchLogoSpacer = mNewTabPageLayout.findViewById(R.id.no_search_logo_spacer);
 
         mSnapScrollRunnable = new SnapScrollRunnable();
+        mUpdateSearchBoxOnScrollRunnable = new UpdateSearchBoxOnScrollRunnable();
 
         initializeSearchBoxTextView();
         initializeVoiceSearchButton();
@@ -494,7 +519,7 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
                 if (event.getActionMasked() == MotionEvent.ACTION_CANCEL
                         || event.getActionMasked() == MotionEvent.ACTION_UP) {
                     mPendingSnapScroll = true;
-                    mRecyclerView.postDelayed(mSnapScrollRunnable, SNAP_SCROLL_DELAY_MS);
+                    mRecyclerView.postDelayed(mSnapScrollRunnable, SCROLL_RUNNABLE_DELAY_MS);
                 } else {
                     mPendingSnapScroll = false;
                 }
@@ -507,7 +532,7 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
     private void handleScroll() {
         if (mPendingSnapScroll) {
             mRecyclerView.removeCallbacks(mSnapScrollRunnable);
-            mRecyclerView.postDelayed(mSnapScrollRunnable, SNAP_SCROLL_DELAY_MS);
+            mRecyclerView.postDelayed(mSnapScrollRunnable, SCROLL_RUNNABLE_DELAY_MS);
         }
         updateSearchBoxOnScroll();
         mRecyclerView.updatePeekingCardAndHeader();
@@ -935,4 +960,11 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
             mRecyclerView.snapScroll(mSearchBoxView, getHeight());
         }
     }
+
+    private class UpdateSearchBoxOnScrollRunnable implements Runnable {
+        @Override
+        public void run() {
+            updateSearchBoxOnScroll();
+        }
+    }
 }