1073053 - Use "scrollable tabs" for panels navigation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Suggestions from bug 978306 comment 7:

Simple solution to consider: use the our tablet tab strip on phones. It allows you to scroll the homepage's tab strip independently of the panels giving you direct access to individual panels i.e. it doesn't force you scroll through the panels sequentially.

Comment 1

[Chenxia Liu [:liuche]]

Android L uses this kind of tabstrip in its Material Design now (see Contacts, Play Newstand, etc). This might be a good time to revisit this.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Could we get a build up to test this ? Scrollable Tabs might be quite useful considering our increasing number of awesome panels, as well as the shift towards bigger phones nowadays.

Comment 3

[:Margaret Leibovic]

I think that 36+ should have just turned into a +.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

renaming for discoverability.

Conversation is currently at testing scrollable tabs on Mobile (similar to what we have on Tablets) in hopes to improve discoverability of other panels as well as usability of this navigation.

Comment 5

[:Margaret Leibovic]

Sebastian, did you make a prototype of this that you want to share? I feel like this would be a nice small improvement to land.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #5)
> Sebastian, did you make a prototype of this that you want to share? I feel
> like this would be a nice small improvement to land.

Yeah, I did something like this for the prototype I've built in Whistler. Code (hacking quality!) and screenshot are here:
https://github.com/pocmo/MaterialFennecPrototype

I used the TabLayout class from the new Material Design Library. We probably do not want to import this library into Fennec just yet (I assume it's quite large with all the images). But I guess we can just re-use the implementation we have for tablets.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1073053_scollable_tabs.patch

This patch replaces the tabs on phones with the scrollable tabs we use on tablets. This part was easy.

Unfortunately our custom implementation doesn't support "tabContentStart" like TabLayout from the Design Support Library. I'll see if I can add this to our implementation. Eventually we might want to use the design support library (for other things too) and get rid of our custom implementation (I'll create a bug for that).

Comment 8

[:Margaret Leibovic]

Comment on attachment 8635235 [details] [diff] [review]
1073053_scollable_tabs.patch

Review of attachment 8635235 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomePager.java
@@ -191,5 @@
>                      setCurrentItem(index, true);
>                  }
>              });
> -        } else if (child instanceof HomePagerTabStrip) {
> -            mTabStrip = child;

Looks like you could also kill the HomePagerTabStrip class and all its references.

I remember this code being confusing to follow, so this might be a good time to audit some of the surrounding logic as well. In particular, I remember finding this `Decor` interface to be confusingly named:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#124

TabMenuStrip is the only thing that implements this interface, and I don't remember what the historic reasons were for adding it. If we're only going to support one type of tab strip view, maybe we can reduce the complexity around this.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Sebastian Kaspari from comment #9)
> Created attachment 8635347 [details]
> MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels
> navigation. r=mhaig

After a couple of iterations with Anthony this patch does a bit more now:

* Set the tab bar height to 48dp (following material design guidelines)

* Indent scrollable tabs (as per guidelines)

* Unifies the default order of panels (phone order == tablet order)

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #8)
> Looks like you could also kill the HomePagerTabStrip class and all its
> references.

I'm about to file follow-up bugs for that. We need to replace the tab bar in the "first run" screen first. :)

Comment 12

[Martyn Haigh (:mhaigh)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

https://reviewboard.mozilla.org/r/13533/#review12239

Ship It!

::: mobile/android/base/home/TabMenuStripLayout.java:125
(Diff revision 1)
> +        if (fromPosition == 0 && toPosition == 1) {

It'd be nice to put a comment explaining the logic here

::: mobile/android/base/resources/values/colors.xml:119
(Diff revision 1)
> +  <color name="panel_tab_text_selected">#FF777777</color>

Are these colours not the same as our placeholder_grey ?  It'd be nice to reference that directly (if it make sense) instead of adding new resources

::: mobile/android/base/home/HomeConfigPrefsBackend.java
(Diff revision 1)
> -        if (HardwareUtils.isTablet()) {

Yay - I love simplifying things :D

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fdee43b169

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

This patch adds comments to describe why the modifier is there and what's it doing. I also simplified the colors and removing duplicates. Preserving r+.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0440c87603c

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Sebastian Kaspari from comment #16)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0440c87603c

testAboutHomeVisibility (actually AboutHomeComponent) needed to be updated

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Comment 19

[Martyn Haigh (:mhaigh)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

https://reviewboard.mozilla.org/r/13533/#review12307

::: mobile/android/tests/browser/robocop/testAboutHomeVisibility.java
(Diff revision 3)
> -import org.mozilla.gecko.home.HomeConfig;

Was this the only change to this file?  

Cleaning up as we go is a great idea, but it'd be good to make a note of any non functinoal changes in the commit or put them in to another patch, else it becomes noise.

::: mobile/android/tests/browser/robocop/components/AboutHomeComponent.java:34
(Diff revision 3)
> -    // TODO: Having a specific ordering of panels is prone to fail and thus temporary.
> +    private static final List<PanelType> PANEL_ORDERING = Arrays.asList(

Nice - the old implementation was nasty.

Comment 20

[Martyn Haigh (:mhaigh)]

Comment on attachment 8635347 [details]
MozReview Request: Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

https://reviewboard.mozilla.org/r/13533/#review12309

Ship It!

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Sebastian Kaspari from comment #6)
> I used the TabLayout class from the new Material Design Library. We probably
> do not want to import this library into Fennec just yet (I assume it's quite
> large with all the images).

fwiw, it's 332k on my machine.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #21)
> fwiw, it's 332k on my machine.

Yeah, at some point I just added it to the gradle build for testing and the APK was about 100k bigger. I still want to file a bug to investigate. Especially if we are going to go forward with 'materializing' Fennec. We definitely don't want to write all the components ourselves.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Martyn Haigh (:mhaigh) from comment #19)
> ::: mobile/android/tests/browser/robocop/testAboutHomeVisibility.java
> (Diff revision 3)
> > -import org.mozilla.gecko.home.HomeConfig;
> 
> Was this the only change to this file?  
> 
> Cleaning up as we go is a great idea, but it'd be good to make a note of any
> non functinoal changes in the commit or put them in to another patch, else
> it becomes noise.

Agreed! I was expecting to change testAboutHomeVisibility but then it was enough to edit AboutHomeComponent. I'm going to remove this change from the patch before landing.

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/8b5431cadfe02ef003f48250ac13fa08a28191eb
changeset:  8b5431cadfe02ef003f48250ac13fa08a28191eb
user:       Sebastian Kaspari <[e-mail]>
date:       Thu Jul 16 20:36:42 2015 +0200
description:
Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b5431cadfe0

Comment 26

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-07-23-10-17-45.png

Tested using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 42.0a1 (2015-07-22)

Is it expected that a blank space is displayed on the left of the "Top Sites" panel header?

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #26)
> Is it expected that a blank space is displayed on the left of the "Top
> Sites" panel header?

Yes! Anthony and I added this to follow the material guidelines for 'scrollable tabs': https://www.google.de/design/spec/components/tabs.html#tabs-types-of-tabs

Comment 28

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #27)
> (In reply to Teodora Vermesan (:TeoVermesan) from comment #26)
> > Is it expected that a blank space is displayed on the left of the "Top
> > Sites" panel header?
> 
> Yes! Anthony and I added this to follow the material guidelines for
> 'scrollable tabs':
> https://www.google.de/design/spec/components/tabs.html#tabs-types-of-tabs

I have to say, I think this looks kinda awkward... I would be in favor of moving the first title all the way to the left, especially since that would also optimize for showing more items to the right.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

The idea is to align the left-most tab with the keyline, see: https://material-design.storage.googleapis.com/publish/material_v_4/material_ext_publish/0B6Okdz75tqQsQ2l2R1lSU0hzZkE/components_tabs_usage_specs11.png

However currently nothing else aligns with the keyline and therefore it's in need of getting used to. We have also been thinking about aligning it with the left top sites tiles and the url bar - at least until they are updated as well.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with 'Use "scrollable tabs" for panels navigation' as wording.
I will update the notes when we have some user doc about it.