feed: Don't spin the spinner when its ancestors are hidden

Currently, the spinner used in feed will only stop() its animation if
the spinner view itself is hidden through setVisibility(). However, in
reality, one of its ancestor views is hidden instead.

This causes a continuous invisible animation to tick at the display's
refresh rate (usually 60fps) if the spinner was ever shown, until it is
destroyed (when the NTP views are eventually evicted).

Instead, stop the animation also when any of the spinner's ancestors
become hidden by listening for visibility changes and window attachment
state changes.

Bug: 1151391
Change-Id: Ib7b7a8e56f083619420fd58f4340991253402535
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552781Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830957}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/shared/ui/MaterialSpinnerView.java

@@ -14,11 +14,15 @@ import androidx.annotation.Nullable;
 import androidx.appcompat.widget.AppCompatImageView;
 import androidx.swiperefreshlayout.widget.CircularProgressDrawable;
 
+import org.chromium.base.FeatureList;
+import org.chromium.base.TraceEvent;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 
 /** View that shows a Material themed spinner. */
 public class MaterialSpinnerView extends AppCompatImageView {
     private final CircularProgressDrawable mSpinner;
+    private final boolean mAlwaysAnimate;
 
     public MaterialSpinnerView(Context context) {
         this(context, null);
@@ -33,6 +37,7 @@ public class MaterialSpinnerView extends AppCompatImageView {
     @SuppressWarnings({"nullness:argument.type.incompatible", "nullness:method.invocation.invalid"})
     public MaterialSpinnerView(Context context, @Nullable AttributeSet attrs, int defStyle) {
         super(context, attrs, defStyle);
+        TraceEvent.begin("MaterialSpinnerView");
         mSpinner = new CircularProgressDrawable(context);
         mSpinner.setStyle(CircularProgressDrawable.DEFAULT);
         setImageDrawable(mSpinner);
@@ -40,19 +45,45 @@ public class MaterialSpinnerView extends AppCompatImageView {
         Theme theme = context.getTheme();
         theme.resolveAttribute(R.attr.feedSpinnerColor, typedValue, true);
         mSpinner.setColorSchemeColors(typedValue.data);
+        mAlwaysAnimate = FeatureList.isInitialized() ? ChromeFeatureList.isEnabled(
+                                 ChromeFeatureList.INTEREST_FEED_SPINNER_ALWAYS_ANIMATE)
+                                                     : false;
+        updateAnimationState(isAttachedToWindow());
+        TraceEvent.end("MaterialSpinnerView");
+    }
 
-        if (getVisibility() == View.VISIBLE) {
-            mSpinner.start();
+    @Override
+    protected void onVisibilityChanged(View changedView, int visibility) {
+        super.onVisibilityChanged(changedView, visibility);
+        updateAnimationState(isAttachedToWindow());
     }
+
+    @Override
+    protected void onAttachedToWindow() {
+        super.onAttachedToWindow();
+        updateAnimationState(/*isAttached=*/true);
     }
 
     @Override
-    public void setVisibility(int visibility) {
-        super.setVisibility(visibility);
+    protected void onDetachedFromWindow() {
+        // isAttachedToWindow() doesn't turn false during onDetachedFromWindow(), so we pass the new
+        // attachment state into updateAnimationState() here explicitly.
+        updateAnimationState(/*isAttached=*/false);
+        super.onDetachedFromWindow();
+    }
+
+    private void updateAnimationState(boolean isAttached) {
+        // TODO(crbug.com/1151391): This feature is used for A:B testing to determine the impact of
+        // a bug fix. Remove after experiment is complete.
+        if (mAlwaysAnimate) {
+            if (!mSpinner.isRunning()) mSpinner.start();
+            return;
+        }
 
-        if (mSpinner.isRunning() && getVisibility() != View.VISIBLE) {
+        boolean visible = isShown() && isAttached;
+        if (mSpinner.isRunning() && !visible) {
             mSpinner.stop();
-        } else if (!mSpinner.isRunning() && getVisibility() == View.VISIBLE) {
+        } else if (!mSpinner.isRunning() && visible) {
             mSpinner.start();
         }
     }

chrome/android/feed/core/javatests/src/org/chromium/chrome/browser/feed/shared/ui/MaterialSpinnerViewTest.java

@@ -8,6 +8,7 @@ import static com.google.common.truth.Truth.assertThat;
 
 import android.app.Activity;
 import android.view.View;
+import android.widget.FrameLayout;
 
 import androidx.swiperefreshlayout.widget.CircularProgressDrawable;
 
@@ -16,6 +17,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.Robolectric;
 import org.robolectric.annotation.Config;
+import org.robolectric.annotation.LooperMode;
 
 import org.chromium.chrome.R;
 import org.chromium.testing.local.LocalRobolectricTestRunner;
@@ -23,38 +25,52 @@ import org.chromium.testing.local.LocalRobolectricTestRunner;
 /** Tests for {@link MaterialSpinnerView}. */
 @RunWith(LocalRobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
+@LooperMode(LooperMode.Mode.PAUSED)
 public class MaterialSpinnerViewTest {
+    private FrameLayout mLayout;
     private MaterialSpinnerView mMaterialSpinnerView;
+    private CircularProgressDrawable mAnimationDrawable;
 
     @Before
     public void setUp() {
-        Activity context = Robolectric.buildActivity(Activity.class).get();
-        context.setTheme(R.style.Light);
-        mMaterialSpinnerView = new MaterialSpinnerView(context);
+        Activity activity = Robolectric.setupActivity(Activity.class);
+        activity.setTheme(R.style.Light);
+
+        // Attach the spinner inside a layout, so we can hide either the spinner
+        // or the parent view (ie. the layout) in the tests. Note that we
+        // require the looper to stay paused (LooperMode.Mode.PAUSED) for the
+        // duration of the tests. Otherwise, Robolectric will run through the
+        // animation and stop it before the tests get run. Because
+        // Robolectric.setupActivity() will run the looper until idle, we call
+        // setContentView() only after launching the activity above.
+        mMaterialSpinnerView = new MaterialSpinnerView(activity);
+        mAnimationDrawable = (CircularProgressDrawable) mMaterialSpinnerView.getDrawable();
+
+        mLayout = new FrameLayout(activity);
+        mLayout.addView(mMaterialSpinnerView);
+        activity.setContentView(mLayout);
     }
 
     @Test
     public void testInit_isVisible_spinnerStarted() {
         assertThat(mMaterialSpinnerView.getVisibility()).isEqualTo(View.VISIBLE);
+        assertThat(mMaterialSpinnerView.isShown()).isTrue();
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isTrue();
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
     }
 
     @Test
     public void testSetVisibility_gone_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.GONE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isFalse();
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
     }
 
     @Test
     public void testSetVisibility_invisible_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.INVISIBLE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isFalse();
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
     }
 
     @Test
@@ -62,7 +78,43 @@ public class MaterialSpinnerViewTest {
         mMaterialSpinnerView.setVisibility(View.GONE);
         mMaterialSpinnerView.setVisibility(View.VISIBLE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isTrue();
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
+    }
+
+    @Test
+    public void testContainerSetVisibility_gone_stopsSpinner() {
+        mLayout.setVisibility(View.GONE);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testContainerSetVisibility_invisible_stopsSpinner() {
+        mLayout.setVisibility(View.INVISIBLE);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testContainerSetVisibility_toTrue_startsSpinner() {
+        mLayout.setVisibility(View.GONE);
+        mLayout.setVisibility(View.VISIBLE);
+
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
+    }
+
+    @Test
+    public void testDetachFromWindow_stopsSpinner() {
+        mLayout.removeView(mMaterialSpinnerView);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testAttachToWindow_startsSpinner() {
+        mLayout.removeView(mMaterialSpinnerView);
+        mLayout.addView(mMaterialSpinnerView);
+
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
     }
 }

chrome/browser/flags/android/chrome_feature_list.cc

@@ -106,6 +106,7 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &feature_engagement::kIPHTabSwitcherButtonFeature,
     &feed::kInterestFeedContentSuggestions,
     &feed::kInterestFeedNoticeCardAutoDismiss,
+    &feed::kInterestFeedSpinnerAlwaysAnimate,
     &feed::kInterestFeedV1ClicksAndViewsConditionalUpload,
     &feed::kInterestFeedV2,
     &feed::kReportFeedUserActions,

chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java

@@ -322,6 +322,8 @@ public abstract class ChromeFeatureList {
     public static final String INTEREST_FEED_CONTENT_SUGGESTIONS = "InterestFeedContentSuggestions";
     public static final String INTEREST_FEED_NOTICE_CARD_AUTO_DISMISS =
             "InterestFeedNoticeCardAutoDismiss";
+    public static final String INTEREST_FEED_SPINNER_ALWAYS_ANIMATE =
+            "InterestFeedSpinnerAlwaysAnimate";
     public static final String INTEREST_FEED_V2 = "InterestFeedV2";
     public static final String KITKAT_SUPPORTED = "KitKatSupported";
     public static final String LOOKALIKE_NAVIGATION_URL_SUGGESTIONS_UI =

components/feed/feed_feature_list.cc

@@ -49,6 +49,10 @@ const base::Feature kInterestFeedV2ClicksAndViewsConditionalUpload{
 const base::Feature kInterestFeedNoticeCardAutoDismiss{
     "InterestFeedNoticeCardAutoDismiss", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Used for A:B testing of a bug fix (crbug.com/1151391).
+const base::Feature kInterestFeedSpinnerAlwaysAnimate{
+    "InterestFeedSpinnerAlwaysAnimate", base::FEATURE_DISABLED_BY_DEFAULT};
+
 const char kDefaultReferrerUrl[] =
     "https://www.googleapis.com/auth/chrome-content-suggestions";
 

components/feed/feed_feature_list.h

@@ -33,6 +33,8 @@ extern const base::Feature kInterestFeedV2ClicksAndViewsConditionalUpload;
 
 extern const base::Feature kInterestFeedNoticeCardAutoDismiss;
 
+extern const base::Feature kInterestFeedSpinnerAlwaysAnimate;
+
 std::string GetFeedReferrerUrl();
 
 }  // namespace feed