Last Comment Bug 739407 - [TABLET] Tabs Menu
: [TABLET] Tabs Menu
Status: VERIFIED FIXED
[tablet]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 16
Assigned To: Sriram Ramasubramanian [:sriram]
:
Mentors:
: 758319 (view as bug list)
Depends on: 763202 770270 757198 758317 758319 760970 762722 762723 762724 762725 762727 clunk 763338 763396 763406 763446 763448 763485 763726 764729 765580 765805 765941 768835 769388
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-26 14:10 PDT by Ian Barlow (:ibarlow)
Modified: 2012-07-25 08:59 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
-
14+


Attachments
Mockup (1.10 MB, image/png)
2012-03-26 14:10 PDT, Ian Barlow (:ibarlow)
no flags Details
Patch (46.54 KB, patch)
2012-06-03 21:35 PDT, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review
Patch (3.93 KB, patch)
2012-06-03 21:45 PDT, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review
Patch (1/2): Layout change (136.58 KB, patch)
2012-06-07 16:26 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
Patch (2/2): Animation (15.73 KB, patch)
2012-06-07 16:27 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Ian Barlow (:ibarlow) 2012-03-26 14:10:55 PDT
Created attachment 609478 [details]
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 [:kbrosnan] 2012-03-26 17:13:04 PDT
-ing to keep triage lists clean
Comment 2 Eitan Isaacson [:eeejay] 2012-05-08 09:11:09 PDT
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] 2012-05-08 09:16:47 PDT
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] 2012-06-03 21:35:40 PDT
Created attachment 629690 [details] [diff] [review]
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] 2012-06-03 21:45:18 PDT
Created attachment 629692 [details] [diff] [review]
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] 2012-06-03 21:47:25 PDT
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] 2012-06-03 22:37:05 PDT
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] 2012-06-07 16:26:27 PDT
Created attachment 631209 [details] [diff] [review]
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] 2012-06-07 16:27:54 PDT
Created attachment 631210 [details] [diff] [review]
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?) 2012-06-07 19:45:09 PDT
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?) 2012-06-07 19:45:38 PDT
This probably needs a clobber so use the clobberer webpage.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-07 19:58:05 PDT
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] 2012-06-07 20:48:29 PDT
(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] 2012-06-07 20:51:06 PDT
(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 17 Sriram Ramasubramanian [:sriram] 2012-06-08 10:10:19 PDT
*** Bug 758319 has been marked as a duplicate of this bug. ***
Comment 18 Sriram Ramasubramanian [:sriram] 2012-06-18 15:33:30 PDT
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] 2012-06-18 15:33:48 PDT
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] 2012-06-19 19:15:29 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.