Fix Android L Settings page's build icon size

VERIFIED FIXED in Firefox 38

Status

()

VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: antlam, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 verified, firefox39 verified, firefox40 verified, fennec38+)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8563763 [details]
N6 Screenshot_2015-02-12-15-09-25.png

Is it me or is this enormous now after the patch?
Blocks: 1030897
No longer blocks: 1098596
tracking-fennec: --- → ?
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 38+
Created attachment 8585762 [details]
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.
Flags: needinfo?(alam)
Attachment #8585762 - Attachment description: Chrome settings → Chrome settings (L)
Created attachment 8585769 [details]
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.
Created attachment 8585780 [details]
Local build settings (L)

For comparison's sake.
Anthony, do we want to keep the icon the same size in the non-Android L builds?
Created attachment 8585796 [details]
Post patch (6dp)

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

Anthony, what do you think?
Attachment #8585796 - Flags: feedback?(alam)
Attachment #8585780 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
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?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Attachment #8585796 - Flags: feedback?(alam) → feedback+
(Reporter)

Comment 7

4 years ago
I'm open to keeping it for now.
(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
Flags: needinfo?(michael.l.comella)
Created attachment 8586371 [details]
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/
Attachment #8586371 - Flags: review?(liuche)
Comment on attachment 8586371 [details]
MozReview Request: bz://1132751/mcomella

https://reviewboard.mozilla.org/r/6379/#review5339
Attachment #8586371 - Flags: review?(liuche) → review+
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.
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.
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.
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 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
Attachment #8586371 - Flags: approval-mozilla-beta?
Attachment #8586371 - Flags: approval-mozilla-aurora?
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ae9a28f9f736
https://hg.mozilla.org/mozilla-central/rev/e9e89c1c7e41
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
status-firefox38: --- → affected
status-firefox39: --- → affected
Attachment #8586371 - Flags: approval-mozilla-beta?
Attachment #8586371 - Flags: approval-mozilla-beta+
Attachment #8586371 - Flags: approval-mozilla-aurora?
Attachment #8586371 - Flags: approval-mozilla-aurora+
Verified as fixed on Nexus 7 (5.0.2) on Firefox 38 Beta 2 and latest Aurora 39.0a2 , Nightly 40.0a1
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified
status-firefox40: fixed → verified
Comment on attachment 8586371 [details]
MozReview Request: bz://1132751/mcomella
Attachment #8586371 - Attachment is obsolete: true
Attachment #8619472 - Flags: review+
Attachment #8619473 - Flags: review+
Created attachment 8619472 [details]
MozReview Request: Bug 1132751 - Remove redundant ActionBar home setting. r=liuche
Created attachment 8619473 [details]
MozReview Request: Bug 1132751 - Add android:logo to fennec application. r=liuche
You need to log in before you can comment on or make changes to this bug.