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

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
6 years ago
3 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

({reproducible})

26 Branch
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 unaffected, fennec+)

Details

Attachments

(3 attachments)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
tracking-fennec: --- → ?
Keywords: reproducible
What release is this happening in? Fx26 or Fx25?
Assignee: nobody → sriram
tracking-fennec: ? → +
(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.
Assignee

Comment 4

6 years ago
Assignee

Updated

6 years ago
Duplicate of this bug: 936857
Assignee

Comment 6

6 years ago
Duping here because there's more discussion here, and linking over the clip rnewman took: https://www.youtube.com/watch?v=BVJh4vvJJAs
Posted patch PatchSplinter Review
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.
Attachment #8335572 - Flags: review?(liuche)
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?
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).
Attachment #8335676 - Flags: feedback?(liuche)
Assignee

Comment 10

6 years ago
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?
Attachment #8335572 - Flags: review?(lucasr.at.mozilla)
Attachment #8335572 - Flags: review?(liuche)
Attachment #8335572 - Flags: feedback+
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);
Attachment #8335572 - Flags: review?(lucasr.at.mozilla) → review-
Assignee

Comment 12

6 years ago
Yeah, I also tested this build on both phones and tablets, and the animation looks really janky again...
(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.
Attachment #8335676 - Flags: feedback?(lucasr.at.mozilla)
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).
Attachment #8335676 - Flags: feedback?(lucasr.at.mozilla) → feedback+
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.
Assignee

Comment 16

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

Clearing review flag because I'm going on PTO.
Attachment #8335676 - Flags: feedback?(liuche)
Chenxia, can you determine whether this is fixed after bug 917896 lands?
Depends on: 917896
Flags: needinfo?(liuche)
Duplicate of this bug: 959010
I'm surprised such a glaring visual glitch is only tracking +.
Assignee: sriram.mozilla → nobody
Assignee

Comment 20

6 years ago
Will do.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Hardware: ARM → All
Assignee

Comment 21

6 years ago
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.
Flags: needinfo?(liuche)
Throbber replaced on trunk with a progress-bar; marking as such. Other channels still affected.
Assignee

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.