Implement new tablet toolbar display mode

VERIFIED FIXED in Firefox 35

Status

()

VERIFIED FIXED
4 years ago
5 months ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 verified)

Details

Attachments

(8 attachments, 15 obsolete attachments)

158.34 KB, image/png
Details
6.43 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
59.81 KB, image/png
Details
30.66 KB, image/png
Details
27.20 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
2.37 MB, image/png
antlam
: feedback+
Details
17.87 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
9.11 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
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.
I got icons and a mock for you Michael :) 

will post soon
Created attachment 8480222 [details]
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?
Created attachment 8480237 [details]
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.
(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.
(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.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Anthony, are there new icons associated with this bug?
Flags: needinfo?(alam)
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?
(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 :)
Flags: needinfo?(alam)
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.
Attachment #8486022 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8486022 - Flags: feedback?(alam)
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?
Attachment #8486022 - Flags: feedback?(alam) → feedback-
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.
(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?
Flags: needinfo?(ywang)
Flags: needinfo?(alam)
(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.
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 on attachment 8486022 [details]
Three page actions

f+ for the way the page actions are arranged, which was a concern I had.
Attachment #8486022 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(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.
Flags: needinfo?(alam)
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?).
Created attachment 8489590 [details] [diff] [review]
Part 1: Don't hide the tabs tray button when opening the tabs tray on new tablet
Attachment #8489590 - Flags: review?(lucasr.at.mozilla)
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!
Flags: needinfo?(ywang)
Created attachment 8489612 [details]
(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.
Attachment #8489612 - Flags: feedback?(alam)
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!
Attachment #8489612 - Flags: feedback?(alam) → feedback-
Duplicate of this bug: 1058322
Created attachment 8489648 [details]
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.
Attachment #8480237 - Attachment is obsolete: true
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.
Attachment #8489590 - Flags: review?(lucasr.at.mozilla) → review+
Created attachment 8490492 [details]
(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.
Attachment #8490492 - Flags: feedback?(alam)
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..
Flags: needinfo?(michael.l.comella)
(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!
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
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...
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Created attachment 8490964 [details]
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).
Attachment #8490492 - Attachment is obsolete: true
Attachment #8490492 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
Whiteboard: [leave-open]
Created attachment 8491122 [details] [diff] [review]
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?
Attachment #8491122 - Flags: feedback?(lucasr.at.mozilla)
Created attachment 8491841 [details] [diff] [review]
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?
Attachment #8491841 - Flags: feedback?(lucasr.at.mozilla)
Created attachment 8491843 [details]
(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).
Attachment #8491843 - Flags: feedback?(alam)
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.
Attachment #8491843 - Flags: feedback?(alam) → feedback+
Created attachment 8491867 [details] [diff] [review]
Part 4: Adjust forward button position and spacing

Complete, besides rebasing.
Attachment #8491867 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8491869 - Flags: feedback?(alam)
Attachment #8489612 - Attachment is obsolete: true
Attachment #8490964 - Attachment is obsolete: true
Attachment #8486022 - Attachment is obsolete: true
(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 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.
Attachment #8491867 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #8491122 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #8491841 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Anthony, thoughts on comment 38?
Flags: needinfo?(alam)
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).
Attachment #8492316 - Flags: feedback?(lucasr.at.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
Created attachment 8492334 [details]
spec_structure_forwardbutton1.png

Alas! moar specs!

Hope this helps, Michael.
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Also, comment 44 Anthony. Thanks!
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
Not sure what you mean by "old favicons". 

Do you mean dimensions or design?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(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
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
Both dimensions and design - can we use one set of the images without any noticeable issues between old tablet and new?
(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
(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?
Flags: needinfo?(lucasr.at.mozilla)
(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
Flags: needinfo?(alam)
Ah - I was just dropped them in, assuming they were different. Are there any other assets that were reused?
Flags: needinfo?(alam)
(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.
Flags: needinfo?(lucasr.at.mozilla)
(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.

Updated

4 years ago
Attachment #8492316 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Created attachment 8493316 [details] [diff] [review]
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.
Attachment #8493316 - Flags: review?(lucasr.at.mozilla)
Attachment #8491122 - Attachment is obsolete: true
Created attachment 8493320 [details] [diff] [review]
Part 3: Adjust new tablet browser_toolbar spacing

Filed bug 1071226 for refining private browsing mode.
Attachment #8493320 - Flags: review?(lucasr.at.mozilla)
Attachment #8491841 - Attachment is obsolete: true
Created attachment 8493346 [details]
New tablet browser toolbar
Attachment #8491843 - Attachment is obsolete: true
Attachment #8491869 - Attachment is obsolete: true
Attachment #8491869 - Flags: feedback?(alam)
Attachment #8493346 - Flags: feedback?(alam)
Attachment #8492316 - Attachment is obsolete: true
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?
Attachment #8493316 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8493320 - Flags: review?(lucasr.at.mozilla)
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?
(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.
Created attachment 8494165 [details]
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
Attachment #8494165 - Flags: feedback?(alam)
(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.
Flags: needinfo?(alam)
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
Attachment #8494165 - Flags: feedback?(alam) → feedback-
(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 on attachment 8493346 [details]
New tablet browser toolbar

This looks nice on top on content! But we can still try 56 first
Attachment #8493346 - Flags: feedback?(alam) → feedback+
(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
Created attachment 8494813 [details] [diff] [review]
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
Attachment #8494813 - Flags: review?(lucasr.at.mozilla)
Attachment #8493320 - Attachment is obsolete: true
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?
Attachment #8494813 - Flags: review?(lucasr.at.mozilla) → review+
Created attachment 8494817 [details] [diff] [review]
Part 4: Adjust forward button position and spacing
Attachment #8494817 - Flags: review?(lucasr.at.mozilla)
Attachment #8491867 - Attachment is obsolete: true
Attachment #8494165 - Attachment is obsolete: true
Comment on attachment 8494817 [details] [diff] [review]
Part 4: Adjust forward button position and spacing

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

Good.
Attachment #8494817 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Created attachment 8494822 [details] [diff] [review]
Part 3: Adjust new tablet browser_toolbar spacing

Updated for nits.
Attachment #8494813 - Attachment is obsolete: true
Created attachment 8494824 [details] [diff] [review]
Part 4: Adjust forward button position and spacing

Rebase.
Attachment #8494817 - Attachment is obsolete: true
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/3e1ec0a3b696
https://hg.mozilla.org/mozilla-central/rev/540bc6af7020
https://hg.mozilla.org/mozilla-central/rev/92d6256f69e9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35

Updated

4 years ago
Depends on: 1073449
status-firefox35: --- → fixed
Flags: qe-verify+

Comment 78

4 years ago
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
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.