1058909 - Implement new tablet toolbar display mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Yuan mentioned over IRC, "I believe we are going to move the refresh icon inside the URL bar and have Bookmarks, Tab indicator, and Menu icons"

The choice of buttons should be finalized and implemented.

Comment 1

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

I got icons and a mock for you Michael :) 

will post soon

Comment 2

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

Attachment: prev_tablet_toolbar3icon.png

here's a WIP of what the "toolbar" currently looks like with 3 icons and tabs on top. 

One of the things I'm concerned about is the spacing when there's 3 icons up there, on the one had, it gives users good quick "hot keys" that we could even possibly make customizable (not talking about that yet), but on the other hand, it takes up quite a lot of space.

Another thing I'm also playing with here is the URL (input) box tucked behind the "back" button on the left. Drawing inspiration form the desktop side, this looks great and matches our desktop counter parts but not sure if it causes any implementation issues.

Thoughts?

Comment 3

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

Attachment: prev_tablet_gutters.png

Also wanted to point out what gutter system I've got for this so far (taking largely from the mobile side and scaling accordingly/ when necessary).

The larger ones are 15dp where as the smaller ones are 7dp. 7 is about the smallest I think we go ATM in the new design just from some visual research and testing I did on various sizes.

Comment 4

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

(In reply to Anthony Lam (:antlam) from comment #2)
> One of the things I'm concerned about is the spacing when there's 3 icons up
> there, on the one had, it gives users good quick "hot keys" that we could
> even possibly make customizable (not talking about that yet), but on the
> other hand, it takes up quite a lot of space.

The width is similar to the current implementation with the exception that the tabs button is currently on the left hand side of the toolbar - as a first implementation, I don't think the width is much of an issue. As a plus, I think this gives us more flexibility because the tabs button has the ability to be user customized.

> Another thing I'm also playing with here is the URL (input) box tucked
> behind the "back" button on the left.

This is the same as the current fennec implmentation (phone and tablet), right? Otherwise, I'm not sure I understand.

Comment 5

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

(In reply to Michael Comella (:mcomella) from comment #4)
> The width is similar to the current implementation with the exception that
> the tabs button is currently on the left hand side of the toolbar - as a
> first implementation, I don't think the width is much of an issue. As a
> plus, I think this gives us more flexibility because the tabs button has the
> ability to be user customized.

Yeah, I'm thinking the same thing. Especially since super long URL bars are... less than useful.
 
> This is the same as the current fennec implmentation (phone and tablet),
> right? Otherwise, I'm not sure I understand.

Doh- dunno what I was thinking.

Comment 6

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

Anthony, are there new icons associated with this bug?

Comment 7

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

Anthony, should the tabs tray button display the number of open tabs any longer? If not, should it continue to animate when a tab is added/removed?

Comment 8

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

(In reply to Michael Comella (:mcomella) from comment #7)
> Anthony, should the tabs tray button display the number of open tabs any
> longer? If not, should it continue to animate when a tab is added/removed?

Yes! The number is missing from the mocks, but I'd like to keep it in for V1 (at least).

(In reply to Michael Comella (:mcomella) from comment #6)
> Anthony, are there new icons associated with this bug?

I will attach the star, tabs, and overflow icons. Their are also "contextual" icons in the URL bar (in this screen shot it's a 'refresh' icon). These are more finicky as me and Yuan try to wrangle the UX behind it but basically it's a bit smaller and change for each context (think page loading, loaded, etc).. we can explain more once we have more spec'd out or just ping us :)

Comment 9

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

Attachment: Three page actions

What do you think, Lucas?

Keep in mind that the refresh icon is likely larger than it will be in the final version, and the icon spacing is not final.

Comment 10

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

Comment on attachment 8486022 [details]
Three page actions

This is coming together well! Just looking at the 3 functions on top, I think it works pretty well.

Spacing and sizing as you mentioned will need to be worked on but I think we also need to consider how to organize all those icons we have in the URL bar now... 

I'm worried that I feel a slight onset of decision paralysis when I look at all those functions. Does anyone else feel similar?

Comment 11

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Do we need the drop-down triangle in the URL bar? I wasn't aware that it is there. I think we could remove the drop-down triangle icon, since it doesn't provide much additional value for the tablet scenario. And it's also hard to hit especially with two icons next to each other. Thoughts?

I feel like two icons on the right side of URL bar is the maximum. "Refresh" should definitely be there. And "Reader mode" icon is dependent the site content. I know the Android guy icon shows up at the same position, but that should be contextual too.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #11)
> Do we need the drop-down triangle in the URL bar? I wasn't aware that it is
> there. I think we could remove the drop-down triangle icon, since it doesn't
> provide much additional value for the tablet scenario. And it's also hard to
> hit especially with two icons next to each other. Thoughts?
> 
> I feel like two icons on the right side of URL bar is the maximum. "Refresh"
> should definitely be there. And "Reader mode" icon is dependent the site
> content. I know the Android guy icon shows up at the same position, but that
> should be contextual too.

Keep in mind that add-ons can add an arbitrary number of page actions to the URL bar. So we need to handle 'overflow' here. We should probably increase the maximum number of actions before the overflow arrow is displayed on tablets. Right now I think it's 2. Maybe try 3 to see how it looks?

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #9)
> Created attachment 8486022 [details]
> Three page actions
> 
> What do you think, Lucas?
> 
> Keep in mind that the refresh icon is likely larger than it will be in the
> final version, and the icon spacing is not final.

I'm fine with how it the arrow looks on the left side of the refresh button. I share the same concern than Yuan regarding the hit areas but this is not a new issue. We can handle that in a follow-up if you prefer.

One question to UX folks: what is the problem we're solving by moving the refresh button inside the URL bar on tablets? Is it because we're planning to do the same on phones (where this would actually make a difference) and want keep things consistent somehow? Consistency with desktop? I'm starting to feel that moving the refresh button inside the URL entry is bringing more problems than solutions (because of the way we display page actions now). It would be nice to get a bit more context about this decision before we go ahead with this.

Comment 14

[Lucas Rocha (:lucasr)]

Oh, now I see that we're promoting the 'star' button as a primary action in the toolbar. Maybe this is why we're moving the refresh button somewhere else? I assume this 'star' button will display the same kind of UI than the new share overlay feature? It's a lot less appealing if it's just the current bookmark action.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8486022 [details]
Three page actions

f+ for the way the page actions are arranged, which was a concern I had.

Comment 16

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

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Keep in mind that add-ons can add an arbitrary number of page actions to the
> URL bar. So we need to handle 'overflow' here. We should probably increase
> the maximum number of actions before the overflow arrow is displayed on
> tablets. Right now I think it's 2. Maybe try 3 to see how it looks?

Yeah I agree, is 3 the max that we show? This might be another issue that we should look at altogether.

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Oh, now I see that we're promoting the 'star' button as a primary action in
> the toolbar. Maybe this is why we're moving the refresh button somewhere
> else?

To be more consistent from a visual structure stand point was definitely one of the considerations. But I agree it seems to be bringing up more issues. I would be ok with considering moving it back to the original position for V1. That way, we could look at grouping page actions and that as a part of V2.

> I assume this 'star' button will display the same kind of UI than the
> new share overlay feature? It's a lot less appealing if it's just the
> current bookmark action.

Great question - My bad, I should've given more context here. 

Back in July we got some UI telemetry data about tablet usage on Beta and we had ~48% of people using bookmarks compared to the ~6% on phones. This, put together with the reload button usage (on tablets) at ~27% (~12% on phones) made me think the bookmarks button needed a better spot (since it was a bit more hidden on phones).

So, I wanted to try this out and see if it would be a better experience because it also functions as a visual "oh hey, I bookmarked this page" indicator that made more sense now that we have Sync up and running. 

Now, that being said, I completely agree with the concerns about whether or not this is interesting enough to warrant it's spot in the top 3. I'd like to look at the ability to customize this 'top 3' in the future but it's definitely not on the board for V1.

Comment 17

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

In today's tablet meeting, we decided to leave the bookmark button in the overflow menu, and the refresh button in the toolbar. This other behavior will be saved for a future iteration (v2?).

Comment 18

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

Attachment: Part 1: Don't hide the tabs tray button when opening the tabs tray on new tablet

Comment 19

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

The original rationale of moving the Refresh icon is to create that visual consistency with Desktop UI. But I wasn't aware of the page action indicators. 

I agree with you that this change seems to raise more issues than solutions. I am fine with having the Refresh replace Bookmarks on the toolbar. 

In the mean time, we should be thinking about:
1. Can we create any shortcut to add a bookmark either from tab strip or tab panel?
2. How to handle the overflow situation of page action indicators?

Thanks!

Comment 20

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

Attachment: (WIP v1) No favicon margin change

Anthony, what do you think?

I did not include the margin change for the favicon because I wasn't sure how to handle the margins of the url title and security indicators. Note that the favicon will be removed from the url bar and this design can be implemented as is after bug 1066253 is implemented. 

Some other design inconsistencies:
  * New tab button in tab strip is not aligned with options button (bug 1067556)
  * Left-most tab in tab strip does not have enough margin to the left (bug 1067556)
  * The back button does not have the same proportions as the mock, but is more consistent with desktop. The back button margins are consistent with neither.

Let me know if there's something you want me to address.

Comment 21

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

Comment on attachment 8489612 [details]
(WIP v1) No favicon margin change

Awesome ! this is coming along nicely.

Now that I have most of the default UI laid out, I can post a spec to address the things you brought up - stay tuned!

Comment 23

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

Attachment: spec_structure_fxtabs1.png

I'm going to post these as I get em so you don't have to wait for one giant image full of crazy specs. :)

Let's start with the overall structure here and some basic clearance gutters.

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8489590 [details] [diff] [review]
Part 1: Don't hide the tabs tray button when opening the tabs tray on new tablet

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

Awesome.

Comment 25

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

Attachment: (WIP) Icon size

Using the new assets from bug 1060413 comment 10 the back icon seems to be too small - do you know what's going on here, Anthony? We typically don't scale (outside of what the Android system does for device pixel-independence) and I don't think there's any scaling going on here.

Comment 26

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

I've since downsized the size of the toolbar so maybe that's why? 

Are we currently using the latest sizes in https://bugzilla.mozilla.org/attachment.cgi?id=8490135 ?

Unless, I got it wrong and it's supposed to be XXHDPI..

Comment 27

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

(In reply to Anthony Lam (:antlam) from comment #26)
> I've since downsized the size of the toolbar so maybe that's why?

What do you mean by downsized the size of the toolbar?

> Are we currently using the latest sizes in
> https://bugzilla.mozilla.org/attachment.cgi?id=8490135 ?

Yes, we should be. (though this is an increase in size up from 56dp)

> Unless, I got it wrong and it's supposed to be XXHDPI..

I'm not sure how much work it is to export resources under the assumption that the N7 is XXHDPI, but we could always try that too!

Comment 28

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

The size of my circle in the design is 42 dp, how large is it there?

(In reply to Michael Comella (:mcomella) from comment #27)
> I'm not sure how much work it is to export resources under the assumption
> that the N7 is XXHDPI, but we could always try that too!

We can try adjusting for this if we can't figure this out but just assuming it's bigger should actually make our icons even smaller...

Comment 29

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

Attachment: 42px back border

(In reply to Anthony Lam (:antlam) from comment #28)
> The size of my circle in the design is 42 dp, how large is it there?

50dp. This attachment is 42dp and I think explains our discrepency (note that I did not yet change the sizes of the remaining toolbar bits).

Comment 30

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

https://hg.mozilla.org/integration/fx-team/rev/829f8ab0bc4f

Comment 31

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

Attachment: Part 2: Add resources

These resources are not yet final but I have some things I'm unsure of:
  1) In our 1x1, you mentioned trying to avoid duplication - does that apply to
any of these? The assets all appear different to me than those that exist
though, with scaling (from phone assets), we could remove a lot of them.
  2) Should we be using xxhdpi resources too?
  3) We should be using the "new_tablet_*" naming for all new drawables, right?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/829f8ab0bc4f

Comment 33

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

Attachment: Part 3: Adjust new tablet browser_toolbar spacing

I still have to do a few things:
  * Get the proper assests and fix the tabs panel button
  * The assets look a little small - figure that out with antlam
  * Fix the forward button (part 4)
  * Correct the spacing between the favicon/security and the page title. This
is hard because it has been to set up dynamically to maintain the old tablet
state. I'll come back to this after the ToolbarDisplayLayout split work in
bug 1066253.

However, I have a few hacks (marked with TODO) to work around keeping the old
tablet that I want to make sure are okay to commit to. What do you think,
Lucas?

Comment 34

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

Attachment: (WIP) Icon size

The menu bar icons look a little small here. Anthony, what do you think? Note that the tabs panel icon is not yet updated to the new resources (which explains its skewing).

Comment 35

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

Comment on attachment 8491843 [details]
(WIP) Icon size

They seem ok to me. :)

They're definitely smaller than before but I've deliberately made them smaller so they have more room to breath around them. I think this makes them feel more comfortable when tapping them and just less cramped to look at.

Comment 36

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

Attachment: Part 4: Adjust forward button position and spacing

Complete, besides rebasing.

Comment 37

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

Attachment: Forward button

Anthony, since I didn't have specs for the forward button, I eye-balled it. What do you think?

Comment 38

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #37)
> Created attachment 8491869 [details]
> Forward button
> 
> Anthony, since I didn't have specs for the forward button, I eye-balled it.
> What do you think?

Drive-by: this is looking a little weird to me. The circular button only really works if it's clearly larger than the toolbar.

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 8491867 [details] [diff] [review]
Part 4: Adjust forward button position and spacing

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

Looks good to me.

Comment 40

[Lucas Rocha (:lucasr)]

Comment on attachment 8491122 [details] [diff] [review]
Part 2: Add resources

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

Before we go ahead with these: can we simply override the current tablet's resources and avoid the duplication? Are these images visually incompatible with our current tablet UI?

Comment 41

[Lucas Rocha (:lucasr)]

Comment on attachment 8491841 [details] [diff] [review]
Part 3: Adjust new tablet browser_toolbar spacing

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

Looking good overall, I have a few some questions though.

::: mobile/android/base/favicons/Favicons.java
@@ +423,5 @@
>  
>          pageURLMappings.putWithoutEviction(AboutPages.HOME, BUILT_IN_SEARCH_URL);
> +        // TODO: (bug 1058909) Remove this branch when old tablet is removed.
> +        final int searchFaviconID = NewTabletUI.isEnabled(context) ?
> +                R.drawable.new_tablet_toolbar_search_normal : R.drawable.favicon_search;

How is this new icon different than the current one? Is it visually incompatible with the current UI? In any case, I'd keep the same resource name, just add the prefix. In other words: new_tablet_favicon_search.

::: mobile/android/base/menu/GeckoMenuInflater.java
@@ +136,5 @@
> +        // TODO: (bug 1058909) Remove this branch when we remove old tablet.
> +        if (item.id == R.id.reload && NewTabletUI.isEnabled(mContext)) {
> +            item.iconRes = R.drawable.new_tablet_ic_menu_reload;
> +        } else {
> +            item.iconRes = a.getResourceId(R.styleable.MenuItem_android_icon, 0);

This is looking a bit suspicious: how's *android_icon related to new_tablet_ic_menu_reload?

::: mobile/android/base/resources/drawable-large/new_tablet_menu_level.xml
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<level-list xmlns:android="http://schemas.android.com/apk/res/android"
> +            xmlns:gecko="http://schemas.android.com/apk/res-auto">

How is the new tablet UI different that you need this new drawable here but not in the current UI? Not obvious from just looking at the patch.

::: mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml
@@ +50,5 @@
>                    android:layout_toLeftOf="@id/menu_items"/>
>  
>      <LinearLayout android:id="@+id/menu_items"
>                    android:layout_width="wrap_content"
> +                  android:layout_height="@dimen/browser_toolbar_menu_item_height"

Does this look ok with the pressed state looking smaller than the toolbar? This looking like 'design smell' to me.

Comment 42

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

Anthony, thoughts on comment 38?

Comment 43

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

Attachment: Old tablet, new icons

(In reply to Lucas Rocha (:lucasr) from comment #40)
> Before we go ahead with these: can we simply override the current tablet's
> resources and avoid the duplication? Are these images visually incompatible
> with our current tablet UI?

Technically, yes, but without scaling, the images are no longer the correct size. Attached is the old UI with our new tablet back, forward, and reload buttons. As for the other buttons, the search favicon is noticeably larger and the menu button is smaller, but they can be reused. The tabs panel button, on the other hand, has different alpha properties and color and most likely cannot be re-used (though antlam is working on an update for spacing, which may happen to fix that too).

Comment 44

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

(In reply to Lucas Rocha (:lucasr) from comment #41)
> > +        final int searchFaviconID = NewTabletUI.isEnabled(context) ?
> > +                R.drawable.new_tablet_toolbar_search_normal : R.drawable.favicon_search;
> 
> How is this new icon different than the current one?

Ignore my previous comment on the favicon images - the new and old icons look identical.

Anthony, are there any differences between the new and old favicon images? The binaries are different, but I think that's because we ran an optimizer over one.

> ::: mobile/android/base/resources/drawable-large/new_tablet_menu_level.xml
> How is the new tablet UI different that you need this new drawable here but
> not in the current UI? Not obvious from just looking at the patch.

The old tablet UI just uses `drawable/menu_level.xml` because it can still take advantage of `drawable-large` that way - we need the new drawable to specify other drawables. Alternatively, we can use custom XML resources, but that seems like a lot of unnecessary work for something we'll be ripping out soon.

> mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml
> >      <LinearLayout android:id="@+id/menu_items"
> >                    android:layout_width="wrap_content"
> > +                  android:layout_height="@dimen/browser_toolbar_menu_item_height"
> 
> Does this look ok with the pressed state looking smaller than the toolbar?
> This looking like 'design smell' to me.

This is an intentional part of the design - see [1]. It currently looks a bit funky, but I think that's because I don't have the rounded corners, as in the spec - I was planning on doing that in a follow-up bug (bug 1070087).

[1]: https://bug1058909.bugzilla.mozilla.org/attachment.cgi?id=8489648

Comment 45

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

Attachment: spec_structure_forwardbutton1.png

Alas! moar specs!

Hope this helps, Michael.

Comment 46

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

Also, comment 44 Anthony. Thanks!

Comment 47

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

Not sure what you mean by "old favicons". 

Do you mean dimensions or design?

Comment 48

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

(In reply to Anthony Lam (:antlam) from comment #47)
> Not sure what you mean by "old favicons". 

New favicons images being the ones you just gave me and old favicon images being the ones in the tree [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/favicon_search.png

Comment 49

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

Both dimensions and design - can we use one set of the images without any noticeable issues between old tablet and new?

Comment 50

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

(In reply to Lucas Rocha (:lucasr) from comment #41)
> ::: mobile/android/base/menu/GeckoMenuInflater.java
> > +        if (item.id == R.id.reload && NewTabletUI.isEnabled(mContext)) {
> > +            item.iconRes = R.drawable.new_tablet_ic_menu_reload;
> > +        } else {
> > +            item.iconRes = a.getResourceId(R.styleable.MenuItem_android_icon, 0);
> 
> This is looking a bit suspicious: how's *android_icon related to
> new_tablet_ic_menu_reload?

`R.styleable.MenuItem_android_icon` appears to be the styleable used to retrieve menu item icons from the menu/ xml [1]. Note sure how these styleables get defined in R.java, however, because it can't be found in a source search.

My if statement basically jumps the "pull the resource ID from the xml" step in favor of passing in the resource ID.

[1]: http://androidxref.com/4.4.4_r1/search?q=MenuItem_android_icon&defs=&refs=&path=&hist=&project=abi&project=art&project=bionic&project=bootable&project=build&project=cts&project=dalvik&project=developers&project=development&project=device&project=docs&project=external&project=frameworks&project=hardware&project=libcore&project=libnativehelper&project=ndk&project=packages&project=pdk&project=prebuilts&project=sdk&project=system&project=tools

Comment 51

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

(In reply to Michael Comella (:mcomella) from comment #49)
> Both dimensions and design - can we use one set of the images without any
> noticeable issues between old tablet and new?

After bug 1066253 lands, the favicon will actually be tab strip size, which ends this issue because the sizes are different. However, to keep APK size down, we can just scale the favicon instead, as with do with all our other favicons.

Lucas, what do you think?

Comment 52

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

(In reply to Michael Comella (:mcomella) from comment #48)
> (In reply to Anthony Lam (:antlam) from comment #47)
> > Not sure what you mean by "old favicons". 
> 
> New favicons images being the ones you just gave me and old favicon images
> being the ones in the tree [1].
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xhdpi/favicon_search.png

Oh no these magnifying glass icons should be the same :) I tried to reuse these files where I could

Comment 53

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

Ah - I was just dropped them in, assuming they were different. Are there any other assets that were reused?

Comment 54

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #51)
> (In reply to Michael Comella (:mcomella) from comment #49)
> > Both dimensions and design - can we use one set of the images without any
> > noticeable issues between old tablet and new?
> 
> After bug 1066253 lands, the favicon will actually be tab strip size, which
> ends this issue because the sizes are different. However, to keep APK size
> down, we can just scale the favicon instead, as with do with all our other
> favicons.
> 
> Lucas, what do you think?

Yeah, let's start with this and look for an alternative if it ends up being problematic.

Comment 55

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #43)
> Created attachment 8492316 [details]
> Old tablet, new icons
> 
> (In reply to Lucas Rocha (:lucasr) from comment #40)
> > Before we go ahead with these: can we simply override the current tablet's
> > resources and avoid the duplication? Are these images visually incompatible
> > with our current tablet UI?
> 
> Technically, yes, but without scaling, the images are no longer the correct
> size. Attached is the old UI with our new tablet back, forward, and reload
> buttons. As for the other buttons, the search favicon is noticeably larger
> and the menu button is smaller, but they can be reused. The tabs panel
> button, on the other hand, has different alpha properties and color and most
> likely cannot be re-used (though antlam is working on an update for spacing,
> which may happen to fix that too).

Yeah, let's just share assets between old and new tablet UI when we feel confident about them. What I want to avoid is us doing follow-ups just to backout/fix issues caused by the new assets on the old UI. This is just a temporary situation after all.

Comment 56

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

Attachment: Part 2: Add new tablet toolbar resources

These should be final. Note that I did not include new_tablet_favicon_search_active because it is currently unused.

Comment 57

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

Attachment: Part 3: Adjust new tablet browser_toolbar spacing

Filed bug 1071226 for refining private browsing mode.

Comment 58

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

Attachment: New tablet browser toolbar

Comment 59

[Lucas Rocha (:lucasr)]

Comment on attachment 8493316 [details] [diff] [review]
Part 2: Add new tablet toolbar resources

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

I wonder if we should simply not have mdpi resources for tablets anymore. Can't think of any tablet with a medium density display. Ping kbrosnan/aaronmt and file a bug to discuss this?

Comment 60

[Lucas Rocha (:lucasr)]

Comment on attachment 8493320 [details] [diff] [review]
Part 3: Adjust new tablet browser_toolbar spacing

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

Looks good overall but I think I need more a bit context before giving r+.

::: mobile/android/base/resources/drawable-large/new_tablet_menu_level.xml
@@ +10,5 @@
> +
> +        <selector>
> +
> +            <item gecko:state_private="true" android:drawable="@drawable/menu_pb"/>
> +            <item android:drawable="@drawable/new_tablet_menu"/>

I this drawable necessary just because of the new menu icon?

::: mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml
@@ +88,5 @@
> +            style="@style/UrlBar.ImageButton.MenuItem"
> +            android:layout_alignLeft="@id/menu"
> +            android:layout_alignRight="@id/menu"
> +            android:src="@drawable/new_tablet_menu_level"
> +            android:visibility="gone"/>

Wondering: do we still need the separate views on the new tablet UI? We need this on phones because we fade the icon when you open the tabs tray. This is not necessary in the tablet UI though. Follow-up?

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +102,5 @@
>      <org.mozilla.gecko.toolbar.ToolbarDisplayLayout android:id="@+id/display_layout"
>                    style="@style/UrlBar.Button"
>                    android:layout_alignLeft="@id/url_bar_entry"
>                    android:layout_alignRight="@id/url_bar_entry"
> +                  android:paddingLeft="8dip"

Why is this change to the phone UI necessary here?

::: mobile/android/base/resources/layout/toolbar_display_layout.xml
@@ +8,5 @@
>  
>      <ImageButton android:id="@+id/favicon"
>                   style="@style/UrlBar.ImageButton"
>                   android:layout_width="@dimen/browser_toolbar_favicon_size"
>                   android:scaleType="fitCenter"

Why is this necessary?

::: mobile/android/base/resources/values-large-v11/dimens.xml
@@ +5,5 @@
>  
>  <resources>
>  
>      <dimen name="browser_toolbar_height">56dp</dimen>
> +    <dimen name="new_tablet_browser_toolbar_height">60dp</dimen>

This looks like design smell to me. 60dp is a bit too much and it's non-standard for a tablet toolbar on Android.

@@ +10,2 @@
>      <dimen name="browser_toolbar_button_padding">16dp</dimen>
> +    <dimen name="browser_toolbar_menu_item_height">48dp</dimen>

Same here. Why is the menu button smaller than the toolbar?

::: mobile/android/base/toolbar/ActionBarViewFlipper.java
@@ +24,5 @@
> +    public void onAttachedToWindow() {
> +        if (NewTabletUI.isEnabled(getContext())) {
> +            final ViewGroup.LayoutParams lp = getLayoutParams();
> +            lp.height = getResources().getDimensionPixelSize(R.dimen.new_tablet_browser_toolbar_height);
> +            this.setLayoutParams(lp);

Why is this new view necessary? Need more context here.

Comment 61

[Lucas Rocha (:lucasr)]

Anthony, a few questions:
1. I noticed that the menu button is smaller than the toolbar itself i.e. it has empty space around it. Is that intended? I'd like to avoid wasting screen real estate with empty space in the toolbar if possible.
2. Why are we using a 60dp toolbar instead of the standard 56dp from Android?

Comment 62

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

(In reply to Lucas Rocha (:lucasr) from comment #60)
> ::: mobile/android/base/resources/drawable-large/new_tablet_menu_level.xml
> > +            <item android:drawable="@drawable/new_tablet_menu"/>
> 
> I this drawable necessary just because of the new menu icon?

Yep.

> Wondering: do we still need the separate views on the new tablet UI? We need
> this on phones because we fade the icon when you open the tabs tray. This is
> not necessary in the tablet UI though. Follow-up?

bug 1071950.

> ::: mobile/android/base/resources/layout/browser_toolbar.xml
> > +                  android:paddingLeft="8dip"
> 
> Why is this change to the phone UI necessary here?

I removed 4dp of left padding from the first item (favicon) in toolbar_display_layout and added it to the browser_toolbar layouts to compensate.

> ::: mobile/android/base/resources/layout/toolbar_display_layout.xml
> >      <ImageButton android:id="@+id/favicon"
> >                   style="@style/UrlBar.ImageButton"
> >                   android:layout_width="@dimen/browser_toolbar_favicon_size"
> >                   android:scaleType="fitCenter"
> 
> Why is this necessary?

fitCenter? It probably isn't. Or the favicon view in general? For phones.

> ::: mobile/android/base/toolbar/ActionBarViewFlipper.java
> Why is this new view necessary? Need more context here.

This is necessary to change the height of the toolbar if we're on new_tablet. It can be removed when we remove old tablet because we can use R.dimen.browser_toolbar_height directly.

Comment 63

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

Attachment: Forward button to spec

Anthony, are you sure the sizes in [1] are correct? Note that the actual forward arrow (as opposed to it's frame) position may not be exactly correct because Android is funky and it'd be more work than I'd like to put in for a prototype.

I prefer the look of my eyeballed sizes ([2]).

[1]: https://bugzilla.mozilla.org/attachment.cgi?id=8492334
[2]: https://bug1058909.bugzilla.mozilla.org/attachment.cgi?id=8493346

Comment 64

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

(In reply to Michael Comella (:mcomella) from comment #53)
> Ah - I was just dropped them in, assuming they were different. Are there any
> other assets that were reused?

Cant think of any ATM, but that does raise a point of revisiting and trimming down some fat.

Comment 65

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

Comment on attachment 8494165 [details]
Forward button to spec

Stroke is a bit thin here, but we discussed this already :) the whole 56 vs 60 convo

Comment 66

[Lucas Rocha (:lucasr)]

(In reply to Anthony Lam (:antlam) from comment #65)
> Comment on attachment 8494165 [details]
> Forward button to spec
> 
> Stroke is a bit thin here, but we discussed this already :) the whole 56 vs
> 60 convo

If you're talking about the stroke in the input entry, we'll need a new asset for them. It would great if the same asset could be used across new and old tablet UIs and phone UI.

Comment 67

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

Comment on attachment 8493346 [details]
New tablet browser toolbar

This looks nice on top on content! But we can still try 56 first

Comment 68

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

(In reply to Lucas Rocha (:lucasr) from comment #66)
> (In reply to Anthony Lam (:antlam) from comment #65)
> > Comment on attachment 8494165 [details]
> > Forward button to spec
> > 
> > Stroke is a bit thin here, but we discussed this already :) the whole 56 vs
> > 60 convo
> 
> If you're talking about the stroke in the input entry, we'll need a new
> asset for them. It would great if the same asset could be used across new
> and old tablet UIs and phone UI.

Right - I think that might be a bit hard at least for V1. But I'll get a new 9 patch file of the URL bar done

Comment 69

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

Attachment: Part 3: Adjust new tablet browser_toolbar spacing

Updated as we discussed in the meeting:
  * 56 dp toolbar
  * Buttons take up full height of toolbar (reload being the exception)
  * Unfinal forward button size
  * Land with non-final assets

Comment 70

[Lucas Rocha (:lucasr)]

Comment on attachment 8494813 [details] [diff] [review]
Part 3: Adjust new tablet browser_toolbar spacing

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

Nice.

::: mobile/android/base/menu/GeckoMenuInflater.java
@@ +134,5 @@
>          item.hasSubMenu = false;
>  
> +        // TODO: (bug 1058909) Remove this branch when we remove old tablet.
> +        if (item.id == R.id.reload && NewTabletUI.isEnabled(mContext)) {
> +            item.iconRes = R.drawable.new_tablet_ic_menu_reload;

It's probably worth adding a comment state why you did here: which I guess is to avoid having a separate menu resource for new tablet just to use a different reload icon here. I thought we had decided to just leave the reload icon untouched for now and update it in a follow-up? I'm fine with landing with this change anyway.

::: mobile/android/base/resources/values-large-v11/styles.xml
@@ -24,5 @@
>      <style name="UrlBar.ImageButton.TabCount.NewTablet">
>          <item name="android:background">@drawable/new_tablet_tabs_count</item>
>      </style>
>  
> -

Unrelated, remove?

Comment 71

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

Attachment: Part 4: Adjust forward button position and spacing

Comment 72

[Lucas Rocha (:lucasr)]

Comment on attachment 8494817 [details] [diff] [review]
Part 4: Adjust forward button position and spacing

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

Good.

Comment 73

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

(In reply to Lucas Rocha (:lucasr) from comment #70)
> I thought we had decided to just leave the
> reload icon untouched for now and update it in a follow-up? I'm fine with
> landing with this change anyway.

I guess it was a miscommunication - I thought we'd add the new reload asset, but update it again later. Your approach makes more sense, but after speaking with antlam, I actually think it's not the asset that's the problem but how we automatically style it. I'll land as is and fix it in bug 1072466.

Comment 74

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

Attachment: Part 3: Adjust new tablet browser_toolbar spacing

Updated for nits.

Comment 75

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

Attachment: Part 4: Adjust forward button position and spacing

Rebase.

Comment 76

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

https://hg.mozilla.org/integration/fx-team/rev/3e1ec0a3b696
https://hg.mozilla.org/integration/fx-team/rev/540bc6af7020
https://hg.mozilla.org/integration/fx-team/rev/92d6256f69e9

Comment 77

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e1ec0a3b696
https://hg.mozilla.org/mozilla-central/rev/540bc6af7020
https://hg.mozilla.org/mozilla-central/rev/92d6256f69e9

Comment 78

[u421692]

Verified as fixed on latest Aurora and latest Nightly on Nexus 7(Android 5.0).
Was not able to verify on FF 35 Beta 1, because the tablet redesign feature is only available for Nightly builds(Nightly and Aurora); see https://bugzilla.mozilla.org/show_bug.cgi?id=1047561#c8