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
--- 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',