762955 - (clunk) Tabs drop-down transition/animation is too clunky

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

See slowness: http://www.youtube.com/watch?v=CpIRhJWQqVw&hd=1

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

The clunkiness was caused by two different dispatchLayoutChange() called for phones. This was required as the tabs didnt inflate during the first call.

Now with fixed height tabs UI on phones, this is greatly avoided, as the height is constant.

The TabsListContainer has been removed from TabsTray and RemoteTabs. It has been added to TabsPanel, as "layout_weight" of "0.5" can't be used (as there is a browsertoolbar and there is an empty space, and calculations will become messier). Instead we force 50% for the lists in the TabsListContainer's instantiation.

Comment 3

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

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

Does it make sense to add TabsListContainer to GeckoViewsFactory?

Comment 4

[Aaron Train [:aaronmt]]

Some small drive-by testing with Sriram's build in channel, clunkiness improved. Galaxy Nexus exposes a 'bounce' near end of the animation which looks poor (http://www.youtube.com/watch?v=rPYDfcUEYNE)

Comment 5

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

(In reply to Aaron Train [:aaronmt] from comment #4)
> Some small drive-by testing with Sriram's build in channel, clunkiness
> improved. Galaxy Nexus exposes a 'bounce' near end of the animation which
> looks poor (http://www.youtube.com/watch?v=rPYDfcUEYNE)

Yeah, I agree about the "bounce". Let's remove it for now.

Comment 6

[Sriram Ramasubramanian [:sriram]]

The custom build used a Bounce interpolator. This patch doesn't have it. I havent posted a patch for bounce interpolator as the default one is good for animation over 2000ms. Ours is just 150 ms.

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 632158 [details] [diff] [review]
> Patch
> 
> Does it make sense to add TabsListContainer to GeckoViewsFactory?

TabsListContainer can be added to GeckoViewsFactory, if we move it to a separate file. I don't feel a need for it, as it's a one time call to create.

Comment 8

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> The custom build used a Bounce interpolator. This patch doesn't have it. I
> havent posted a patch for bounce interpolator as the default one is good for
> animation over 2000ms. Ours is just 150 ms.

OK

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> (In reply to Mark Finkle (:mfinkle) from comment #3)
> > Comment on attachment 632158 [details] [diff] [review]
> > Patch
> > 
> > Does it make sense to add TabsListContainer to GeckoViewsFactory?
> 
> TabsListContainer can be added to GeckoViewsFactory, if we move it to a
> separate file. I don't feel a need for it, as it's a one time call to create.

That's fine. The TabsPanel is in GeckoViewsFactory and that's what mainly affects startup. If we see startup issues, we can adjust.

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/70b9974c9b4f

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/70b9974c9b4f

Comment 11

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> > The custom build used a Bounce interpolator. This patch doesn't have it. I
> > havent posted a patch for bounce interpolator as the default one is good for
> > animation over 2000ms. Ours is just 150 ms.
> 
> OK

Confirming this inbound build doesn't have a bounce on my Galaxy Nexus.

Comment 12

[Aaron Train [:aaronmt]]

Something regressed this; the clunk returneth (06/14).

Comment 13

[Aaron Train [:aaronmt]]

And now I can not reproduce. Will open a new bug if I get STR.

Comment 14

[Sriram Ramasubramanian [:sriram]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
There wouldn't be a smooth animation when Tabs UI is shown.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> Risk to taking this patch (and alternatives if risky): 

Would you mind addressing the risk here?

Comment 16

[Alex Keybl [:akeybl]]

(It sounds like we'll need this since we're uplifting the new tabs UI, but I just want to make sure.)

Comment 17

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

(In reply to Alex Keybl [:akeybl] from comment #15)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> > Risk to taking this patch (and alternatives if risky): 
> 
> Would you mind addressing the risk here?

Risk is moderate to low, but we do need this for the tabs ui uplift. Later patches are building on it too. Nothing seems to have regressed from this patch yet.

Comment 18

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> (In reply to Alex Keybl [:akeybl] from comment #15)
> > (In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> > > Risk to taking this patch (and alternatives if risky): 
> > 
> > Would you mind addressing the risk here?
> 
> Risk is moderate to low, but we do need this for the tabs ui uplift. Later
> patches are building on it too. Nothing seems to have regressed from this
> patch yet.

This is a pretty important patch. Though it may seem big, the risk is low. It basically merges two similar layouts into one -- hence seems like more cleanup and addition. But this patch is needed to have a smooth animation on phones and tablets -- without which the tabs ui might feel bumpy.

Comment 19

[Alex Keybl [:akeybl]]

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

Sounds good - let's land on Aurora 15.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/249506237ff5

Comment 21

[Cristian Nicolae (:xti)]

Tabs drop-down transition/animation is fast and smooth now on the latest Aurora and Nightly build. Closing bug as verified fixed on:

Firefox 16.0a1 (2012-07-09)
Firefox 15.0a2 (2012-07-09)

Device: Galaxy Nexus
OS: Android 4.0.4

Comment 22

[Aaron Train [:aaronmt]]

I beg to differ, but that can be a different bug.