Adding animations for view pager transitions

chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
index dd76948cbbe1e9b0f01ef264b72dd508555bea10..67ba19d1c6024e6d032da8f48bd5b9b002416f2a 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
@@ -4,11 +4,19 @@
 
 package org.chromium.chrome.browser.contextmenu;
 
+import android.animation.ValueAnimator;
+import android.animation.ValueAnimator.AnimatorUpdateListener;
+import android.annotation.SuppressLint;
 import android.content.Context;
+import android.graphics.Canvas;
+import android.graphics.Rect;
+import android.graphics.drawable.Drawable;
 import android.support.v4.view.ViewPager;
+import android.support.v4.view.animation.LinearOutSlowInInterpolator;
 import android.util.AttributeSet;
 import android.view.View;
 
+import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.chrome.R;
 
 /**
@@ -16,6 +24,17 @@ import org.chromium.chrome.R;
  * pager.
  */
 public class TabularContextMenuViewPager extends ViewPager {
+    private ValueAnimator mAnimator;
+    private int mOldHeight;
+    private int mCanvasWidth;
+    private int mClipHeight;
+
+    // Animation value hits the limit twice if the animation duration is long enough (by developer
+    // options); this boolean helps to ensure that it only happens once.
+    private boolean mAtMaxAnimationValue;
+
+    private int mDifferenceInHeight = 0;
+    private int mPreviousChildIndex = 1;
     private final int mContextMenuMinimumPaddingPx =
             getResources().getDimensionPixelSize(R.dimen.context_menu_min_padding);
 
@@ -33,11 +52,8 @@ public class TabularContextMenuViewPager extends ViewPager {
      */
     @Override
     protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
-        int menuHeight = 0;
         int tabHeight = 0;
-
-        // getCurrentItem() does not take into account the tab layout unlike getChildCount().
-        int currentItemsIndex = getCurrentItem() + 1;
+        int menuHeight = 0;
 
         // The width of the context menu is defined so that it leaves space between itself and the
         // screen's edges. It is also bounded to a max size to prevent the menu from stretching
@@ -48,34 +64,109 @@ public class TabularContextMenuViewPager extends ViewPager {
 
         widthMeasureSpec = MeasureSpec.makeMeasureSpec(contextMenuWidth, MeasureSpec.EXACTLY);
 
-        // The height of the context menu is calculated as the sum of:
-        // 1. The tab bar's height, which is only visible when the context menu requires it
-        //    (i.e. an ImageLink is clicked)
-        // 2. The height of the View being displayed for the current tab.
-        for (int i = 0; i < getChildCount(); i++) {
-            View child = getChildAt(i);
-
-            child.measure(
-                    widthMeasureSpec, MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED));
-            int measuredHeight = child.getMeasuredHeight();
-
-            // The ViewPager also considers the tab layout one of its children, and needs to be
-            // treated separately from getting the largest height.
-            if (child.getId() == R.id.tab_layout && child.getVisibility() != GONE) {
-                tabHeight = measuredHeight;
-            } else if (i == currentItemsIndex) {
-                menuHeight = child.getMeasuredHeight();
-                break;
+        // getCurrentItem() does not take into account the tab layout unlike getChildCount().
+        int currentChildIndex = getCurrentItem() + 1;
+
+        // Handles the case where this is called while the pager is scrolling between views.
+        if (getScrollX() != 0 && getScrollX() != mCanvasWidth) {
+            heightMeasureSpec = MeasureSpec.makeMeasureSpec(mOldHeight, MeasureSpec.EXACTLY);
+        } else {
+            // The height of the context menu is calculated as the sum of:
+            // 1. The tab bar's height, which is only visible when the context menu requires it
+            //    (i.e. an ImageLink is clicked)
+            // 2. The height of the View being displayed for the current tab.
+            for (int i = 0; i < getChildCount(); i++) {
+                View child = getChildAt(i);
+                child.measure(
+                        widthMeasureSpec, MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED));
+                int measuredHeight = child.getMeasuredHeight();
+
+                // The ViewPager also considers the tab layout one of its children, and needs to be
+                // treated separately from getting the largest height.
+                if (child.getId() == R.id.tab_layout && child.getVisibility() != GONE) {
+                    tabHeight = measuredHeight;
+                } else if (i == currentChildIndex) {
+                    menuHeight = child.getMeasuredHeight();
+                    break;
+                }
+            }
+            int fullHeight = menuHeight + tabHeight;
+            int deviceHeightPx = getResources().getDisplayMetrics().heightPixels;

[Theresa, 2017/07/14 16:56:38]

nit: This is technically the current available height based on the app's window
(which is what you want); the variable could use a better name.

[Daniel Park, 2017/07/14 23:15:40]

Done.

+            fullHeight = Math.min(fullHeight, deviceHeightPx - 2 * mContextMenuMinimumPaddingPx);
+            mDifferenceInHeight = fullHeight - mOldHeight;
+
+            if (currentChildIndex == mPreviousChildIndex) {
+                // Handles the case where the height of the current tab changes (i.e. an image
+                // finished loading or the link became fully visible).

[Theresa, 2017/07/14 16:56:38]

The CL description says "Note: this does not handle the image loading or link
text view enlargement animations.", but this comment appears to indicate that
they are handled?

[Daniel Park, 2017/07/14 23:15:39]

Done.

+                mClipHeight = fullHeight;
+                if (menuHeight != 0) {
+                    mOldHeight = fullHeight;

[Theresa, 2017/07/14 16:56:38]

nit: this can fit on the line above

if (menuHeight != 0) mOldHeight = fullHeight;

[Daniel Park, 2017/07/14 23:15:40]

Done.

+                }
+                heightMeasureSpec = MeasureSpec.makeMeasureSpec(fullHeight, MeasureSpec.EXACTLY);
+                // Clip logic is not called on orientation changes, so it must be set here.
+                setClipDimensionsForResize(contextMenuWidth);
+            } else {
+                // Handles the case where the view pager has completely scrolled to a different
+                // child.
+                if (mAnimator == null) {
+                    initAnimator();

[Theresa, 2017/07/14 16:56:38]

nit: this can also fit on the line above

[Daniel Park, 2017/07/14 23:15:40]

Done.

+                }
+                mAnimator.start();
+                heightMeasureSpec = MeasureSpec.makeMeasureSpec(

[Theresa, 2017/07/14 16:56:38]

Should we measure before starting the animation?

[Daniel Park, 2017/07/14 23:15:39]

Done.

+                        Math.max(mOldHeight, fullHeight), MeasureSpec.EXACTLY);
             }
         }
+        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
+        mPreviousChildIndex = currentChildIndex;
+    }
 
-        // Cap the height of the context menu so that it fits on the screen without touching the
-        // screen's edges.
-        int fullHeight = menuHeight + tabHeight;
-        int deviceHeightPx = getResources().getDisplayMetrics().heightPixels;
-        fullHeight = Math.min(fullHeight, deviceHeightPx - 2 * mContextMenuMinimumPaddingPx);
+    @SuppressLint("NewApi")

[Theresa, 2017/07/14 16:56:38]

This is a real warning. It looks like setClipBounds wasn't added until API level
18, which is JellyBean MR2. We support back to API level 16 (the first JellyBean
release). This will likely crash on JellyBean and JellyBean MR1.

To avoid a crash, you could use ViewCompat#setClipBounds(View view, Rect
clipBounds), but prior to API 18 the method does nothing so this animation will
be incorrect.

You may be able to clip the canvas in onDraw or otherwise write custom draw code
so that the clipping is handled properly on all OS versions that Chrome
supports.

[Daniel Park, 2017/07/14 23:15:39]

Acknowledged. I added a todo. I'll play around with some things, but leaving it
with ViewCompat for now.

[Ted C, 2017/07/17 21:25:31]

As you are already overwriting onDraw, you should be able to just save
mClipHeight
and do something like this inside of onDraw and get rid of setClipBounds
entirely.

boolean clipped = false;
if (mClipHeight != 0) {
    canvas.save();
    canvas.clipRect(0, 0, getMeasuredWidth(), mClipHeight);
    clipped = true;
}
super.onDraw();
if (clipped) {
    canvas.restore();
}

setClipBounds seems to be needed for when you haven't overwritten the view.

[Daniel Park, 2017/07/19 00:30:08]

Done.

+    public void setClipDimensionsForResize(int width) {
+        setClipBounds(new Rect(0, 0, width, mClipHeight));
+    }
 
-        heightMeasureSpec = MeasureSpec.makeMeasureSpec(fullHeight, MeasureSpec.EXACTLY);
-        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
+    @SuppressLint("NewApi")
+    private void initAnimator() {
+        mAnimator = ValueAnimator.ofFloat(0f, 1f);
+        mAnimator.setDuration(250);

[Theresa, 2017/07/14 16:56:38]

nit: This should be a static final int at the top of the file.

[Daniel Park, 2017/07/14 23:15:40]

Done.

+        mAnimator.setInterpolator(new LinearOutSlowInInterpolator());
+        mAnimator.addUpdateListener(new AnimatorUpdateListener() {
+
+            @Override
+            public void onAnimationUpdate(ValueAnimator animation) {
+                float animatedValue = (float) animation.getAnimatedValue();
+                if (animatedValue == 0) {
+                    mAtMaxAnimationValue = false;

[Theresa, 2017/07/14 16:56:38]

Instead of keeping a class variable, you could just check:

if (MathUtils.areFloatsEqual(animatedValue, 1) {
  mOldHeight = mClipHeight;
  ...
  return;
}

<all of your other code>

[Daniel Park, 2017/07/14 23:15:39]

Done.

+                }
+                if (!mAtMaxAnimationValue) {
+                    if (mDifferenceInHeight < 0) {
+                        setTranslationY(animatedValue * -mDifferenceInHeight / 2);
+                    } else {
+                        setTranslationY((1 - animatedValue) * mDifferenceInHeight / 2);
+                    }
+                    mClipHeight = mOldHeight + (int) (mDifferenceInHeight * animatedValue);
+                    setClipBounds(new Rect(0, 0, mCanvasWidth, mClipHeight));
+                    invalidate();
+                }
+                if (animatedValue == 1) {
+                    mAtMaxAnimationValue = true;

[Theresa, 2017/07/14 16:56:38]

It's more customary for us to do this sort of thing in onAnimationEnd() instead
of onAnimationUpdate().

[Daniel Park, 2017/07/14 23:15:40]

Done.

+                    mOldHeight = mClipHeight;
+                    requestLayout();
+                    setTranslationY(0);
+                }
+            }
+        });
+    }
+
+    @Override
+    public void onDraw(Canvas canvas) {
+        mCanvasWidth = canvas.getWidth();
+        int backgroundOffsetX = getScrollX();
+        Drawable drawable = ApiCompatibilityUtils.getDrawable(
+                getResources(), R.drawable.white_with_rounded_corners);
+        drawable.setBounds(

[Theresa, 2017/07/14 16:56:38]

You should call drawable.mutate() before setting the bounds:

"Make this drawable mutable. This operation cannot be reversed. A mutable
drawable is guaranteed to not share its state with any other drawable. This is
especially useful when you need to modify properties of drawables loaded from
resources. By default, all drawables instances loaded from the same resource
share a common state; if you modify the state of one instance, all the other
instances will receive the same modification. Calling this method on a mutable
Drawable will have no effect."

https://developer.android.com/reference/android/graphics/drawable/Drawable.ht...


Currently white_with_rounded_corners is only used for the context menu, but if
it's used other places in the future and mutate isn't called we may end up with
odd visual behavior due to the shared state.

[Daniel Park, 2017/07/14 23:15:40]

Done.

+                backgroundOffsetX, 0, canvas.getWidth() + backgroundOffsetX, mClipHeight);
+        drawable.draw(canvas);
+        super.onDraw(canvas);
     }
 }