Closed
Bug 1072466
Opened 10 years ago
Closed 10 years ago
Update new tablet assets
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(7 files, 3 obsolete files)
(deleted),
application/zip
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/zip
|
Details | |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Reload is too small and the tabs panel button is the incorrect color.
Assignee | ||
Comment 1•10 years ago
|
||
NI :antlam for the assets.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Flags: needinfo?(alam)
Assignee | ||
Comment 2•10 years ago
|
||
This may also include the url bar entry, as per bug 1058909 comment 65 and subsequent comments.
Assignee | ||
Comment 3•10 years ago
|
||
Include the url bar entry.
Summary: Update new tablet reload and tabs panel button assets → Update new tablet assets
Comment 4•10 years ago
|
||
Here's the tab icon in parts, adjusted the color so it should match now.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8494871 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8495561 -
Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
If you like the attachment in comment 12, Lucas, here is the patch.
Attachment #8495604 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 15•10 years ago
|
||
Assets in patch in comment 14 are trimage'd.
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
(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)
Assignee | ||
Comment 18•10 years ago
|
||
Updated for nits.
Assignee | ||
Updated•10 years ago
|
Attachment #8495604 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8496114 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Leave open to investigate the smaller reload button.
Whiteboard: [leave-open]
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8496915 -
Flags: review?(lucasr.at.mozilla)
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
Removed the unintentional changes to old tablet.
Attachment #8497166 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8496915 -
Attachment is obsolete: true
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
(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.
Assignee | ||
Comment 28•10 years ago
|
||
Updated with new_tablet prefix.
Assignee | ||
Updated•10 years ago
|
Attachment #8497166 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8497678 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Whiteboard: [leave-open]
Comment 30•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•