739407 - [TABLET] Tabs Menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Let's build on our existing tabs menu and make it even better.

Users should be able to 
* Open / close menu with a swipe gesture
* Create a new tab
* Close tabs
* Undo closed tabs

A mockup is attached, and more detailed specs and assets will follow soon.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

-ing to keep triage lists clean

Comment 2

[Eitan Isaacson [:eeejay]]

Slick! Does the darker background denote the current tab or the focus state? There should be a focus state indicator, also for the close button. Same with the phone version.

Comment 3

[Eitan Isaacson [:eeejay]]

Just a quick clarification of what I mean by "focus": It is when the user uses a keyboard or directional controller to navigate through on screen controls, like pressing tab on a desktop.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch adds new layout files (gecko_app.xml) for Tablet UI. 
The Tabs-Tray and Remote-Tabs are made as LinearLayout instead of an Activity.
The TabsPanel takes care of displaying the required UI. TabsPanel is a private variable in GeckoApp.
This doesn't change the Tabs-button yet.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

It's been a long time since the misnomer was rectified.
In the holy name of Fennec, I (would like to) hereby pronounce that TabsTray be called as "LocalTabs". ;)

Comment 6

[Sriram Ramasubramanian [:sriram]]

At this point, this requires three more patches:
1. Adding an expandable tabs-button, moving the "+" and "synced tabs" to it.
2. Adding the cleanup from margaret's patch
3. Changing the layout for tab thumnails and titles in the list.

In this, #3 is just addition of layout files, and hence won't be a big problem if we have to uplift it.

Comment 7

[Sriram Ramasubramanian [:sriram]]

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

This requires more changes, in styles, themes, filenames for drawables -- to be consistent. Hence I dont feel like having this change now.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/2): Layout change

* This patch changes the layout of the tabs-tray and remote-tabs to be a LinearLayout instead of an Activity.
* Newer files are added for tablets
* The tabs button won't work until gecko is ready.
  - Earlier, we opened the AwesomeBar. Now, we show the list, even if there is just one tab.
* This also kills the ActionBar.
  - I spent time in fixing the regression, to layout the actionbar on rotation. It didn't work. (Filing a followup).

Further followups:
1. The thumbnails are of different sizes in tablets and phones as per specification. This would potentially affect the about:home also.
2. The tabs-button should get rid of black portion. -- Need resources.
3. The sync-button are re-using existing resources. -- Need resources.
4. Orange! All tab and tab-button highlights should use Orange!! -- :sigh:

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/2): Animation

This patch adds a nice little sliding animation to the tabs.
It feels a bit rough because Gecko is unable to resize the viewport at 10ms.

Comment 10

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

Comment on attachment 631209 [details] [diff] [review]
Patch (1/2): Layout change

This is a huge patch, but, thankfully, it's not adding a lot of new functionality. The patch does add some behavior that might be tweaked or changed later. We really need to try it out first to see how it feels. The core changes seem sane.

Before landing this, I want you to make sure that basic browser functionality does not lead to crashes or severely broken functionality. I am OK with some rough edges, but not a busted browser. Test on phones and tablets.

>diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml

>     <style name="FindBar">
>-        <item name="android:layout_width">fill_parent</item>
>-        <item name="android:layout_height">wrap_content</item>

Why drop these. I see you have fill_parent and wrap_content in the layout xml files anyway. Wouldn't it be cleaner to leave this in place?

>-        <item name="android:orientation">horizontal</item>

I assume this is the default, so it's redundant

r+, and make sure followup bugs for animation problems and new resources are filed and link back to this bug so we can track the followups.

Comment 11

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

This probably needs a clobber so use the clobberer webpage.

Comment 12

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

Comment on attachment 631210 [details] [diff] [review]
Patch (2/2): Animation

The TabsLayoutChangeListener is a nice way to keep the code decoupled. I suppose I can let it go for now that the TabsPanel can only have a single TabsLayoutChangeListener. We can add support when we need it.

I have a contempt for all types of animations and this is no different. Slow phones and tablets will be really rough. We definitely need support for suspending the Gecko rendering and maybe a non-linear property interpolator, so we have less animation steps overall. Filed bugs for these issues.

I think we need this landed so we can move it forward. It definitely can't be uplifted until we fix some of the follow up bugs first.

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 631209 [details] [diff] [review]
> Patch (1/2): Layout change
> 
> This is a huge patch, but, thankfully, it's not adding a lot of new
> functionality. The patch does add some behavior that might be tweaked or
> changed later. We really need to try it out first to see how it feels. The
> core changes seem sane.
> 
> Before landing this, I want you to make sure that basic browser
> functionality does not lead to crashes or severely broken functionality. I
> am OK with some rough edges, but not a busted browser. Test on phones and
> tablets.
> 
> >diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml
> 
> >     <style name="FindBar">
> >-        <item name="android:layout_width">fill_parent</item>
> >-        <item name="android:layout_height">wrap_content</item>
> 
> Why drop these. I see you have fill_parent and wrap_content in the layout
> xml files anyway. Wouldn't it be cleaner to leave this in place?
> 
> >-        <item name="android:orientation">horizontal</item>
> 
> I assume this is the default, so it's redundant
> 
> r+, and make sure followup bugs for animation problems and new resources are
> filed and link back to this bug so we can track the followups.

I have moved the Find-In-Page to be inside the RelativeLayout. While moving, I found every other layout there have the layout-params specified there. This alone was referring to a style, which was more of an eye-sore to me. Hence moved it there, to make everything feel like in place.

Comment 14

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 631210 [details] [diff] [review]
> Patch (2/2): Animation
> 
> The TabsLayoutChangeListener is a nice way to keep the code decoupled. I
> suppose I can let it go for now that the TabsPanel can only have a single
> TabsLayoutChangeListener. We can add support when we need it.
> 
> I have a contempt for all types of animations and this is no different. Slow
> phones and tablets will be really rough. We definitely need support for
> suspending the Gecko rendering and maybe a non-linear property interpolator,
> so we have less animation steps overall. Filed bugs for these issues.
> 
> I think we need this landed so we can move it forward. It definitely can't
> be uplifted until we fix some of the follow up bugs first.

I have a added a Decelerating interpolator, that comes with Android. We can change this to anything, based on phones/tablet. We can increase the factor for the interpolator, say 2.0 or something, so that the number of steps reduce.

(Actually I wanted a Bouncing interpolator for phones to make it feel nice)

I've already filed bugs for the followups and tagged them here.
Both the patches have been tested on 1 gingerbread phone, 2 ICS phones, 1 honeycomb tablet. All possible regressions have been fixed in the patches.

Comment 15

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c5327e377a1
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d23c3d5e804

Comment 16

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/6c5327e377a1
https://hg.mozilla.org/mozilla-central/rev/6d23c3d5e804

(Merged by Ed Morley)

Comment 18

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 631209 [details] [diff] [review]
Patch (1/2): Layout change

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature - Tabs UI
User impact if declined: Tablet wouldnt have an optimized Tabs UI
Testing completed (on m-c, etc.): 06/08
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: -

Comment 19

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 631210 [details] [diff] [review]
Patch (2/2): Animation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature - Tabs UI
User impact if declined: Tablet wouldnt have an optimized Tabs UI
Testing completed (on m-c, etc.): 06/08
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: -

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 631209 [details] [diff] [review]
Patch (1/2): Layout change

[Triage Comment]
Tablet UI bugs are approved for Aurora 15, as they carry little risk to mobile and none to desktop.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/65c97b27926c
https://hg.mozilla.org/releases/mozilla-aurora/rev/6824811f5808