1132751 - Fix Android L Settings page's build icon size

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: N6 Screenshot_2015-02-12-15-09-25.png

Is it me or is this enormous now after the patch?

Comment 1

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

Attachment: Chrome settings (L)

Worth noting that other applications (I checked Chrome & gmail) do not put the application icon in their settings menus though they did in JB.

Do you still want to include the icon, Anthony? I like it as a way of branding.

I think the icon might be larger because the ActionBar height may have increased - I'm looking for a way to style the icon without writing a custom ActionBar layout.

Comment 2

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

Attachment: Settings (JB)

(In reply to Michael Comella (:mcomella) from comment #1)
> I think the icon might be larger because the ActionBar height may have
> increased

Actually, it looks like it might be the same size - the changed layout just makes it look funky.

If we want to change the icon size, I think we have a few options:
  * Custom layout (too much work :|)
  * New assets (too much memory)
  * Inset drawable as the icon

We can use android:logo, rather than android:icon, as an element that only gets put into the ActionBar.

Comment 3

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

Attachment: Local build settings (L)

For comparison's sake.

Comment 4

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

Anthony, do we want to keep the icon the same size in the non-Android L builds?

Comment 5

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

Attachment: Post patch (6dp)

I added 6dp of padding in all directions on the icon.

Anthony, what do you think?

Comment 6

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

Comment on attachment 8585796 [details]
Post patch (6dp)

WFM! 

what size does that make this icon?

Also, what's the size of the margin between the <- and the build icon?

Comment 7

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

I'm open to keeping it for now.

Comment 8

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

(In reply to Anthony Lam (:antlam) from comment #6)
> what size does that make this icon?
> 
> Also, what's the size of the margin between the <- and the build icon?

I tried digging into the source but it's non-obvious.

For anyone following in my footsteps, it's somewhere around:
  * screen_action_bar.xml: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/res/res/layout/screen_action_bar.xml
  * action_bar_home.xml: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/res/res/layout/action_bar_home.xml
  * ActionBarView.java: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/com/android/internal/widget/ActionBarView.java#63
  * Widget.ActionBar: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/res/res/values/styles.xml#1193

Comment 9

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

Attachment: MozReview Request: bz://1132751/mcomella

/r/6381 - Bug 1132751 - Remove redundant ActionBar home setting. r=liuche
/r/6383 - Bug 1132751 - Add android:logo to fennec application. r=liuche

Pull down these commits:

hg pull -r 93722e1935fa363e527e230b5498339f93f2f911 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8586371 [details]
MozReview Request: bz://1132751/mcomella

https://reviewboard.mozilla.org/r/6379/#review5339

Comment 11

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/6381/#review5335

::: mobile/android/base/preferences/GeckoPreferences.java
(Diff revision 1)
> -                actionBar.setHomeButtonEnabled(true);

For some reason I thought this was related to some v14+ bug:
https://code.google.com/p/android/issues/detail?id=58007

But this doesn't even seem to be the fix, so if the "back" button works on, say, v14 devices for going "up", feel free to remove it.

Comment 12

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/6383/#review5337

::: mobile/android/base/resources/drawable/logo.xml
(Diff revision 1)
> +    android:src="@drawable/icon"/>

Was the sizing of the logo fine on other devices? It looked pretty big on JB too.

Comment 13

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

https://reviewboard.mozilla.org/r/6383/#review5341

> Was the sizing of the logo fine on other devices? It looked pretty big on JB too.

Anthony okayed this in comment 7.

Comment 14

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

https://reviewboard.mozilla.org/r/6381/#review5343

> For some reason I thought this was related to some v14+ bug:
> https://code.google.com/p/android/issues/detail?id=58007
> 
> But this doesn't even seem to be the fix, so if the "back" button works on, say, v14 devices for going "up", feel free to remove it.

The up button works as expected on my 4.4 N7 and my 5.0 N4 after these changes.

Comment 15

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

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

Comment 16

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

Comment on attachment 8586371 [details]
MozReview Request: bz://1132751/mcomella

Approval Request Comment
[Feature/regressing bug #]: bug 1097337
[User impact if declined]:
  Users will see an awkwardly large Firefox asset (specific to build type) in the Settings menu (i.e. polish)

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
  Short answer: Android L users may see a smaller application logo in some places.
  Long answer: We add a new attribute to the base level of the application, android:logo. This appears to only be used in the ActionBar which, to my knowledge, we only use in the Settings menu. On Android L+, this icon has some extra padding over the standard application icon (i.e. the firefox logo) which means it'll appear smaller in places where android:logo is used in the place of android:icon. Again, I think this is just ActionBar but if I'm wrong, Android L+ users may see a smaller application logo in those places.

[String/UUID change made/needed]: None

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/ae9a28f9f736
https://hg.mozilla.org/mozilla-central/rev/e9e89c1c7e41

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ad0c7cad3bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/445036402091

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8be55cae236f
https://hg.mozilla.org/releases/mozilla-beta/rev/b28b502e4aca

Comment 20

[Catalin Suciu [:csuciu]]

Verified as fixed on Nexus 7 (5.0.2) on Firefox 38 Beta 2 and latest Aurora 39.0a2 , Nightly 40.0a1

Comment 22

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

Attachment: MozReview Request: Bug 1132751 - Remove redundant ActionBar home setting. r=liuche

Comment 23

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

Attachment: MozReview Request: Bug 1132751 - Add android:logo to fennec application. r=liuche