Closed
Bug 739407
Opened 13 years ago
Closed 12 years ago
[TABLET] Tabs Menu
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox15 verified, firefox16 verified, firefox17 verified, blocking-fennec1.0 -, fennec14+)
VERIFIED
FIXED
Firefox 16
People
(Reporter: ibarlow, Assigned: sriram)
References
Details
(Whiteboard: [tablet])
Attachments
(3 files, 2 obsolete files)
1.10 MB,
image/png
|
Details | |
136.58 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
15.73 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
tracking-fennec: --- → 14+
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
* 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)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Assignee | ||
Comment 15•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/6c5327e377a1 http://hg.mozilla.org/integration/mozilla-inbound/rev/6d23c3d5e804
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6c5327e377a1 https://hg.mozilla.org/mozilla-central/rev/6d23c3d5e804 (Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Assignee | ||
Comment 18•12 years ago
|
||
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?
Assignee | ||
Comment 19•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Comment 20•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #631210 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 21•12 years ago
|
||
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/65c97b27926c https://hg.mozilla.org/releases/mozilla-aurora/rev/6824811f5808
Updated•12 years ago
|
status-firefox15:
--- → fixed
Updated•12 years ago
|
Whiteboard: [tablet]
Depends on: 770270
Updated•12 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•