Closed Bug 1073053 Opened 6 years ago Closed 5 years ago

Use "scrollable tabs" for panels navigation

Categories

(Firefox for Android :: Awesomescreen, defect)

35 Branch
All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed
relnote-firefox --- 42+
fennec + ---

People

(Reporter: Margaret, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

(Keywords: user-doc-needed)

Attachments

(2 files, 1 obsolete file)

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.
tracking-fennec: 36+ → ---
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.
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.
I think that 36+ should have just turned into a +.
tracking-fennec: --- → +
Summary: Use about:home tab strip from tablets on phones → Use "scrollable tabs" for panels navigation
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.
Assignee: nobody → kbenhmida
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.
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
Blocks: 1183149
No longer depends on: 1183149
Assignee: kbenhmida → s.kaspari
Attached patch 1073053_scollable_tabs.patch (obsolete) — Splinter Review
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 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.
Attachment #8635235 - Attachment is obsolete: true
Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh
Attachment #8635347 - Flags: review?(mhaigh)
(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)
(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. :)
Status: NEW → ASSIGNED
Attachment #8635347 - Flags: review?(mhaigh) → review+
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
Attachment #8635347 - Flags: review+ → review?(mhaigh)
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 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+.
Attachment #8635347 - Flags: review?(mhaigh) → review+
(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 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
Attachment #8635347 - Flags: review+ → review?(mhaigh)
Attachment #8635347 - Flags: review?(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.
Attachment #8635347 - Flags: review+
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!
(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.
(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.
(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.
url:        https://hg.mozilla.org/integration/fx-team/rev/8b5431cadfe02ef003f48250ac13fa08a28191eb
changeset:  8b5431cadfe02ef003f48250ac13fa08a28191eb
user:       Sebastian Kaspari <s.kaspari@gmail.com>
date:       Thu Jul 16 20:36:42 2015 +0200
description:
Bug 1073053 - Use "scrollable tabs" for panels navigation. r=mhaigh
https://hg.mozilla.org/mozilla-central/rev/8b5431cadfe0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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?
(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
(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.
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.
Depends on: 1186841
Depends on: 1187278
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.
You need to log in before you can comment on or make changes to this bug.