Closed
Bug 1130850
Opened 10 years ago
Closed 10 years ago
Windows: outdated plugin notification bar text difficult to read
Categories
(Firefox :: Theme, defect)
Tracking
()
People
(Reporter: soeren.hentzschel, Assigned: adw)
References
Details
(Keywords: access, regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Unfocused
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Black text on a dark grey background is really difficult to read.
Updated•10 years ago
|
Reporter | ||
Comment 2•10 years ago
|
||
Unfortunately I don't know, I'am usally OS X user.
Flags: needinfo?(cadeyrn)
Comment 3•10 years ago
|
||
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.
status-firefox36:
--- → unaffected
status-firefox38:
--- → affected
Keywords: regression,
regressionwindow-wanted
Comment 4•10 years ago
|
||
I wonder if this is bug 1120716... that's a lot of !important to be sprinkling about. :-\
Comment 5•10 years ago
|
||
[Tracking Requested - why for this release]:
We shouldn't have unreadable security vulnerability warnings.
status-firefox37:
--- → affected
tracking-firefox37:
--- → ?
Comment 6•10 years ago
|
||
Keywords: regressionwindow-wanted
Comment 7•10 years ago
|
||
Drew, do you have cycles to pick this up? It looks to me like it's caused by bug 1120716.
Assignee | ||
Comment 8•10 years ago
|
||
Yikes, that is bad.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Updated•10 years ago
|
Iteration: --- → 38.3 - 23 Feb
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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-
Updated•10 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Updated•10 years ago
|
tracking-firefox38:
--- → +
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8568158 -
Attachment is obsolete: true
Attachment #8572883 -
Flags: review?(bmcbride)
Updated•10 years ago
|
Attachment #8572883 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Updated•10 years ago
|
QA Contact: cornel.ionce
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
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•10 years ago
|
||
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
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.
Description
•