Closed Bug 1198415 Opened 4 years ago Closed 4 years ago

Search suggestions opt-in notification in the urlbar has low contrast, is hard to read

Categories

(Firefox :: Search, defect, P1)

43 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: tecgirl, Assigned: adw)

References

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(2 files, 3 obsolete files)

In a fresh install of Nightly 43.0a1, I was given this tooltip when entering into the URLbar. The light grey background makes it hard to read.
Robin, what OS are you using?  Do you use some kind of high-contrast mode or other non-standard OS theme?
Flags: needinfo?(randersen)
Priority: -- → P1
Whiteboard: [suggestions][fxsearch]
Blocks: 959567
Summary: Tooltip has low contrast, is hard to read → Search suggestions opt-in notification in the urlbar has low contrast, is hard to read
Assignee: nobody → adw
Status: NEW → ASSIGNED
Or a Firefox theme?  I couldn't reproduce this on Windows 7 with the built-in high-contrast OS themes or on OS X.  I only tested with the default Firefox theme though.
Flags: firefox-backlog+
Oh, it's lightweight themes.
Flags: needinfo?(randersen)
Attached patch use -moz-DialogText (obsolete) — Splinter Review
Not sure which color we should be using exactly, but this works.  Tested on Windows 7 in default and high-contrast themes and on OS X.
Attachment #8662121 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #1)
> Robin, what OS are you using?  Do you use some kind of high-contrast mode or
> other non-standard OS theme?
Flags: needinfo?(randersen)
If I'm not wrong, the autocomplete has background-color: -moz-Field; so likely this should use -moz-FieldText?
Attached patch use -moz-FieldText (obsolete) — Splinter Review
That works too.  It does look like autocomplete has a -moz-Field background, so -moz-FieldText would be the right color to use, but FWIW there's no visual difference that I can tell between -moz-FieldText and -moz-DialogText on Windows or OS X with non-standard Firefox and OS themes.
Attachment #8662121 - Attachment is obsolete: true
Attachment #8662121 - Flags: review?(mak77)
Attachment #8662487 - Flags: review?(mak77)
Rank: 10
Attachment #8662487 - Flags: review?(mak77) → review+
Robin, I'll leave the needinfo on you just to verify that you're using a non-default Firefox theme.
I am using the default theme.
Flags: needinfo?(randersen)
Hmm, interesting.  I can't explain then why your fresh install, as you say, looks like that, but the patch should fix it since it specifies a specific text color.  What OS and OS theme are you using?

Please try the new Nightly whenever this patch makes its way there and let us know whether it's still a problem.
https://hg.mozilla.org/mozilla-central/rev/9d4b08fb58f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Sorry, I'm backing this patch out because it made the text unreadable with the Ubuntu default theme: https://hg.mozilla.org/integration/fx-team/rev/d1ae9771dd06
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, here are the colors used on Windows, OS X, and Ubuntu (without the backed-out patch):

Opt-in background
-----------------
Windows, OS X: urlbarSearchSuggestionsNotification.inc.css (as expected)
Ubuntu: A dark brown color.  I couldn't find where this comes from.

Opt-in foreground
-----------------
Windows, Ubuntu: panel { color: MenuText } (popup.css)
OS X: window { color: -moz-DialogText } (global.css)

Richlistbox item background
---------------------------
Windows, OS X: Transparent "browser style", don't know where this comes from.
Ubuntu: A dark brown color.  I couldn't find where this comes from.

Richlistbox item foreground
---------------------------
Windows, OS X: richlistbox { color: -moz-FieldText } (richlistbox.css)
Ubuntu: .autocomplete-richlistitem { color: MenuText } (autocomplete.css)

Ubuntu styles both the opt-in and richlistbox item background colors with a dark brown, even though urlbarSearchSuggestionsNotification.inc.css hardcodes a background color for the opt-in.  I couldn't find where that color comes from using the browser toolbox.  So on Ubuntu, we should use the same color as the richlistbox items, MenuText.  MenuText also happens to be the color that the opt-in inherits.

MenuText happens to be black on Windows and OS X too, so we could use that everywhere.  But maybe it's better to keep using -moz-FieldText, like the richlistbox items on those platforms, so that's what this patch does.

I tested MenuText on Ubuntu with lightweight themes too and it looks fine.  The panel's colors don't change at all.

Also tested on Fedora and it looks fine.
Attachment #8662487 - Attachment is obsolete: true
Attachment #8663820 - Flags: review?(mak77)
Comment on attachment 8663820 [details] [diff] [review]
Windows, OS X: -moz-FieldText; Linux: MenuText

(In reply to Drew Willcoxon :adw from comment #14)
> Opt-in background
> -----------------
> Windows, OS X: urlbarSearchSuggestionsNotification.inc.css (as expected)
> Ubuntu: A dark brown color.  I couldn't find where this comes from.

From the panel: http://hg.mozilla.org/mozilla-central/annotate/197af2fb7e29/toolkit/themes/linux/global/popup.css#l18

> Ubuntu styles both the opt-in and richlistbox item background colors with a
> dark brown, even though urlbarSearchSuggestionsNotification.inc.css
> hardcodes a background color for the opt-in.

That background color is mostly transparent (7% opacity).

> color comes from using the browser toolbox.  So on Ubuntu, we should use the
> same color as the richlistbox items, MenuText.  MenuText also happens to be
> the color that the opt-in inherits.

So if it's already inherited, why did you add a rule for it in this patch?

>--- a/browser/themes/shared/urlbarSearchSuggestionsNotification.inc.css
>+++ b/browser/themes/shared/urlbarSearchSuggestionsNotification.inc.css
>@@ -1,10 +1,11 @@
> #PopupAutoCompleteRichResult > hbox[anonid="search-suggestions-notification"] {
>   border-bottom: 1px solid hsla(210, 4%, 10%, 0.14);
>+  color: -moz-FieldText;

Please move this to autocomplete.css where -moz-field is set as a background. (Except for Linux where -moz-field is set but not used.)
Attachment #8663820 - Flags: review?(mak77) → review-
Attached patch patch v3Splinter Review
(In reply to Dão Gottwald [:dao] from comment #15)
> From the panel:
> http://hg.mozilla.org/mozilla-central/annotate/197af2fb7e29/toolkit/themes/
> linux/global/popup.css#l18

The background color?  From the -moz-appearance?

> That background color is mostly transparent (7% opacity).

Oh, right.

> So if it's already inherited, why did you add a rule for it in this patch?

I thought it was necessary to override changes made by themes, which is the point of this bug.  But I tested just now without specifying a color at all, and that only seems to be a problem on OS X, where the inherited color is -moz-DialogText.  So lightweight themes can set -moz-DialogText but not MenuText?
Attachment #8663820 - Attachment is obsolete: true
Attachment #8663865 - Flags: review?(dao)
Comment on attachment 8663865 [details] [diff] [review]
patch v3

Let's see who gets to it first.
Attachment #8663865 - Flags: review?(mak77)
Comment on attachment 8663865 [details] [diff] [review]
patch v3

(In reply to Drew Willcoxon :adw from comment #16)
> Created attachment 8663865 [details] [diff] [review]
> patch v3
> 
> (In reply to Dão Gottwald [:dao] from comment #15)
> > From the panel:
> > http://hg.mozilla.org/mozilla-central/annotate/197af2fb7e29/toolkit/themes/
> > linux/global/popup.css#l18
> 
> The background color?  From the -moz-appearance?

yep

> > So if it's already inherited, why did you add a rule for it in this patch?
> 
> I thought it was necessary to override changes made by themes, which is the
> point of this bug.  But I tested just now without specifying a color at all,
> and that only seems to be a problem on OS X, where the inherited color is
> -moz-DialogText.  So lightweight themes can set -moz-DialogText but not
> MenuText?

Lightweight themes can't change those colors at all, plus Robin said she was using the default theme.

We still don't even know what OS she is on, so who knows if this patch will help in her case... but r+ because this change makes sense either way.
Attachment #8663865 - Flags: review?(mak77)
Attachment #8663865 - Flags: review?(dao)
Attachment #8663865 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/1e7f9effc62b

(In reply to Dão Gottwald [:dao] from comment #18)
> Lightweight themes can't change those colors at all, plus Robin said she was
> using the default theme.

True, although FWIW I was able to reproduce by using lightweight themes.
please remember we need to uplift this.
Target Milestone: Firefox 43 → Firefox 44
Robin, which OS are you using, and are you using a non-standard OS theme?
Flags: needinfo?(randersen)
https://hg.mozilla.org/mozilla-central/rev/1e7f9effc62b
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Comment on attachment 8663865 [details] [diff] [review]
patch v3

Approval Request Comment
[Feature/regressing bug #]: Search suggestions in the awesomebar
[User impact if declined]: The opt-in notification bar is unreadable with some theme combinations
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: low risk, trivial css change
[String/UUID change made/needed]: none
Attachment #8663865 - Flags: approval-mozilla-aurora?
Flags: needinfo?(randersen)
Robin, you didn't actually answer my questions.  Which OS are you using, and are you using a non-standard OS theme?
Flags: needinfo?(randersen)
(In reply to Drew Willcoxon :adw from comment #24)
> Robin, you didn't actually answer my questions.  Which OS are you using, and
> are you using a non-standard OS theme?

I am on Mac OS Yosemite 10.10.5 (14F27), and not using a non-standard OS theme.
Flags: needinfo?(randersen)
Thanks.  One more thing: now that this patch is on Nightly, could you please test again with the latest Nightly and let us know whether it's fixed?
Flags: needinfo?(randersen)
LGTM
Flags: needinfo?(randersen)
Status: RESOLVED → VERIFIED
Comment on attachment 8663865 [details] [diff] [review]
patch v3

Verified in Nightly; let's uplift to aurora.
Attachment #8663865 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.