1215079 - Menu background colour is incorrect on Gingerbread devices

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

Attachment: device-2015-10-15-142104.png

Open menu

Result: see attached screenshot

good:2015-10-08
bad:2015-10-09

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a955ea9382afedd66ea0da21fcd2932465168d84&tochange=d01dd42e654b8735d86f9e7c723cc869a3b56798

Regressed from bug 1201206 ?

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Ouch. Maybe I should push bug 1209967 forward. This menu has to die.

Comment 2

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

My commit in bug 1201206 comment 22 set the background color because the panel is transparent by default in AppCompat [1]. I chose a dark color because the Samsung device I was using had light colored text, but I was afraid that wouldn't be a general solution.

I see a few solutions:
  * Sebastian's suggestion below – I like this the most, if we don't care about GB platform conventions. However, it could be a bit more work and I have questions if it's possible (see below).
  * Use android:Theme.NoTitleBar, as suggested by one of the answers in [1]. However, there will probably be more fallout from changing the colors.

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> Ouch. Maybe I should push bug 1209967 forward. This menu has to die.

Can we override the system menu on GB? Do you have opinions on the best approach? If not, I'd like to pursue this.

[1]: http://stackoverflow.com/a/31777488

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

There's no official Android way of styling the system menu items afaik. I remember seeing hacks on StackOverflow when GB menus where still a thing. It looked painful and I never did this myself.

> My commit in bug 1201206 comment 22 set the background color because the panel is transparent by default in AppCompat [1].

How about we look into GB if we can find a color that defines the menu background color and then reference this (@android:color/super_official_background_color). This should always point to the correct one, right?

(In reply to Michael Comella (:mcomella) from comment #2)
> Can we override the system menu on GB? Do you have opinions on the best
> approach? If not, I'd like to pursue this.

I can't follow. What do you want to override? Color? Styles? Layout? ..?

Comment 4

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

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> > Can we override the system menu on GB? Do you have opinions on the best
> > approach? If not, I'd like to pursue this.
> 
> I can't follow. What do you want to override? Color? Styles? Layout? ..?

I interpreted comment 1 to say we kill the standard GB menus in favor of displaying the HC+ menu when the menu button is hit. However, I wasn't sure if it was possible to override the action taken when the menu button is hit on GB. Is that possible?

Then I was asking your opinion on how we should fix this bug – I think you commented to this effect in comment 3 (i.e. we should look for an official color).

If you didn't have an opinion on the best approach, I'd like to move ahead with bug 1209967.

Comment 5

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

I found the official asset [1], declared at [2]. The official color is #191919, which is still a dark grey.

Margaret suggested trying to change the font color.

[1]: http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/res/res/drawable-xhdpi/menu_background.9.png
[2]: http://androidxref.com/5.1.1_r6/xref/frameworks/base/core/res/res/values/themes.xml#229

Comment 6

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

At [1] I discovered we don't extend AppCompatActivity but we can't because the menu doesn't open on GB (known issue). This could be the cause for this bug.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1220196#c3

Comment 7

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

(In reply to Michael Comella (:mcomella) from comment #5)
> The official color is #191919, which is still a dark grey.

We are currently using #1b1b1b.

> Margaret suggested trying to change the font color.

I have not been successful, perhaps because we're not extending AppCompatActivity.

Without doing that, the easiest solution is to pick a background color that both white & black look good on, though our disabled buttons are also grey so we need to be careful.

Notably, the screenshot in comment 0 doesn't appear to have a disabled text color (on about:firefox, "Share", "Add to Reading List", and a few others should be disabled) so something weird might be going on here.

Mihai, do you remember which device you repro'd this on?

Comment 8

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

Anthony, given my thoughts to use a single background color, do you know of a color that would work well with the following font colors? #000000, #C8C8C8, & #8D8D8D. The latter two are standard and the #000000 is an outlier so the background should look better with the latter two.

Comment 9

[u421692]

Attachment: samsung galaxy r screenshot

I've reproduced this on HTC Desire HD (Android 2.3.5), but also reproduces on HTC Desire S (Android 2.3.3) and Motorola Droid Pro (Android 2.3.4). 
On Samsung Galaxy R (Android 2.3.4) it looks the same on all branches(see attached screenshot)

Comment 10

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

(In reply to Michael Comella (:mcomella) from comment #8)
> Anthony, given my thoughts to use a single background color, do you know of
> a color that would work well with the following font colors? #000000,
> #C8C8C8, & #8D8D8D. The latter two are standard and the #000000 is an
> outlier so the background should look better with the latter two.

suggestion off the top of my head, but why don't we use our toolbar grey?

Comment 11

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

Attachment: Screenshot (toolbar_grey)

(In reply to Anthony Lam (:antlam) from comment #10)
> suggestion off the top of my head, but why don't we use our toolbar grey?

Looks a bit bright to me – perhaps we should use a color, rather than just a shade of grey? This only affects GB so I'd be fine using something imperfect.

Comment 12

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

What about our tabs tray grey?

Comment 13

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

Attachment: Screenshot (text_and_tabs_tray_grey)

I superimposed black text on this. Since black is the unexpected and less common color, I think this will work.

What say you, antlam?

Comment 14

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

Comment on attachment 8682232 [details]
Screenshot (text_and_tabs_tray_grey)

WFM. it's going to be hard to find a color that works well enough for black as well as these whites and light greys.

Comment 15

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

Attachment: MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Bug 1215079 - Change gingerbread menu color to something readable. r=margaret

Each device can have different color font - this is a best guess about making
it readable everywhere.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

https://reviewboard.mozilla.org/r/24019/#review21521

::: mobile/android/base/resources/values/themes.xml:18
(Diff revision 1)
> -        <item name="android:panelBackground">@color/gingerbread_menu_background_color</item>
> +        <item name="android:panelBackground">@color/text_and_tabs_tray_grey</item>

Maybe expand on this comment to explain how we can to using this color here.

Comment 17

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

https://hg.mozilla.org/integration/fx-team/rev/9f0a34db5ed12819912401e71ce0030279955c9d
Bug 1215079 - Change gingerbread menu color to something readable. r=margaret

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f0a34db5ed1

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9f0a34db5ed1

Comment 20

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

via bug 1201206 comment 44:

(In reply to Dominique Vincent [:domivinc] from comment #43)
> Michael, on my device (HTC Android 2.3), using Nightly, I still see the
> black text with a black background in the Firefox vertical menu. It seems
> that the following changes [1] didn't fix the issue in all the 2.3 cases.

This is unfortunate.

I think the only solution here is bug 1220309 which necessitates an older support library version (which can come with its own set of problems) or bug 1209967.

Comment 21

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

Or using Theme.NoTitleBar, as per comment 2.

Comment 22

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

(In reply to Michael Comella (:mcomella) from comment #21)
> Or using Theme.NoTitleBar, as per comment 2.

I realized this works because it's the built-in android:Theme.NoTitleBar as opposed to the AppCompat version. We'd prefer to use APpCompat (because, you know, that's kind of the point), but since AppCompat on older versions probably just reverts to GB styles anyway, we might be fine using android:Theme.NoTitleBar. It'd definitely be the simplest fix to uplift.

Comment 23

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

(In reply to Michael Comella (:mcomella) from comment #22)
> but since AppCompat on older versions
> probably just reverts to GB styles anyway

That is, we can use this style on v1-10 and not v11+.

Comment 24

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

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24019/diff/1-2/

Comment 25

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

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

rb didn't reflag. :(

Comment 26

[:Margaret Leibovic]

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

https://reviewboard.mozilla.org/r/24019/#review22753

Comment 27

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

Crap – this patch does not work with ExternalIntentDuringPrivateBrowsingFragment extends DialogFragment since DialogFragment requires Theme.AppCompat.

I'll look into bug 1209967 or bug 1220309.

Comment 28

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

Spoke with Margaret on Vidyo: we can use the patch that uses built-in styles on GB in 44 and disable ExternalIntentDuringPrivateBrowsingFragment on GB in 44 until bug 1209967 lands (hopefully 45?). NI self to get this rolling.

Comment 29

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

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24019/diff/2-3/

Comment 30

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

Attachment: MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

ExternalIntent... extends DialogFragment requires Theme.AppCompat, which we no
longer inherit from on GB.

Comment 31

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

These patches apply to 45. I tried to build on 44 directly, but I had some weird issues with Gecko not loading in an Aurora build so I was unable to test.

Comment 32

[:Margaret Leibovic]

Comment on attachment 8691039 [details]
MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

https://reviewboard.mozilla.org/r/26053/#review23433

Comment 33

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

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Note: these patches should only land on 44 – we want to land bug 1209967 on 45.

Approval Request Comment
[Feature/regressing bug #]: bug 1201206.
[User impact if declined]: Users on some GB devices will have black text on black background in their menu.

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Medium, we're inheriting from a new style sheet for the main Activity and there could be some unknown side effects as far as styles are concerned. They're unlikely to cause crashes, but they could look bad. Luckily, the styles used by the old style sheet tends to default to the system styles so I'm hopeful there will be no changes we didn't see. The code changes are pretty safe.

[String/UUID change made/needed]: None

Comment 34

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

As I said, we'll fix 45 with bug 1209967.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Margaret, Michael, given the medium risk, do you think it would make sense to get help from Mihai to test the fix on a try build before uplifting to Aurora? 

I am trying to determine whether we should do a bit of focused testing before uplifting or wait for any news of possible regressions to come in post-aurora uplifts. Please let me know.

Comment 36

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

I doubt there will be crashes and I've tested as many screens as I can think of (given previous similar style regressions from bug 1201206) on the patch on 45. My reason for giving it a medium is because there is a large unknown – the unintended style changes come out of nowhere. That being said, they're just style changes so it's not terrible if they slip through the cracks – I don't think testing on try is necessary.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Rendering black text on black background on Gingerbread devices is not great. Michael is confident this patch is safe, let's uplift to Aurora44.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Requesting QE team to ensure there are no unexpected regressions.

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8691039 [details]
MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

Aurora44+

Comment 40

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

Ryan, will this get checked into aurora soon? It's negatively affecting GB users on that channel.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

I haven't been doing uplifts since the end of August.

Comment 42

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

I'm getting conflicts trying to land this on aurora. Can we get a rebased patch?

Comment 43

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

Comment on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24019/diff/3-4/

Comment 44

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

Comment on attachment 8691039 [details]
MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26053/diff/1-2/

Comment 45

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

Should be good! Thanks!

Comment 46

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/04c3f8a3a7be
https://hg.mozilla.org/releases/mozilla-aurora/rev/7aad7dee8515

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/04c3f8a3a7be
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7aad7dee8515

Comment 48

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

Landed on 44 and wontfix on 45 – sounds like this is fixed. :)

Comment 49

[Mart Rootamm]

I'm at Firefox 43 and Gingerbread, but menu background colour has so far been iffy, in that there's black text on a relatively dark blue backround, as seen here:

https://en.wikipedia.org/wiki/File:NoScript_Anywhere_3.5a8_site_permissions_in_Firefox_38.0.5_on_Android_2.3.6.png

Are these related?

Comment 50

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

(In reply to Mart Rootamm from comment #49)
> I'm at Firefox 43 and Gingerbread, but menu background colour has so far
> been iffy, in that there's black text on a relatively dark blue backround,
> as seen here:

I don't believe so – I think this only affects the standard Android overflow menu and not the context menus.

We can't change this in 43 but if you can reproduce on Beta (44), can you file a separate bug?

Comment 51

[Mart Rootamm]

(In reply to Michael Comella (:mcomella) from comment #50)
> (In reply to Mart Rootamm from comment #49)
> > I'm at Firefox 43 and Gingerbread, but menu background colour has so far
> > been iffy, in that there's black text on a relatively dark blue backround,
> > as seen here:
> 
> I don't believe so – I think this only affects the standard Android overflow
> menu and not the context menus.
> 
> We can't change this in 43 but if you can reproduce on Beta (44), can you
> file a separate bug?

The standard Android overflow menu does appear when I need to change NoScript permissions, and that's where the menu colour is not legible enough.

Other Firefox menus are reasonably legible.

Comment 52

[Mira Lobontiu (Android QA)]

Hi,

Is the qe-verify+ flag still valid?

Thanks!

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to miralobontiu from comment #52)
> Hi,
> 
> Is the qe-verify+ flag still valid?
> 
> Thanks!
Yes, it is. Ioana, please correct me if I am wrong.

Comment 54

[Kevin Brosnan [Ex-Mozilla]]

qe-verify + means that this needs manual verification. I don't see why we would verify this bug for an Android OS version we have not supported in years.

Comment 55

[Mira Lobontiu (Android QA)]

Attachment: Untitled.png

Comment 56

[Mira Lobontiu (Android QA)]

Hi,

I`ve tested with Firefox Mobile Nightly 64.0a1 (2018-09-12) on Prestigio Grace X5 (Android 4.4.2), and the menu is displayed exactly as on Huawei Honor 8 (Android 7). I`ve attached a screenshot.

And, based on the Comment 54, I will remove the qe-verify+ flag.

Thank you!

Comment 57

[Kevin Brosnan [Ex-Mozilla]]

Testing with a current Firefox nightly is an invalid test as this bug was only in Gingerbread devices (Android 2.3). We have not supported that version of Android for a couple years.

Comment 58

[Ioana Chiorean]

Kevin is right. Sorry to see this so late - i was traveling for a work weekend. 
Also trying with an old Fennec on an old device that we do not support anymore brings no value back.