763049 - Phones should slide the browser, "not shrink" on opening and closing the tab-menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

As per Sriram's suggestion

Comment 1

[Sriram Ramasubramanian [:sriram]]

(In reply to Aaron Train [:aaronmt] from comment #0)
> As per Sriram's suggestion

As per Ian's suggestion ;)

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch adds a sliding animation for phones and a sliding+shrinking animation for tablets.

In case of tablets, the browser_toolbar needs to shrink and the content portion should slide and then shrink. Since the PropertyAnimator can be easily extended, I have moved the constructor to an "attach()" method, to attach as many views as needed. This gets iterated over in run() and invalidate() happens for each view. "from" is calculated during the start(), to keep the calling side simple. ViewGroup.MarginLayoutParams is used to make things generic.

onPropertyAnimationStart() and onPropertyAnimationEnd() are used to be used for knowing the start and end of animations.
Note: They are called from background thread, hence mMainHandler's post() method should be used. Always posting to GeckoApp.mMainHandler directly from PropertyAnimator will make it lose its generic nature.

Comment 3

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

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

Looks OK. I like the use of the PropertyAnimationListener to handle "view specific" cleanup.  Needing to handle two views in different ways on tablet does make life more complicated.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            // Set the gecko layout for sliding.
>+            if (!mTabsPanel.isShown()) {
>+                ((LinearLayout.LayoutParams) mGeckoLayout.getLayoutParams()).setMargins(0, 0, 0, 0);
>+                mGeckoLayout.scrollTo(mTabsPanel.getWidth() * -1, 0);
>+                mGeckoLayout.requestLayout();
>+            }

Is it possible to move this the the onPropertyAnimationStart callback? That makes it more symmetric with the onPropertyAnimationEnd code.

r+, but see if we can move the code to the onPropertyAnimationStart method

Comment 4

[Sriram Ramasubramanian [:sriram]]

I thought of moving it there. But here are the problems:
1. The onPropertyAnimationStart() is called after "from" value is taken and everything is scheduled to happen. But this should happen before that.
2. Even if I change the logic of start to do these before scheduling animation, this needs to happen in an UI thread. Hence there will be a race between posting a runnable here in onPropertyAnimationStart(), that refreshing the view, and that being taken as from value in start().

That's why I had to have here.

Comment 5

[Wesley Johnston (:wesj)]

Is using ScrollBy better for us than using a straight up TranslateAnimation somehow?

http://developer.android.com/reference/android/view/animation/TranslateAnimation.html

On phones, it seems like that should deliver a smoother result.

Comment 6

[Sriram Ramasubramanian [:sriram]]

ScrollBy looks better to me. They way Android calculates things and the way I'm calculating things are bit different. I remember seeing whole content flashing with Android's animation.

Also, browser toolbar needs to be shrunk. If that uses my logic and content portion uses Android's logic, then everything wouldn't sync up easily.

Comment 7

[Wesley Johnston (:wesj)]

Yeah, translate would only work (well) on phones (where browserToolbar doesn't need to shrink). I've seen flickering before, but most of it was cleared up by explicitly setting fillAfter and fillBefore. I'll wait and see how this looks.

Comment 8

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/764e33d3648f

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/764e33d3648f

Comment 11

[Sriram Ramasubramanian [:sriram]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: The intended animation for tabs UI will be clunky.
Testing completed (on m-c, etc.): 06/13
Risk to taking this patch (and alternatives if risky): Low. This makes the animation handler robust. This has been in central for a week now.
String or UUID changes made by this patch: None.

Comment 12

[Alex Keybl [:akeybl]]

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

[Triage Comment]
In support of the new tabs UI. Approving for Aurora 15.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch for Aurora

It so happened that wesj's patches for refactoring GeckoApp landed right before I landed this in central. Hence some of the code in GeckoApp had to go into BrowserApp.

But aurora doesn't have BrowserApp, hence moving that piece of code from BrowserApp to GeckoApp.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 635032 [details] [diff] [review]
Patch for Aurora

The older patch posted is for central -- which is same as new patch. Killing the new one.

Comment 15

[Sriram Ramasubramanian [:sriram]]

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

Comment 16

[Paul Feher]

Verified fixed on:
Htc Desire Z (2.3.3)
Using:
Nightly Fennec 16.0a1 (2012-07-09)
Aurora Fennec 15.0a2 (2012-07-09)

When tab-menu is opened the browser slides.