1085406 - Update new tablet reload assets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Since we currently use something traced from desktop with odd sizes (since bug 1079183), I believe the intent here is to use mobile-specific assets.

Comment 1

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

Yes, currently the icons math are just off. I'll need to clean up and round them to the closest, clean divisible value.

Note: Martyn also mentioned we might be able to get away with just using XXHDPI and HDPI drawables. That might be helpful in the future.

Comment 2

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

NI antlam for assets.

Comment 3

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

Attachment: icon_tablet_refresh2.zip

It would appear that this is no longer a pressing issue. Though, the OCD side of mine does still want to clean up our assets.

It's just XHDPI and MDPI assets that will change (but I have included the whole set). The difference is 2px max so it's not a pressing issue. Plus, I think we've tweaked some of the padding during our work week to compensate for current icons. So, we might come across issues from simply replacing the assets.

Still, attaching icons. Might make a good mentor bug.

Comment 4

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

Attachment: tablet_refresh.tar.gz

I compressed the assets.

Comment 5

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

Summary: You should replace the old refresh assets (which are inconsistent across different dpi levels) with the new assets (hopefully more consistent!). Then, as antlam mentions, we may have added padding to compensate for size inconsistencies so you should look for that padding, remove it, and post a screenshot for antlam to check over. He'll give some feedback and you can adjust the reload padding as necessary.

The existing icons can be found at mobile/android/base/resources/drawable-large-*-v11/new_tablet_ic_menu_reload.png.

Comment 6

[Christian Schmitz (:tynn)]

Attachment: Updated reload assets

I've replaced the assets and removed the padding. The reload icon now is centered vertically in the ActionBar. How should the padding be adjusted?

Comment 7

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

Comment on attachment 8660130 [details]
Updated reload assets

looks good, do you mind including a before and after since the change is rather hard to see?

Mike, can you help direct the padding here to make sure it's centered inside that "visual feedback" rounded square users' see when they press it?

Comment 8

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

(In reply to Anthony Lam (:antlam) from comment #7)
> looks good, do you mind including a before and after since the change is
> rather hard to see?
> 
> Mike, can you help direct the padding here to make sure it's centered inside
> that "visual feedback" rounded square users' see when they press it?

Christian, thanks for the patch! To meet ^, can you upload a shot of the asset before your change, and also when the reload button is pressed?

Comment 9

[Christian Schmitz (:tynn)]

Attachment: Old and updated assets

On the left you find the old version and the updated on the right.
I've adjusted the padding, so that the "visual feedback" of the reload button has the same height as the one of the tab counter. This is 1dp more than before but also means the icon is centered.
The third image shows the button with all vertical padding removed.

Comment 10

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

Comment on attachment 8660494 [details]
Old and updated assets

+ looks good!

(In reply to Christian from comment #9)
> Created attachment 8660494 [details]
> Old and updated assets
> 
> On the left you find the old version and the updated on the right.
> I've adjusted the padding, so that the "visual feedback" of the reload
> button has the same height as the one of the tab counter. 

Awesome! definitely want all the visual feedbacks the same size. and then we just center the icon within the action bar container.

> This is 1dp more
> than before but also means the icon is centered.

As long as it's all centered :)

> The third image shows the button with all vertical padding removed.

Definitely do not want to remove all the vertical padding

Comment 11

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

Looks like the UI changes are good so I'd be willing to review the patch whenever you're ready, Christian!

Comment 12

[Christian Schmitz (:tynn)]

Attachment: bug1085406.patch

Hi Michael,
I've uploaded a portable solution, which also works for any other icon like the bookmarks star.
If you prefer setting the padding explicitly to 21dp, I'll update the patch for that.

Comment 13

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

I'll get to this tomorrow.

Comment 14

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

Comment on attachment 8663310 [details] [diff] [review]
bug1085406.patch

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

Looks good locally! Just a quick question regarding the possibility of making this static.

::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java
@@ +103,5 @@
>  
>      @Override
>      public boolean addActionItem(final View actionItem) {
>          actionItemBar.addView(actionItem);
> +        actionItem.getLayoutParams().height = LayoutParams.MATCH_PARENT;

Can this be set in XML instead? I see the comment in the other file but there are obviously layout_width/height values in the styles so perhaps it was an oversight – did you try changing those?

If this doesn't work in XML, this should probably happen before addView (which might make a measure pass, I'm not sure – it probably waits for the application code to return though). I think the safest pattern here is getLayoutParams, make a change, setLayoutParams.

Comment 15

[Christian Schmitz (:tynn)]

Attachment: bug1085406.patch

android:layout_* attributes are only used when inflating a layout resource. I left these in there since the style is also referenced in layout files.

> Can this be set in XML instead?
In our case the view is created programatically and added to the layout. This should set the LayoutParams for us the first time, but can't handle XML styles.

> If this doesn't work in XML, this should probably happen before addView.
But I've changed the addView method to use width and height and set it before anything else.

Comment 16

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

Comment on attachment 8664857 [details] [diff] [review]
bug1085406.patch

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

Looks good! Thanks Christian!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f74986c29a5

Comment 17

[Pulsebot]

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

Comment 18

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

If you're looking for more, perhaps bug 1122072?

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3528ffa6c9b