1130850 - Windows: outdated plugin notification bar text difficult to read

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

Black text on a dark grey background is really difficult to read.

Comment 1

[:Gijs (he/him)]

Do you know if this is a regression?

Comment 2

[Sören Hentzschel]

Unfortunately I don't know, I'am usally OS X user.

Comment 3

[:Gijs (he/him)]

To quickly reproduce:

gPluginHandler.updateHiddenPluginUI(gBrowser.selectedBrowser, true, [{pluginName: "Test plugin", fallbackType: Ci.nsIObjectLoadingContent.PLUGIN_VULNERABLE_UPDATABLE}], gBrowser.contentPrincipal, gBrowser.contentWindow.location.hostname, gBrowser.contentWindow.location);

in the browser console.

This doesn't reproduce in 36 rc (to be released this week). Going to see where it does.

Comment 4

[:Gijs (he/him)]

I wonder if this is bug 1120716... that's a lot of !important to be sprinkling about. :-\

Comment 5

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
We shouldn't have unreadable security vulnerability warnings.

Comment 6

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0c2f7434c325&tochange=34a66aaaca81

Comment 7

[:Gijs (he/him)]

Drew, do you have cycles to pick this up? It looks to me like it's caused by bug 1120716.

Comment 8

[Drew Willcoxon :adw]

Yikes, that is bad.

Comment 9

[Drew Willcoxon :adw]

Attachment: patch

Gijs, I'm asking Blair to review since he reviewed the problematic CSS changes in bug 1120716, but you're welcome to review/feedback too if you'd like.

I tested this on Windows and OS X and it retains the desired link colors from bug 1120716 but doesn't mess up other colors like in this bug.

Comment 10

[Drew Willcoxon :adw]

If it's not clear, the !importants were to override global.css.  Moving the text-link selectors to browser.css seems to have the same effect but without the !importants, so I'm guessing browser.css is applied after global.css, but I don't know if that's the best solution.  Seems acceptable though.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8568158 [details] [diff] [review]
patch

Review of attachment 8568158 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Drew Willcoxon :adw from comment #10)
> so I'm guessing browser.css is applied after global.css

Ugh. So... apparently notification.css counts as a user-agent stylesheet, so it can only override global.css via !important. browser.css isn't a user-agent stylesheet, so can override global.css.

Using !important to override global.css is, sadly, a fact of life. We do it a lot.

::: browser/themes/windows/browser.css
@@ +2997,5 @@
>  }
> +
> +/* Text links in notification boxes */
> +
> +notification .messageText > .text-link {

Having these in browser is a footgun. Given it's fairly standard to use !important to override global.css for very specific things, I'd rather see this rule in notification.css (only need !important on the color property). And given notification.css is scoped to the XBL binding, you don't need "notification" in the selector.

@@ +3006,5 @@
> +notification[type="info"] .messageText > .text-link {
> +  color: inherit;
> +}
> +
> +notification[type="critical"] .messageText > .text-link {

Shouldn't need these additional rules - just the one above should apply to these other notification levels.

Comment 13

[Drew Willcoxon :adw]

Attachment: patch 2

Comment 14

[Drew Willcoxon :adw]

Thanks Blair.

https://hg.mozilla.org/integration/fx-team/rev/c834ba946535

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c834ba946535

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8572883 [details] [diff] [review]
patch 2

Approval Request Comment
[Feature/regressing bug #]:
bug 1120716

[User impact if declined]:
The outdated plugin notification bar text would be difficult to read.  See comment 0.

[Describe test coverage new/current, TreeHerder]:
Manual testing

[Risks and why]:
Low risk, only a CSS change, manually tested

[String/UUID change made/needed]:
None

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8572883 [details] [diff] [review]
patch 2

This may be a CSS only change but there is still risk of regressing some other notification. (Perhaps not a lot of risk but I'm going to flag QE anyway.) I'm approving this fix for the regression that was introduced in 37 for Beta 4. Beta+ Aurora+

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Confirming the fix on Latest Nightly, build ID: 20150308030227.

The text is now white and easily readable.
Exploratory testing was performed around several other notifications to ensure no issues were caused by this fix.
Filled bug 1141061 for the close button (which should also be inverted).

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d0326e1f387

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d241bf1edd8

Comment 21

[Cornel Ionce [:noni] [Hubs QA]]

I've also completed the verification on:
- latest Aurora, build ID: 20150310004228
- Firefox 37 beta 4, build ID: 20150309191715.