Closed Bug 1275880 Opened 8 years ago Closed 8 years ago

"Settings" header is incorrectly colored

Categories

(Firefox for Android Graveyard :: General, defect)

49 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox47 unaffected, firefox48 verified, firefox49 verified, fennec48+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified
fennec 48+ ---

People

(Reporter: TeoVermesan, Assigned: mcomella)

References

Details

(Keywords: regression)

Attachments

(6 files)

Steps to reproduce:
1. Go to Menu -> Settings

Expected results:
- Grey background and white text

Actual results:
- Grey background and black text

Note:
- on Moto X (Android 4.4) the text header is not visible: http://i.imgur.com/ShorO4K.png

-regression:
25-05 unaffected
26-05 affected

pushlog:http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5511d54a3f172c1d68f98cc55dce4de1d0ba1b51&tochange=8d0aadfe7da782d415363880008b4ca027686137

Bug 1268606 - URL bar text selection action bar is incorrectly colored
Blocks: 1268606
Also action bar is incorrectly colored.
Hi Margaret, this seems to be a concerning issue on Nightly. It was mentioned at the channel meeting this morning. Could you please help find an owner who can investigate? Thanks!
Flags: needinfo?(margaret.leibovic)
mcomella, can you look into this? Or maybe ahunt or liuche can help.
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
tracking-fennec: --- → 49+
Is this actually a regression from bug 1268606 if it doesn't affect 48? Or is that flag incorrect?
Flags: needinfo?(teodora.vermesan)
I'd guess it's a regression from bug 1268606.
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella)
Anthony, to keep things simple, after my patch, the settings screen on all devices will look like this (whereas it looked different on pre-Lollipop devices before). fwiw, it already looks like this on my GS5. Does that sound okay?
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #6)
> Created attachment 8756929 [details]
> Prefs: screenshot after patch
> 
> Anthony, to keep things simple, after my patch, the settings screen on all
> devices will look like this (whereas it looked different on pre-Lollipop
> devices before). fwiw, it already looks like this on my GS5. Does that sound
> okay?

Talked on IRC too, I'm ok with this.

Just to double check here. Did the Appbar color change?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(In reply to Anthony Lam (:antlam) from comment #7)
> Just to double check here. Did the Appbar color change?

What's the app bar?

That being said, my intent is to keep everything the same as it is on the latest Android devices. This includes the things broken in this bug (preferences action bar & text-selection action bar in web content body) as well as the bug I was originally solving (text-selection action bar in url bar).
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
The app bar is just the dark grey bar up top (https://www.google.com/design/spec/layout/structure.html#structure-app-bar) 

As long as that hasn't changed color from before, then its all good. I just wanted to double check.
Flags: needinfo?(alam)
(In reply to :Margaret Leibovic from comment #4)
> Is this actually a regression from bug 1268606 if it doesn't affect 48? Or
> is that flag incorrect?

status-firefox48: unaffected is correct. I cannot reproduce the issue on Firefox for Android 48.0a2 (2016-05-26).
Flags: needinfo?(teodora.vermesan)
Comment on attachment 8757465 [details]
MozReview Request: Bug 1275880 - Consolidate gecko preferences theme to one api level. r=liuche

https://reviewboard.mozilla.org/r/55902/#review52636

Asked about android: prefix IRL, summary is, AppCompat doesn't use some attributes with the android: prefix, but the one in this commit is actually removed in a later commit.
Attachment #8757465 - Flags: review?(liuche) → review+
Comment on attachment 8757466 [details]
MozReview Request: Bug 1275880 - Give preferences action bar the correct config & delete unused. r=liuche

https://reviewboard.mozilla.org/r/55904/#review52638

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 1)
> -     *
> -     * Declaring these attributes in XML does not work on some devices for an unknown reason
> -     * (e.g. the back button stops working or the logo disappears; see bug 1152314) so we
> -     * duplicate those attributes in code here. Note: the order of these calls matters.
> -     *
> -     * We keep the XML attributes because not all of these methods are available on pre-v14.

Great! Remove special-case code :)

::: mobile/android/base/resources/values-v21/styles.xml
(Diff revision 1)
>        <item name="android:textColor">#fff</item>
>     </style>
>  
> -   <style name="ActionBarThemeGeckoPreferences">
> -       <!-- Icon color -->
> -       <item name="colorControlNormal">#fff</item>

It's confusing that we used to have these around but apparently they're unused (since we don't have to add them back in).
Attachment #8757466 - Flags: review?(liuche) → review+
Comment on attachment 8757467 [details]
MozReview Request: Bug 1275880 - Correct action mode styling. r=liuche

https://reviewboard.mozilla.org/r/55906/#review52652
Attachment #8757467 - Flags: review?(liuche) → review+
https://reviewboard.mozilla.org/r/55904/#review52638

> It's confusing that we used to have these around but apparently they're unused (since we don't have to add them back in).

Actually, they're defined by the parent style: ThemeOverlay.AppCompat.Dark.ActionBar inherits from Base.ThemeOverlay.AppCompat.Dark.ActionBar which [sets colorControlNormal to the primary text color](http://androidxref.com/6.0.1_r10/xref/frameworks/support/v7/appcompat/res/values/themes_base.xml#612), which we [set to @color/primary_text](https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/mobile/android/base/resources/values/themes.xml#35), which eventually is found to be #999. I assume the title text follows the same style.
Comment on attachment 8757465 [details]
MozReview Request: Bug 1275880 - Consolidate gecko preferences theme to one api level. r=liuche

Approval Request Comment
[Feature/regressing bug #]: bug 1268606 (which was getting uplifted to fix older issues)
[User impact if declined]: Users will have inconsistent action bar coloring
[Describe test coverage new/current, TreeHerder]: Tested locally on my 5+ N9 & 4.4 GS4.
[Risks and why]: Low. For the most part we're just changing style code so in the worst case, we get the style wrong again (but it's unlikely to be as wrong as it is now).
[String/UUID change made/needed]: None
Attachment #8757465 - Flags: approval-mozilla-aurora?
 "Settings" header is correctly colored, so:
Verified as fixed using:
Devices: Moto X (Android 4.4), Nexus 7 (Android 5.1.1), One A2001 (Android 5.1.1), Nexus 5X (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-06-01)
Status: RESOLVED → VERIFIED
tracking-fennec: 49+ → 48+
Comment on attachment 8757465 [details]
MozReview Request: Bug 1275880 - Consolidate gecko preferences theme to one api level. r=liuche

This patch fixes an inconsistent action bar coloring regression. Take it in aurora.
Attachment #8757465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
"Settings" header is correctly colored, so:
Verified as fixed using:
Devices: Moto X (Android 4.4), One A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a2 (2016-06-06)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.