Closed Bug 1215079 Opened 4 years ago Closed 4 years ago

Menu background colour is incorrect on Gingerbread devices

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox41 --- unaffected
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 + fixed
firefox45 --- wontfix
fennec 44+ ---

People

(Reporter: u421692, Assigned: mcomella)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

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 ?
Ouch. Maybe I should push bug 1209967 forward. This menu has to die.
Flags: needinfo?(michael.l.comella)
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
Flags: needinfo?(michael.l.comella) → needinfo?(s.kaspari)
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? ..?
Flags: needinfo?(s.kaspari)
tracking-fennec: --- → 44+
(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.
Assignee: nobody → michael.l.comella
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
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
(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?
Flags: needinfo?(mihai.g.pop)
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.
Flags: needinfo?(alam)
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)
Flags: needinfo?(mihai.g.pop)
(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?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Attached image Screenshot (toolbar_grey) (obsolete) —
(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.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
What about our tabs tray grey?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
I superimposed black text on this. Since black is the unexpected and less common color, I think this will work.

What say you, antlam?
Attachment #8682193 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attachment #8682232 - Flags: feedback?(alam)
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.
Attachment #8682232 - Flags: feedback?(alam) → feedback+
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.
Attachment #8682245 - Flags: review?(margaret.leibovic)
Attachment #8682245 - Flags: review?(margaret.leibovic) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/9f0a34db5ed1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Or using Theme.NoTitleBar, as per comment 2.
(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.
(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+.
Attachment #8682245 - Attachment description: MozReview Request: Bug 1215079 - Change gingerbread menu color to something readable. r=margaret → MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret
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 on attachment 8682245 [details]
MozReview Request: Bug 1215079 - Inherit from built-in styles on GB. r=margaret

rb didn't reflag. :(
Attachment #8682245 - Flags: review+ → review?(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
Attachment #8682245 - Flags: review?(margaret.leibovic) → review+
Crap – this patch does not work with ExternalIntentDuringPrivateBrowsingFragment extends DialogFragment since DialogFragment requires Theme.AppCompat.

I'll look into bug 1209967 or bug 1220309.
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.
Flags: needinfo?(michael.l.comella)
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/
Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

ExternalIntent... extends DialogFragment requires Theme.AppCompat, which we no
longer inherit from on GB.
Attachment #8691039 - Flags: review?(margaret.leibovic)
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.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8691039 [details]
MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

https://reviewboard.mozilla.org/r/26053/#review23433
Attachment #8691039 - Flags: review?(margaret.leibovic) → review+
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
Attachment #8682245 - Flags: approval-mozilla-aurora?
Attachment #8691039 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8682245 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting QE team to ensure there are no unexpected regressions.
Flags: qe-verify+
Comment on attachment 8691039 [details]
MozReview Request: Bug 1215079 - Don't open ExternalIntent... on GB. r=margaret

Aurora44+
Attachment #8691039 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ryan, will this get checked into aurora soon? It's negatively affecting GB users on that channel.
Flags: needinfo?(ryanvm)
I haven't been doing uplifts since the end of August.
Flags: needinfo?(wkocher)
Flags: needinfo?(ryanvm)
Flags: needinfo?(cbook)
I'm getting conflicts trying to land this on aurora. Can we get a rebased patch?
Flags: needinfo?(wkocher)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(cbook)
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 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/
Should be good! Thanks!
Flags: needinfo?(michael.l.comella) → needinfo?(wkocher)
Landed on 44 and wontfix on 45 – sounds like this is fixed. :)
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
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?
(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?
Flags: needinfo?(martrootamm)
(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.
Flags: needinfo?(martrootamm)
Hi,

Is the qe-verify+ flag still valid?

Thanks!
Flags: needinfo?(rkothari)
(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.
Flags: needinfo?(rkothari) → needinfo?(ioana.chiorean)
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.
Attached image Untitled.png
Flags: needinfo?(ioana.chiorean)
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!
Flags: qe-verify+
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.
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.
You need to log in before you can comment on or make changes to this bug.