918015 - Regression: Tablet throbber animation does not move smoothly horizontal when navigating forward

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

On tablet, when navigating forward to a page, the loading throbber dips while the forward button is being hidden.

It's pretty adorable, but should probably be fixed.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

What release is this happening in? Fx26 or Fx25?

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> What release is this happening in? Fx26 or Fx25?

This was introduced in bug 853844 which landed in Fx26.

Comment 3

[Richard Newman [:rnewman]]

Dupe of Bug 936857?

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: rolling throbber

Comment 6

[Chenxia Liu [:liuche]]

Duping here because there's more discussion here, and linking over the clip rnewman took: https://www.youtube.com/watch?v=BVJh4vvJJAs

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Using <animated-rotate/> instead of rotating the view. I still haven't figured out how Android accomplishes it with just one drawable. But they use "ProgressBar" which does a lot of work. May be, if we copy and create our own version, we could do better.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Here's another option I'm trying:

<rotate/> can rotate a drawable from start to end degrees based on a "level". So, if we increment the level from 0 to 10000 at 1000 increments for every 100ms, we would get a nice animations. Do we want to take that approach? Or the default one above is enough?

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Animation inside View

This is another option. The Drawable can be a rotate drawable, and the level can be changed from 0 to 10000, and the drawable rotates smoooooooooothly. (If its called every 16ms).

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8335572 [details] [diff] [review]
Patch

Review of attachment 8335572 [details] [diff] [review]:
-----------------------------------------------------------------

I'll need to test this on a tablet tomorrow before giving r+. I'm also not sure I'm clear on why using Animatable fixes this problem, while using AnimationDrawable causes the problem? Not sure I know Android Animations well enough.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +42,5 @@
>  import android.content.res.Resources;
>  import android.graphics.Bitmap;
>  import android.graphics.Color;
>  import android.graphics.Rect;
> +import android.graphics.drawable.Animatable;

How does using an Animatable fix this problem? We still use the same .start() call, but just cast to Animatable.

@@ -972,2 @@
>                  setPageActionVisibility(true);
> -                mFavicon.setAnimation(mProgressSpinner);

Do we need Favicons to implement setAnimation anymore, or is that dead code now?

@@ +972,5 @@
>              if (selectedTab != null)
>                  setFavicon(selectedTab.getFavicon());
>  
>              if (mSpinnerVisible) {
>                  setPageActionVisibility(false);

Is the throbber still running at this point? If not, why didn't you need to stop it?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8335572 [details] [diff] [review]
Patch

Review of attachment 8335572 [details] [diff] [review]:
-----------------------------------------------------------------

Using AnimationDrawables has the (big) drawback of crappy framerates (I tested your patch locally to confirm). I'd prefer something more like a custom progress bar. But independently of the specific solution here, we should keep a smooth framerate on the throbber.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +963,3 @@
>              //To stop the glitch caused by mutiple start() calls.
>              if (!mSpinnerVisible) {
> +                ((Animatable) mFavicon.getDrawable()).start();

I'd prefer something like:
mFavicon.getDrawable().setVisible(true, true);

Comment 12

[Chenxia Liu [:liuche]]

Yeah, I also tested this build on both phones and tablets, and the animation looks really janky again...

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 8335572 [details] [diff] [review]
> Patch
> 
> Review of attachment 8335572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Using AnimationDrawables has the (big) drawback of crappy framerates (I
> tested your patch locally to confirm). I'd prefer something more like a
> custom progress bar. But independently of the specific solution here, we
> should keep a smooth framerate on the throbber.
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +963,3 @@
> >              //To stop the glitch caused by mutiple start() calls.
> >              if (!mSpinnerVisible) {
> > +                ((Animatable) mFavicon.getDrawable()).start();
> 
> I'd prefer something like:
> mFavicon.getDrawable().setVisible(true, true);

<animated-rotate/> is non-standard (non-public) API. There is no way to specify the number of frames or duration. That's the reason why it feels so.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8335676 [details] [diff] [review]
WIP: Animation inside View

Review of attachment 8335676 [details] [diff] [review]:
-----------------------------------------------------------------

Have an apk to share? Just curious about the framerate with this patch. This will be constantly invalidating the drawable (and the holding view consequently) which is slightly less efficient than doing animations with a hw layers.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +967,5 @@
>              Log.i(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - Throbber start");
>          } else {
>              Tab selectedTab = Tabs.getInstance().getSelectedTab();
>              if (selectedTab != null)
>                  setFavicon(selectedTab.getFavicon());

Strictly speaking, you should probably make things like setImageBitmap() and setImageDrawable() calls stop the animation runnable internally.

::: mobile/android/base/toolbar/FaviconButton.java
@@ +28,5 @@
> +            @Override
> +            public void run() {
> +                final int level = mDrawable.getLevel();
> +                mDrawable.setLevel(level + 100 >= 10000 ? 0 : level + 100);
> +                postDelayed(this, 16);

Replace this with ViewCompat.postOnAnimation(FaviconButton.this, this).

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8335676 [details] [diff] [review]
WIP: Animation inside View

Review of attachment 8335676 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +967,5 @@
>              Log.i(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - Throbber start");
>          } else {
>              Tab selectedTab = Tabs.getInstance().getSelectedTab();
>              if (selectedTab != null)
>                  setFavicon(selectedTab.getFavicon());

Drive-by, 'cos I'm touching this code right now and finding issues -- bear in mind that Bug 941844 and Bug 924712 still apply, so I wouldn't be surprised if this code were invoked multiple times... perhaps as much as six times on a page load. That might mess with your framerate, or even make a difference in which solution is best.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8335676 [details] [diff] [review]
WIP: Animation inside View

Clearing review flag because I'm going on PTO.

Comment 17

[Brian Nicholson (:bnicholson)]

Chenxia, can you determine whether this is fixed after bug 917896 lands?

Comment 19

[Aaron Train [:aaronmt]]

I'm surprised such a glaring visual glitch is only tracking +.

Comment 20

[Chenxia Liu [:liuche]]

Will do.

Comment 21

[Chenxia Liu [:liuche]]

I can't repro the rolling favicon with the the current APK from bug 917896, but the forward button behavior doesn't really seem to work anymore.

Comment 22

[Aaron Train [:aaronmt]]

Throbber replaced on trunk with a progress-bar; marking as such. Other channels still affected.