Closed Bug 762955 (clunk) Opened 10 years ago Closed 10 years ago

Tabs drop-down transition/animation is too clunky

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 verified, firefox16 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: aaronmt, Assigned: sriram)

References

Details

Attachments

(1 file)

Alias: clunk
Duplicate of this bug: 763178
Attached patch PatchSplinter Review
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.
Attachment #632158 - Flags: review?(mark.finkle)
Comment on attachment 632158 [details] [diff] [review]
Patch

Does it make sense to add TabsListContainer to GeckoViewsFactory?
Attachment #632158 - Flags: review?(mark.finkle) → review+
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)
(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.
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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/70b9974c9b4f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
(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.
Something regressed this; the clunk returneth (06/14).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
And now I can not reproduce. Will open a new bug if I get STR.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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:
Attachment #632158 - Flags: approval-mozilla-aurora?
(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?
(It sounds like we'll need this since we're uplifting the new tabs UI, but I just want to make sure.)
(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.
(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 on attachment 632158 [details] [diff] [review]
Patch

Sounds good - let's land on Aurora 15.
Attachment #632158 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
I beg to differ, but that can be a different bug.
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.