1183149 - Remove bounce animation of left and right Panel labels in about:home

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Chenxia, I know you'll miss this bounce animation.. but I don't think its necessary anymore if we use "scrollable tabs".

It's served it's purpose now and given lots of tips to our users :)

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Filing to keep track of removing this.

Comment 2

[Chenxia Liu [:liuche]]

We no longer want to animate the titles, so the animateTitles should be removed, along with the PreDrawListener.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePagerTabStrip.java#85

Comment 3

[Giovanny Gongora [:gioyik]]

Attachment: 1183149.patch

Comment 4

[Giovanny Gongora [:gioyik]]

Hi,

I submitted a patch for this bug. Let me know if is good or needs changes.

Thanks

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8637645 [details] [diff] [review]
1183149.patch

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

Looks good, just needs some cleanup of unused fields.

Also, include r=liuche at the end of the commit message for this patch so that we can keep track of the reviewer.

::: mobile/android/base/home/HomePagerTabStrip.java
@@ -90,5 @@
> -            return;
> -        }
> -
> -        // Set up initial values for the views that will be animated.
> -        ViewHelper.setTranslationX(prevTextView, -INIT_OFFSET);

Take a look at the constants defined in this file - we're removing a lot of them in this patch, so you should make sure that they're removed from the declarations as well.

Comment 6

[Giovanny Gongora [:gioyik]]

Attachment: 1183149.patch

Patch updates

Comment 7

[Giovanny Gongora [:gioyik]]

Hi Chenxia,

Thanks for the feedback. I didn't take in count constants until you mentioned them. Thank you a lot, this will help me to be more careful in future bugs. I attached another patch that should be a good on to land and fix this bug.

Let me know if it needs changes or anything else to fix.

Thanks

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8638256 [details] [diff] [review]
1183149.patch

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

One last thing to check - since you're removing all the animations from this file, there are imports that are no longer used. Can you remove them as well?

Comment 9

[Giovanny Gongora [:gioyik]]

Attachment: 1183149.patch

Comment 10

[Giovanny Gongora [:gioyik]]

Hi Chenxia,

I attached a new patch. Hope this one is complete and correct. Let me know if you need something. Also if is necessary to run this patch on try server I could do it too, I have commit access level 1, so I can make push to try server, only need the commit message with the syntax for this.

Thanks

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8645138 [details] [diff] [review]
1183149.patch

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

Hmm, did you build this? I think you'll see that it doesn't compile, because you're referencing the method animateTitles, which you removed.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

We removed HomePagerTabStrip and replaced it with TabMenuStrip in bug 1185002. This removed the animation too.