Animating TwoWayView has to account for view recycling. Some investigation is needed here.

Comment on attachment 8513620 [details] [diff] [review] Animate tab strip when tabs added added or removed (r=mcomella) Feels a lot better. We might want to tweak the animation after it lands but this is a pretty good start.

FYI: I noticed the animations are not as smooth as they should be. This is likely related to the expensive drawing operations done in each tab item in the strip. Filed bug 1091519.

Comment on attachment 8513620 [details] [diff] [review] Animate tab strip when tabs added added or removed (r=mcomella) Review of attachment 8513620 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits ::: mobile/android/base/tabs/TabStripView.java @@ +91,4 @@ > } > } > > + private void animateTabRemoval(Tab removedTab) { nit: I'd like this and animateNewTab to have the same naming convention, e.g. animateRemoveTab. @@ +93,5 @@ > > + private void animateTabRemoval(Tab removedTab) { > + final int removedPosition = adapter.getPositionForTab(removedTab); > + > + final View removedView = getViewForTab(removedTab); Add a comment for when this might be null. @@ +110,5 @@ > + final List<Animator> childAnimators = new ArrayList<Animator>(); > + > + final int childCount = getChildCount(); > + for (int i = removedPosition - firstPosition; i < childCount; i++) { > + final View child = getChildAt(i); You should make a comment here, or at the top of the method, that mentions that the removed view is not actually animated, i.e. just disappears. @@ +149,5 @@ > + if (newChild == null) { > + return true; > + } > + > + final List<Animator> childAnimators = new ArrayList<Animator>(); nit: Move this above where it is first used. @@ +152,5 @@ > + > + final List<Animator> childAnimators = new ArrayList<Animator>(); > + > + final int tabSize = newChild.getWidth(); > + final int newIndex = newPosition - firstPosition; nit: Move above the loop where it is first used. @@ +159,5 @@ > + ObjectAnimator.ofFloat(newChild, "translationY", newChild.getHeight(), 0)); > + > + final int childCount = getChildCount(); > + for (int i = newIndex + 1; i < childCount; i++) { > + final View child = getChildAt(i); I'd add a comment here, or at the top of the method, mentioning that a view that gets pushed off-screen will not get animated back here due to a limitation in android. @@ +161,5 @@ > + final int childCount = getChildCount(); > + for (int i = newIndex + 1; i < childCount; i++) { > + final View child = getChildAt(i); > + > + child.setLayerType(View.LAYER_TYPE_HARDWARE, null); Why don't we setLayoutType on newChild as well?

(In reply to Michael Comella (:mcomella) from comment #4) > Comment on attachment 8513620 [details] [diff] [review] > Animate tab strip when tabs added added or removed (r=mcomella) > > Review of attachment 8513620 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ w/ nits > > ::: mobile/android/base/tabs/TabStripView.java > @@ +91,4 @@ > > } > > } > > > > + private void animateTabRemoval(Tab removedTab) { > > nit: I'd like this and animateNewTab to have the same naming convention, > e.g. animateRemoveTab. Done. > @@ +93,5 @@ > > > > + private void animateTabRemoval(Tab removedTab) { > > + final int removedPosition = adapter.getPositionForTab(removedTab); > > + > > + final View removedView = getViewForTab(removedTab); > > Add a comment for when this might be null. Done. > @@ +110,5 @@ > > + final List<Animator> childAnimators = new ArrayList<Animator>(); > > + > > + final int childCount = getChildCount(); > > + for (int i = removedPosition - firstPosition; i < childCount; i++) { > > + final View child = getChildAt(i); > > You should make a comment here, or at the top of the method, that mentions > that the removed view is not actually animated, i.e. just disappears. Good point, done. > @@ +149,5 @@ > > + if (newChild == null) { > > + return true; > > + } > > + > > + final List<Animator> childAnimators = new ArrayList<Animator>(); > > nit: Move this above where it is first used. Ok, done. > @@ +152,5 @@ > > + > > + final List<Animator> childAnimators = new ArrayList<Animator>(); > > + > > + final int tabSize = newChild.getWidth(); > > + final int newIndex = newPosition - firstPosition; > > nit: Move above the loop where it is first used. Done. > @@ +159,5 @@ > > + ObjectAnimator.ofFloat(newChild, "translationY", newChild.getHeight(), 0)); > > + > > + final int childCount = getChildCount(); > > + for (int i = newIndex + 1; i < childCount; i++) { > > + final View child = getChildAt(i); > > I'd add a comment here, or at the top of the method, mentioning that a view > that gets pushed off-screen will not get animated back here due to a > limitation in android. Ok, done. > @@ +161,5 @@ > > + final int childCount = getChildCount(); > > + for (int i = newIndex + 1; i < childCount; i++) { > > + final View child = getChildAt(i); > > + > > + child.setLayerType(View.LAYER_TYPE_HARDWARE, null); > > Why don't we setLayoutType on newChild as well? Good catch. I should have removed this line from the patch. I'll probably add it back in a different form as part of bug 1091519.