Closed Bug 1072466 Opened 5 years ago Closed 5 years ago

Update new tablet assets

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(7 files, 3 obsolete files)

3.86 KB, application/zip
Details
16.56 KB, application/zip
Details
5.95 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
10.31 KB, application/zip
Details
370.05 KB, image/png
antlam
: feedback+
Details
12.05 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
8.06 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Reload is too small and the tabs panel button is the incorrect color.
NI :antlam for the assets.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Flags: needinfo?(alam)
This may also include the url bar entry, as per bug 1058909 comment 65 and subsequent comments.
Include the url bar entry.
Summary: Update new tablet reload and tabs panel button assets → Update new tablet assets
Attached file icon_tabs_parts.zip
Here's the tab icon in parts, adjusted the color so it should match now.
Attached file 9 patch.zip
Here's the URL 9 patch files.
Flags: needinfo?(alam)
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.
Flags: needinfo?(lucasr.at.mozilla)
(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?
Flags: needinfo?(lucasr.at.mozilla)
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.
Attachment #8494871 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Attached file thinner_9patch.zip
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?
Flags: needinfo?(michael.l.comella)
Attached image Thinner border
Attachment #8495561 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
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?
Attachment #8495561 - Flags: feedback?(alam) → feedback+
Flags: needinfo?(lucasr.at.mozilla)
If you like the attachment in comment 12, Lucas, here is the patch.
Attachment #8495604 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8495604 - Flags: review?(lucasr.at.mozilla) → review+
(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!
Flags: needinfo?(lucasr.at.mozilla)
Attachment #8495604 - Attachment is obsolete: true
Leave open to investigate the smaller reload button.
Whiteboard: [leave-open]
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?
Attachment #8496915 - Flags: review?(lucasr.at.mozilla)
(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.
Attachment #8497166 - Flags: review?(lucasr.at.mozilla)
Attachment #8496915 - Attachment is obsolete: true
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?
Attachment #8497166 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Attachment #8497166 - Attachment is obsolete: true
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/911d32969372
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.