Closed Bug 1130850 Opened 5 years ago Closed 5 years ago

Windows: outdated plugin notification bar text difficult to read

Categories

(Firefox :: Theme, defect)

All
Windows 8.1
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox36 --- unaffected
firefox37 + verified
firefox38 + verified
firefox39 --- verified

People

(Reporter: soeren.hentzschel, Assigned: adw)

References

Details

(Keywords: access, regression)

Attachments

(2 files, 1 obsolete file)

Attached image screenshot
Black text on a dark grey background is really difficult to read.
Points: --- → 3
Flags: qe-verify?
Flags: in-testsuite-
Flags: firefox-backlog+
Keywords: access
Do you know if this is a regression?
Flags: needinfo?(cadeyrn)
Unfortunately I don't know, I'am usally OS X user.
Flags: needinfo?(cadeyrn)
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.
I wonder if this is bug 1120716... that's a lot of !important to be sprinkling about. :-\
[Tracking Requested - why for this release]:
We shouldn't have unreadable security vulnerability warnings.
Drew, do you have cycles to pick this up? It looks to me like it's caused by bug 1120716.
Blocks: 1120716
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(adw)
Yikes, that is bad.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Iteration: --- → 38.3 - 23 Feb
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8568158 - Flags: review?(bmcbride)
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 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.
Attachment #8568158 - Flags: review?(bmcbride) → review-
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Duplicate of this bug: 1139210
Attached patch patch 2Splinter Review
Attachment #8568158 - Attachment is obsolete: true
Attachment #8572883 - Flags: review?(bmcbride)
Attachment #8572883 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/c834ba946535
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
QA Contact: cornel.ionce
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
Attachment #8572883 - Flags: approval-mozilla-beta?
Attachment #8572883 - Flags: approval-mozilla-aurora?
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+
Flags: needinfo?(florin.mezei)
Attachment #8572883 - Flags: approval-mozilla-beta?
Attachment #8572883 - Flags: approval-mozilla-beta+
Attachment #8572883 - Flags: approval-mozilla-aurora?
Attachment #8572883 - Flags: approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
I've also completed the verification on:
- latest Aurora, build ID: 20150310004228
- Firefox 37 beta 4, build ID: 20150309191715.
You need to log in before you can comment on or make changes to this bug.