Closed Bug 739407 Opened 8 years ago Closed 8 years ago

[TABLET] Tabs Menu

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- -
fennec 14+ ---

People

(Reporter: ibarlow, Assigned: sriram)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [tablet])

Attachments

(3 files, 2 obsolete files)

Attached image 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.
-ing to keep triage lists clean
blocking-fennec1.0: --- → -
tracking-fennec: --- → 14+
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.
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.
Depends on: 757198
Depends on: 758317
Depends on: 758319
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → sriram
Attachment #629690 - Flags: review?(mark.finkle)
Attached patch Patch (obsolete) — Splinter Review
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". ;)
Attachment #629692 - Flags: review?(mark.finkle)
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 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.
Attachment #629692 - Attachment is obsolete: true
Attachment #629692 - Flags: review?(mark.finkle)
* 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:
Attachment #629690 - Attachment is obsolete: true
Attachment #629690 - Flags: review?(mark.finkle)
Attachment #631209 - Flags: review?(mark.finkle)
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.
Attachment #631210 - Flags: review?(mark.finkle)
Depends on: 762722
Depends on: 762723
Depends on: 762724
Depends on: 762725
Depends on: 762727
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.
Attachment #631209 - Flags: review?(mark.finkle) → review+
This probably needs a clobber so use the clobberer webpage.
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.
Attachment #631210 - Flags: review?(mark.finkle) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/6c5327e377a1
https://hg.mozilla.org/mozilla-central/rev/6d23c3d5e804

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Depends on: clunk
Duplicate of this bug: 758319
Depends on: 763202
Depends on: 763448
Depends on: 763338
Depends on: 763446
Depends on: 763406
Depends on: 763396
Depends on: 763485
Depends on: 763726
Depends on: 764729
Depends on: 765580
Depends on: 765805
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: -
Attachment #631209 - Flags: approval-mozilla-aurora?
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: -
Attachment #631210 - Flags: approval-mozilla-aurora?
Depends on: 765941
Depends on: 760970
No longer depends on: 765941
Depends on: 765941
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.
Attachment #631209 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #631210 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 769388
Depends on: 768835
Whiteboard: [tablet]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.