402356 - netError (and others) use hard-coded colors for warnings

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Johnathan Nightingale [:johnath]]

As part of his review of bug 401575, bz pointed out that netError.css (in both pinstripe and winstripe themes) uses hardcoded colours in cases where it should probably be using system colours.  Specifically, the "securityOverrideContent" styles use a light yellow as a warning colour, hardcoded to #FFF090".  In attempting to find a system colour to use instead, I found that both the notification bar styling for warnings and the error console styling for warnings in the log use hardcoded values, each different though all approximately "pale yellow."

This seems like it could have accessibility repercussions, though in the netError case specifically, the background colour does not communicate any additional information directly (i.e. it is not necessary to notice the colour in order to properly use the page.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

It's not about whether the background color communicates information.  It's about the fact that the relevant style are:

112 #securityOverrideContent {
113   background-color: #FFF090; /* Pale yellow */

 and

 14 body {
 17   color: -moz-FieldText;

(this is in the winstripe version of netError.css).  Now there's no guarantee that that foreground color is different from #FFF090, since it's just a system color that depends on the OS theme.  So it's possible for all the text inside #securityOverrideContent to be invisible.

Setting just one of background and foreground without setting the other is just generally a bad idea...

Comment 2

[Damon Sicore (:damons)]

+'ing.  P3.

Comment 3

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with light gray foreground text

A screenshot on the Linux/GTK2 (Nodoka-Midnight theme) which shows the problem.
I can hardly read it.

Comment 4

[Takeshi Kurosawa :takenspc]

Attachment: workaround (setting the foreground color to black)

A workaround patch which sets the foreground color of |#securityOverrideContent| to black.

Comment 5

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with patch

Comment 6

[Magnus Melin [:mkmelin]]

color: "black"; ?

Comment 7

[Takeshi Kurosawa :takenspc]

Attachment: workaround #2 (setting the foreground color to black)

Oops!
A workaround patch fixes that typo.

Comment 8

[Takeshi Kurosawa :takenspc]

Attachment: Patch for netError.css (winstripe/gnomestripe)

Patch for netError.css (winstripe/gnomestripe).
# notificationbar uses the combination of InfoBackground/Infotext too.

Comment 9

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with patch

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: screenshot with patch (normal theme)

Maybe it's just my monitor, or viewing angle, but I can hardly tell the difference between InfoBackground and white, unless I look very closely.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: screenshot with patch (normal theme)

Cropped out too much of the background color in the previous screenshot, makes it look strange.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 303998 [details] [diff] [review]
Patch for netError.css (winstripe/gnomestripe)

r=me on this, assuming beltzner is OK with the less differentiable color.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Another option would be to keep #FFF090 as the background, and hard-code a black foreground. Means it might look weird with strange native themes, but will guarantee that it will always be legible (for people that can read black-on-yellowish, granted - pretty sure that's a large majority of our users :).

Comment 14

[Takeshi Kurosawa :takenspc]

Attachment: Patch for netError.css #2

> I can hardly tell the difference between InfoBackground and white, unless I look very closely.

I added another border. It helps emphasizing |InfoBackground|

Comment 15

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with patch #2

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305062 [details]
screenshot with patch (normal theme)

uir=beltzner

Comment 17

[Reed Loden [:reed]]

Comment on attachment 303998 [details] [diff] [review]
Patch for netError.css (winstripe/gnomestripe)

This is a blocker, so no approval1.9 is required.

Comment 18

[Reed Loden [:reed]]

Comment on attachment 305158 [details]
Screenshot with patch #2

beltzner: which screenshot of the two do you like the best?

Comment 19

[Reed Loden [:reed]]

I'm going to sit on this one before landing it, just until beltzner has looked at the second screenshot and picked which one he likes better.

Comment 20

[Dão Gottwald [:dao]]

ThreeDShadow for the inner box looks cluttered. There should be no border or maybe ThreeDLightShadow, imho.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Let's just land the reviewed patch.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/themes/winstripe/global/netError.css 	1.9
mozilla/toolkit/themes/gnomestripe/global/netError.css 	1.4