As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1202861 - (compact-tabs) Add "Compact tabs" settings item under "Customize"
(compact-tabs)
: Add "Compact tabs" settings item under "Customize"
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal with 2 votes (vote)
: Firefox 53
Assigned To: Tom Klein
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 1201945 (view as bug list)
Depends on: 1203182 1202066 1214602 1215158 1215162 1215232 1310081 1323945 1323951 1323952
Blocks: tabs-tray-v3
  Show dependency treegraph
 
Reported: 2015-09-08 14:46 PDT by Anthony Lam (:antlam)
Modified: 2017-01-10 07:34 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
prev_mob_tray4b.png (415.27 KB, image/png)
2015-09-10 09:56 PDT, Anthony Lam (:antlam)
no flags Details
Screenshot_2015-10-08-13-38-26.png (163.51 KB, image/png)
2015-10-08 13:39 PDT, Edwin Wong [:edwong]
no flags Details
Bug 1202861 - 1. Create compact (two column) tabs tray option for portrait mode phones. (58 bytes, text/x-review-board-request)
2016-11-29 11:57 PST, Tom Klein
s.kaspari: review+
Details | Review
Bug 1202861 - 2. Refresh tabs panel when compact tabs configuration changes. (58 bytes, text/x-review-board-request)
2016-11-29 11:57 PST, Tom Klein
s.kaspari: review+
Details | Review

Description User image Anthony Lam (:antlam) 2015-09-08 14:46:57 PDT
This is what we currently call it in the iOS Fennec settings. I think it might be worth while putting this in as a pref for now, enabled by default. 

Pros:
 - Allows us to measure telemetry on this pref
 - We can use this to gauge feedback 
 - Keeps consistent with iOS
 - Enabled by default means all users will still get to see the new tray
Comment 1 User image Anthony Lam (:antlam) 2015-09-08 16:37:53 PDT
Title: Compact tabs
Subtitle: tbd

I think this pref should not revert between old and new UI, but rather offer two column versus one column layout. That is to say, new proportions, full screen tabs tray, and back button (bug 1200619), will all still be the same when a user flips between this pref. 

That way, this will be a way to address feedback from users that require a page title and a 1 column layout. 

Thoughts? I'll attach a mock.
Comment 2 User image Anthony Lam (:antlam) 2015-09-10 09:56:50 PDT
Created attachment 8659350 [details]
prev_mob_tray4b.png

This is what I meant. Let me know if you have questions. I've changed nothing except placing the text next to the tab preview. 

Paddings are all the same, dimensions, etc
Comment 3 User image Mark Finkle (:mfinkle) (use needinfo?) 2015-09-10 11:03:42 PDT
(In reply to Anthony Lam (:antlam) from comment #1)
> Title: Compact tabs
> Subtitle: tbd
> 
> I think this pref should not revert between old and new UI, but rather offer
> two column versus one column layout. That is to say, new proportions, full
> screen tabs tray, and back button (bug 1200619), will all still be the same
> when a user flips between this pref. 

Sounds good to me. Most feedback is about the size of the thumbnails, having too many hit targets jammed together, small 'x', and strange swipe-to-close behavior with two columns. Your mock/plan should address all of those issues.

One thing I would suggest: Should this be in Settings > Customize? Or should we just have a simple toggle button in the actionbar of the tabs tray?

I like having the button in the tabs tray. It's easier to find and it's better for context. Tap, and the layout changes.
Comment 4 User image Anthony Lam (:antlam) 2015-09-10 11:52:46 PDT
(In reply to Mark Finkle (:mfinkle) from comment #3)
> One thing I would suggest: Should this be in Settings > Customize? Or should
> we just have a simple toggle button in the actionbar of the tabs tray?
> 
> I like having the button in the tabs tray. It's easier to find and it's
> better for context. Tap, and the layout changes.

Hm, interesting. How about under the overflow menu "..." ?

While I want to give it top visibility, after the user chooses a preference, it's not necessarily great UX to leave it around all the time.
Comment 5 User image Mark Finkle (:mfinkle) (use needinfo?) 2015-09-10 11:59:08 PDT
(In reply to Anthony Lam (:antlam) from comment #4)
> (In reply to Mark Finkle (:mfinkle) from comment #3)
> > One thing I would suggest: Should this be in Settings > Customize? Or should
> > we just have a simple toggle button in the actionbar of the tabs tray?
> > 
> > I like having the button in the tabs tray. It's easier to find and it's
> > better for context. Tap, and the layout changes.
> 
> Hm, interesting. How about under the overflow menu "..." ?
> 
> While I want to give it top visibility, after the user chooses a preference,
> it's not necessarily great UX to leave it around all the time.

SOLD!
Comment 6 User image Barbara Bermes [:barbara] - NI please! 2015-09-15 11:30:18 PDT
Can we please make sure that we add some probes to measure this, i.e. how many people use compact vs. old style?

Thanks :)
Comment 7 User image Anthony Lam (:antlam) 2015-09-15 13:59:58 PDT
*** Bug 1201945 has been marked as a duplicate of this bug. ***
Comment 8 User image Barbara Bermes [:barbara] - NI please! 2015-09-30 08:22:31 PDT
Hey Martyn, please let me know if you have any questions regarding the probes I'd like to put in there and or if there is anything else you need info from me.
Comment 9 User image Edwin Wong [:edwong] 2015-10-08 13:37:11 PDT
+1 on this bug. My problem with the new two column view is:
* I can't see enough of the title to understand what tab contains
* Often I see a globe icon rather than a screen contents as I open in background tab from FB/Twitter
* the status message fade in/out covers the bottom row as this is my most recent tabs

Mcomella asked me to NI him on this in passing.
Comment 10 User image Edwin Wong [:edwong] 2015-10-08 13:39:29 PDT
Created attachment 8671592 [details]
Screenshot_2015-10-08-13-38-26.png
Comment 11 User image Michael Comella (:mcomella) [not active on fennec/Bugzilla: contact me via IRC] 2015-10-09 04:37:18 PDT
(In reply to Edwin Wong [:edwong] from comment #9)
> * Often I see a globe icon rather than a screen contents as I open in
> background tab from FB/Twitter

This should be filed in another bug, though I'd expect it to already be filed.

> * the status message fade in/out covers the bottom row as this is my most
> recent tabs

We plan to fix this with bug 1157526, where a SnackBar will cover significantly less of the tabs.

> Mcomella asked me to NI him on this in passing.

I don't recall asking that – are you sure it was me?
Comment 12 User image Paul [pwd] 2015-10-14 05:12:12 PDT
I raised some of these issues a while back with bugs like bug 651822. I would ideally like to see us have three options, the grid option we have presently, a compact option with just the text and favicon akin to what we present on desktop and also implement a tab carousel as per what Chrome* and the Android Recent Task switcher users. We could store thumbnails locally to ensure that users are never presented with a grey globe. Users could switch between modes via pinch-to-zoom

I filed bug 1119628 to put lay the foundation for that.
Comment 13 User image Tom Klein 2016-11-29 11:57:10 PST Comment hidden (mozreview-request)
Comment 14 User image Tom Klein 2016-11-29 11:57:10 PST Comment hidden (mozreview-request)
Comment 15 User image Sebastian Kaspari (:sebastian) 2016-12-07 12:43:23 PST
Comment on attachment 8815432 [details]
Bug 1202861 - 1. Create compact (two column) tabs tray option for portrait mode phones.

https://reviewboard.mozilla.org/r/96334/#review96808

I used it on my phone and it's great \o/

I think we can land this, but we should change the default for now. We are currently thinking about moving this behind switchboard flags to enable/disable this for a random sample of users and see how they react. But for now we can just land it disabled by default.

::: mobile/android/base/java/org/mozilla/gecko/tabs/AutoFitTabsGridLayout.java:17
(Diff revision 1)
> +import android.content.res.Resources;
> +import android.support.v7.widget.GridLayoutManager;
> +import android.util.AttributeSet;
> +import android.util.Log;
> +
> +class AutoFitTabsGridLayout extends TabsGridLayout {

Random thought: Ignoring all the different spacing values and if we get a design that accommodates this: Could we at some point have a single layout that supports 1, 2 or dynamic columns? Apart from a different tab layout in the 1 column case this "feels" like it's the same grid only with a different number of columns.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:79
(Diff revision 1)
>  
>          if (HardwareUtils.isTablet() || isLandscape) {
> -            return new TabsGridLayout(context, attrs);
> +            return new AutoFitTabsGridLayout(context, attrs);
> +        } else {
> +            // Phone in portrait mode.
> +            if (GeckoSharedPrefs.forApp(context).getBoolean("android.not_a_preference.compact_tabs", true)) {

Use the pref constant here! (And see below for default)

::: mobile/android/base/resources/xml/preferences_general.xml:36
(Diff revision 1)
> +    <SwitchPreference android:key="android.not_a_preference.compact_tabs"
> +                      android:title="@string/pref_compact_tabs"
> +                      android:summary="@string/pref_compact_tabs_summary"
> +                      android:defaultValue="true"
> +                      android:persistent="true"/>

Product (barbara) and UX (antlam) are currently discussing whether we are going to make this the default or not. But I don't want to block landing this, so maybe we should just make it off by default for now. And then later change the default again in a follow-up bug.
Comment 16 User image Sebastian Kaspari (:sebastian) 2016-12-07 13:37:31 PST
Comment on attachment 8815433 [details]
Bug 1202861 - 2. Refresh tabs panel when compact tabs configuration changes.

https://reviewboard.mozilla.org/r/96336/#review96792

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1430
(Diff revision 1)
> +        if (mTabsPanel != null) {
> +            mTabsPanel.unregisterListeners();
> +        }

Instead of doing this here, maybe it makes more sense to do it in the view in onAttachedToWindow() / onDetachedFromWindow()?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:51
(Diff revision 1)
>                         implements GeckoPopupMenu.OnMenuItemClickListener,
>                                    LightweightTheme.OnChangeListener,
> -                                  IconTabWidget.OnTabChangedListener {
> +                                  IconTabWidget.OnTabChangedListener,
> +                                  SharedPreferences.OnSharedPreferenceChangeListener {
>      private static final String LOGTAG = "Gecko" + TabsPanel.class.getSimpleName();
> +    private static final String PREF_COMPACT_TABS_KEY = "android.not_a_preference.compact_tabs";

We keep most of them in GeckoPreferences. There's also a constant for "android.not_a_preference.".
Comment 17 User image Tom Klein 2016-12-07 19:29:36 PST Comment hidden (mozreview-request)
Comment 18 User image Tom Klein 2016-12-07 19:29:36 PST Comment hidden (mozreview-request)
Comment 19 User image Tom Klein 2016-12-07 19:39:06 PST
Comment on attachment 8815433 [details]
Bug 1202861 - 2. Refresh tabs panel when compact tabs configuration changes.

https://reviewboard.mozilla.org/r/96336/#review96792

> Instead of doing this here, maybe it makes more sense to do it in the view in onAttachedToWindow() / onDetachedFromWindow()?

To be honest it's not clear to me when/under what circumstances onDetachedFromWindow for TabsPanel can get called, but testing looks good, so I've made the switch.
Comment 20 User image Tom Klein 2016-12-07 20:13:56 PST
Comment on attachment 8815432 [details]
Bug 1202861 - 1. Create compact (two column) tabs tray option for portrait mode phones.

https://reviewboard.mozilla.org/r/96334/#review96808

> Random thought: Ignoring all the different spacing values and if we get a design that accommodates this: Could we at some point have a single layout that supports 1, 2 or dynamic columns? Apart from a different tab layout in the 1 column case this "feels" like it's the same grid only with a different number of columns.

I'm not sure exactly what you mean, but basically I think the answer is yes.  We could (probably) have a single TabsLayout class that determines, based on phone vs. tablet, portrait vs. landscape, dimensions available, and a range of permissible inter-tab spacings, how many "ideal"-width tabs we can fit across, and then sets things accordingly.  It might not give desired span counts in all cases though without some extra guidance (is my guess).  Implementation-wise it would help if we just did the current LinearLayoutManager case using a one-column GridLayoutManager, and it would also help (i.e. simplify things) if we always allowed a range of spacing options between tabs (unlike the current compact tabs, where the inner spacing is fixed equal to the outer padding).
Comment 21 User image Pulsebot 2016-12-08 20:44:11 PST
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4de97a01678b
1. Create compact (two column) tabs tray option for portrait mode phones. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/88e0fb654a10
2. Refresh tabs panel when compact tabs configuration changes. r=sebastian
Comment 23 User image Oana Horvath 2017-01-10 07:34:46 PST
Verified as fixed in Nightly build 53.0a1 2017-01-09;
Devices: 
-LG G4 Android  5.1;
-Lenovo A536 Android 4.4.2;
Verified there's no change made on tablets, on: Asus ZenPad (Android 6.0.1).

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