Bug 1313144 - Fix tabs panel animator issues. r?sebastian draft
authorTom Klein <twointofive@gmail.com>
Tue, 25 Oct 2016 14:00:00 -0500
changeset 431529 bd72b26ebbe63e59db246efef64107e7d235de49
parent 429179 c6ccd71126ff514bfc44b53e2217562e29a0cc38
child 535420 9c4e6a22bdd06a41541af963ccde2e497cc09b94
push id34059
push userbmo:twointofive@gmail.com
push dateSun, 30 Oct 2016 00:20:05 +0000
reviewerssebastian
bugs1313144
milestone52.0a1
Bug 1313144 - Fix tabs panel animator issues. r?sebastian The tabs list issues fixed here are dependent on timing. If we add a tab A and the currently selected tab B gets moved as a result, and if the UNSELECT on B arrives after the move animation on B has started, then the following steps occur: * the bind on B which occurs as a result of the UNSELECT resets translationY to zero; * the animateChange for the UNSELECT runs with fromX=toX, fromY=toY and gets (as a matter of course with setSupportsChangeAnimations(false)) changed to an animateMove; * the animateMove stores the current translationY, which is now 0, and then as a matter of course runs endAnimation to end the currently running animation; * the animateMove doesn't see any moves underway, and didn't receive any new move information, so it returns without starting a new move animation. If the UNSELECT arrives before the move is queued, which is what usually happens, then the animation runs normally. The fix here is to reset properties in RecyclerView.onChildAttachedToWindow instead of in adapter.onBindViewHolder. The other fix is to DefaultItemAnimator's animateChange, which is just generally not well behaved in the case where setSupportsChangeAnimations(false) is set and multiple animations on a view can occur at the same time. For example, if tab A is added and then the SELECT for tab A arrives, the animateChange call resulting from the SELECT's notifyItemChanged will be rerouted to an animateMove, which cancels the existing add animation without taking it over (animateMove only checks for already running translation* animations, not alpha animations as in an add). The add animation cancel causes the added tab to appear immediately instead of waiting to appear after other tabs have moved to make room for it. If the SELECT arrives before the animateAdd for the add is called, which is what usually happens, then the animation runs normally. More generally the provided change to animateChange prevents the unneeded passing off of existing running move animations into new pending animations when the animateChange call started off with no new move data. MozReview-Commit-ID: HowKhVUzqhy
mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutRecyclerAdapter.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java
mobile/android/base/java/org/mozilla/gecko/widget/NoChangeItemAnimator.java
mobile/android/base/moz.build
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutRecyclerAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutRecyclerAdapter.java
@@ -103,20 +103,19 @@ public class TabsLayoutRecyclerAdapter
         return tabs.get(position);
     }
 
     @Override
     public void onBindViewHolder(TabsListViewHolder viewHolder, int position) {
         final Tab tab = getItem(position);
         final TabsLayoutItemView itemView = (TabsLayoutItemView) viewHolder.itemView;
         itemView.assignValues(tab);
-        // Make sure we didn't miss any resets after animations and swipes:
-        itemView.setAlpha(1);
-        itemView.setTranslationX(0);
-        itemView.setTranslationY(0);
+        // Be careful (re)setting position values here: bind is called on each notifyItemChanged,
+        // so you could be stomping on values that have been set in support of other animations
+        // that are already underway.
     }
 
     @Override
     public TabsListViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
         final TabsLayoutItemView viewItem = (TabsLayoutItemView) inflater.inflate(tabLayoutId, parent, false);
         viewItem.setPrivateMode(isPrivate);
         viewItem.setCloseOnClickListener(closeOnClickListener);
 
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
@@ -31,22 +31,17 @@ public class TabsListLayout extends Tabs
 
         setLayoutManager(new LinearLayoutManager(context));
 
         // A TouchHelper handler for swipe to close.
         final TabsTouchHelperCallback callback = new TabsTouchHelperCallback(this);
         final ItemTouchHelper touchHelper = new ItemTouchHelper(callback);
         touchHelper.attachToRecyclerView(this);
 
-        final TabsListLayoutAnimator animator = new TabsListLayoutAnimator();
-        animator.setRemoveDuration(ANIMATION_DURATION);
-        // A fade in/out each time the title/thumbnail/etc. gets updated isn't helpful, so disable
-        // the change animation.
-        animator.setSupportsChangeAnimations(false);
-        setItemAnimator(animator);
+        setItemAnimator(new TabsListLayoutAnimator(ANIMATION_DURATION));
     }
 
     @Override
     public void closeAll() {
         final int childCount = getChildCount();
 
         // Just close the panel if there are no tabs to close.
         if (childCount == 0) {
@@ -106,9 +101,17 @@ public class TabsListLayout extends Tabs
             cascadeDelay += ANIMATION_CASCADE_DELAY;
         }
     }
 
     @Override
     protected boolean addAtIndexRequiresScroll(int index) {
         return index == 0 || index == getAdapter().getItemCount() - 1;
     }
+
+    @Override
+    public void onChildAttachedToWindow(View child) {
+        // Make sure we reset any attributes that may have been animated in this child's previous
+        // incarnation.
+        child.setTranslationX(0);
+        child.setAlpha(1);
+    }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java
@@ -1,34 +1,38 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.tabs;
 
 import org.mozilla.gecko.util.ThreadUtils;
+import org.mozilla.gecko.widget.NoChangeItemAnimator;
 
 import android.animation.Animator;
 import android.animation.AnimatorListenerAdapter;
 import android.support.v4.animation.AnimatorCompatHelper;
-import android.support.v7.widget.DefaultItemAnimator;
 import android.support.v7.widget.RecyclerView;
 import android.view.View;
 import android.view.ViewPropertyAnimator;
 
 import java.util.ArrayList;
 import java.util.List;
 
 // This is a light rewrite of DefaultItemAnimator to support a non-default remove animation.
-class TabsListLayoutAnimator extends DefaultItemAnimator {
+class TabsListLayoutAnimator extends NoChangeItemAnimator {
     private List<RecyclerView.ViewHolder> pendingRemovals = new ArrayList<>();
     // ViewHolders on which remove animations are currently being run.
     private List<RecyclerView.ViewHolder> removeAnimations = new ArrayList<>();
 
+    public TabsListLayoutAnimator(int removeDuration) {
+        setRemoveDuration(removeDuration);
+    }
+
     @Override
     public void runPendingAnimations() {
         if (pendingRemovals.isEmpty()) {
             super.runPendingAnimations();
             return;
         }
 
         for (RecyclerView.ViewHolder holder : pendingRemovals) {
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/java/org/mozilla/gecko/widget/NoChangeItemAnimator.java
@@ -0,0 +1,43 @@
+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.widget;
+
+import android.support.v7.widget.DefaultItemAnimator;
+import android.support.v7.widget.RecyclerView;
+
+// In spite of what the (self-contradictory) Android documentation says, animateChange can be called
+// in response to an adapter.notifyItemChanged call even when setSupportsChangeAnimations(false) has
+// been set. Internally what happens in that case is that the animateChange call is rerouted to an
+// animateMove call. Such calls must be handled when a move is required, but if the original change
+// call data doesn't indicate a move is required, then turning it into a move can have unwanted side
+// effects:
+//   * even if the animateMove successfully takes over a move animation already underway, it can
+//     still cause a visible animation delay since the new move animation doesn't start running
+//     again until after the next runPendingAnimations call.
+//   * DefaultItemAnimator.animateMove (as written) calls resetAnimation on every call, no matter
+//     what, so an already scheduled animation can be dropped if it, for example, doesn't animate a
+//     translation value.
+//
+// This class more closely implements the idea that setSupportsChangeAnimations(false) should mean
+// that calling adapter.notifyItemChanged has no impact on animations.
+public class NoChangeItemAnimator extends DefaultItemAnimator {
+
+    public NoChangeItemAnimator() {
+        setSupportsChangeAnimations(false);
+    }
+
+    @Override
+    public boolean animateChange(RecyclerView.ViewHolder oldHolder, RecyclerView.ViewHolder newHolder,
+                                 int fromX, int fromY, int toX, int toY) {
+        if (oldHolder == newHolder && fromX == toX && fromY == toY) {
+            // We're not supporting change animations, and this call provides no new move
+            // information, so cancel it to avoid side effects.
+            dispatchChangeFinished(oldHolder, true);
+            return false;
+        }
+        return super.animateChange(oldHolder, newHolder, fromX, fromY, toX, toY);
+    }
+}
--- a/mobile/android/base/moz.build
+++ b/mobile/android/base/moz.build
@@ -747,16 +747,17 @@ gbjar.sources += ['java/org/mozilla/geck
     'widget/FaviconView.java',
     'widget/FilledCardView.java',
     'widget/FlowLayout.java',
     'widget/GeckoActionProvider.java',
     'widget/GeckoPopupMenu.java',
     'widget/HistoryDividerItemDecoration.java',
     'widget/IconTabWidget.java',
     'widget/LoginDoorHanger.java',
+    'widget/NoChangeItemAnimator.java',
     'widget/RecyclerViewClickSupport.java',
     'widget/ResizablePathDrawable.java',
     'widget/RoundedCornerLayout.java',
     'widget/SiteLogins.java',
     'widget/SquaredImageView.java',
     'widget/SquaredRelativeLayout.java',
     'widget/SwipeDismissListViewTouchListener.java',
     'widget/TabThumbnailWrapper.java',