Reload is too small and the tabs panel button is the incorrect color.

NI :antlam for the assets.

This may also include the url bar entry, as per bug 1058909 comment 65 and subsequent comments.

Include the url bar entry.

Here's the tab icon in parts, adjusted the color so it should match now.

Here's the URL 9 patch files.

Lucas, we currently have 9 patch files which are used for both phone and tablet. The assets in comment 6 add tablet assets specifically for tablet - is this desireable? I'm not sure what the tradeoff here is between size and non-scaled assets.

(In reply to Michael Comella (:mcomella) from comment #7) > Lucas, we currently have 9 patch files which are used for both phone and > tablet. The assets in comment 6 add tablet assets specifically for tablet - > is this desireable? I'm not sure what the tradeoff here is between size and > non-scaled assets. I think having different assets between phones and tablets is a valid design choice. The constraints are different for phones and tablets and this often means applying different visual design. What I'd like to avoid though is having one asset for old and another for new tablet UI. I assume this new image works fine in the old tablet UI? Maybe post screenshots for antlam's feedback?

Comment on attachment 8494871 [details] [diff] [review] Part 1: Update tabs panel button asset Review of attachment 8494871 [details] [diff] [review]: ----------------------------------------------------------------- Good. Please run something like trimage (see trimage.org) on these new images before landing.

(In reply to Lucas Rocha (:lucasr) from comment #9) > Good. Please run something like trimage (see trimage.org) on these new > images before landing. Doing it because you asked :D, but I filed bug 1073125 to make sure we don't miss anything.

Updating with new 9 patch files for XXHDPI and XHDPI URL bar. These match the thinner line around the back button better. I had forgotten to update the stroke in the design file after realizing the nexus 7 takes XHDPI asset and not XXHDPI. Let's try these and test it out on a build Michael?

Comment on attachment 8495561 [details] Thinner border I think this is better. The stroke doesn't need to be any more prominent, and it feels lighter/ less cramped overall. Thoughts, Lucas?

If you like the attachment in comment 12, Lucas, here is the patch.

Assets in patch in comment 14 are trimage'd.

Comment on attachment 8495604 [details] [diff] [review] Part 2: Update url bar entry asset Review of attachment 8495604 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/base/resources/values-large-v11/dimens.xml @@ +7,5 @@ > > <dimen name="browser_toolbar_height">56dp</dimen> > <dimen name="browser_toolbar_button_padding">16dp</dimen> > > + <dimen name="nav_button_border_width">1dp</dimen> Isn't this defined in values/dimens.xml already?

(In reply to Anthony Lam (:antlam) from comment #13) > Comment on attachment 8495561 [details] > Thinner border > > I think this is better. The stroke doesn't need to be any more prominent, > and it feels lighter/ less cramped overall. > > Thoughts, Lucas? Thumbs up!

Updated for nits.

https://hg.mozilla.org/integration/fx-team/rev/e6e9519087cd https://hg.mozilla.org/integration/fx-team/rev/163110429e09

Leave open to investigate the smaller reload button.

https://hg.mozilla.org/mozilla-central/rev/e6e9519087cd https://hg.mozilla.org/mozilla-central/rev/163110429e09

Comment on attachment 8496915 [details] [diff] [review] Part 3: Fix reload button padding in new tablet browser toolbar Review of attachment 8496915 [details] [diff] [review]: ----------------------------------------------------------------- I thought we had agreed to make the tap area bigger than the visual pressed state? This means you shouldn't really be padding the toolbar buttons. You should probably be using a shape drawable with a rounded rectangle with padding (in the drawable itself). Didn't have a separate bug to show rounded pressed state on toolbar buttons? ::: mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml @@ +58,1 @@ > android:layout_marginLeft="6dp" What is this marginLeft about? ::: mobile/android/base/resources/values-large-v11/styles.xml @@ +64,5 @@ > > <style name="Widget.MenuItemActionBar"> > + <item name="android:layout_width">wrap_content</item> > + <item name="android:layout_height">wrap_content</item> > + <item name="android:padding">@dimen/browser_toolbar_menu_item_padding</item> It looks like this change will break old tablet UI. This padding should only be applied to the new tablet UI, right?

(In reply to Lucas Rocha (:lucasr) from comment #23) > I thought we had agreed to make the tap area bigger than the visual pressed > state? This means you shouldn't really be padding the toolbar buttons. You > should probably be using a shape drawable with a rounded rectangle with > padding (in the drawable itself). Didn't have a separate bug to show rounded > pressed state on toolbar buttons? Sorry, I misread this. The padding is not used to make the hit area a different size from the button, it's used because apparently ImageButtons with match_parent height still only wraps content so I used the padding instead. I say we land like this (after fixing the other issues) if we're just going to overwrite this work in bug 1070087. > mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml > @@ +58,1 @@ > > android:layout_marginLeft="6dp" > > What is this marginLeft about? It's in the spec: https://bug1060413.bugzilla.mozilla.org/attachment.cgi?id=8490135 > It looks like this change will break old tablet UI. This padding should only > be applied to the new tablet UI, right? Yep, good call.

Removed the unintentional changes to old tablet.

Comment on attachment 8497166 [details] [diff] [review] Part 3: Fix reload button padding in new tablet browser toolbar Review of attachment 8497166 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'd like to see answers to these questions though. ::: mobile/android/base/resources/values-large-v11/dimens.xml @@ +14,5 @@ > > <dimen name="forward_default_offset">-13dip</dimen> > <dimen name="new_tablet_forward_default_offset">-6dp</dimen> > > + <dimen name="browser_toolbar_menu_item_padding">19dp</dimen> Shouldn't this be prefixed with new_tablet? ::: mobile/android/base/resources/values-large-v11/styles.xml @@ +72,5 @@ > > + <style name="Widget.MenuItemActionBar.NewTablet"> > + <item name="android:layout_width">wrap_content</item> > + <item name="android:layout_height">wrap_content</item> > + <item name="android:padding">@dimen/browser_toolbar_menu_item_padding</item> Why is the padding necessary if you're using gravity=center_vertical and scaleType=center? Is it to force a specific icon size on the view level instead of creating a separate icon for new tablets?

(In reply to Lucas Rocha (:lucasr) from comment #26) > > + <dimen name="browser_toolbar_menu_item_padding">19dp</dimen> > > Shouldn't this be prefixed with new_tablet? I thought we were only preferencing things that have an existing value for phone and/or old tablet, but it couldn't hurt. > ::: mobile/android/base/resources/values-large-v11/styles.xml > Why is the padding necessary if you're using gravity=center_vertical and > scaleType=center? Is it to force a specific icon size on the view level > instead of creating a separate icon for new tablets? Without padding, the View is only as large as the image - the padding increases the View size so that its tappable region fills the entire height of the toolbar and 56dp width.

Updated with new_tablet prefix.