Line 46 in update_status.css sets a background color on a table without verifying the font's color. This can result in a white line with white text that looks rather like an extra-wide divider, yet contains textual information that is unreadable to a user who has a theme with white text and black backgrounds. Not the kind of thing that Update Status should be doing, eh? ;=)

CommentFileSizeAuthor
#4 update_status_colors_0.patch632 bytescburschka
#1 update_status_colors.patch620 bytescburschka
updatestatus_screenie_css_problem.png58.83 KBsenpai

Comments

cburschka’s picture

cburschka
they
commented
Status: Active » Needs review
StatusFileSize
new update_status_colors.patch620 bytes

Would this be solved by adding a declaration to make the font itself dark? Does it only have to be added to tr.warning .version-recommended or also to tr.error?

I currently can't test it exhaustively since I deploy from CVS, but I've attached a patch that should set the text in the warning box to black (since its background is white) and that in the error box to a dark red, #800, since its background is a slightly reddish white (#fdd). If those colors should be different or there should be more declarations in other places, I'll make a new one.

This patch is rolled against HEAD. (This is also the only place where .version-recommended actually exists; apparently the DRUPAL-5 branch doesn't have that selector in its CSS at all)

dww’s picture

dww
we/he/they
commented

a) DRUPAL-5--2 now exists for the 5.x-2.* series (it was in HEAD for a while). It's 5.x-2.0-rc you filled the issue against, after all, it shouldn't be in DRUPAL-5 branch, since that's only for 5.x-1.* versions of anything...

b) i'm tempted to mark this "by design". see http://drupal.org/node/153783. there are other classes that only define background colors and not foreground colors, since these background colors provide meaning. if your theme chooses to override these, you should override the "zebra stripe" classes, too. not my problem. ;)

i'll leave this open for a little while in case others have a strong opinion, but i don't consider this a bug, and i doubt i'll commit a patch to "fix" it.

senpai’s picture

senpai commented

DWW wrote:

DRUPAL-5--2 now exists for the 5.x-2.* series (it was in HEAD for a while). It's 5.x-2.0-rc you filled the issue against, after all, it shouldn't be in DRUPAL-5 branch, since that's only for 5.x-1.* versions of anything...

DWW, Huh? I downloaded the 5.x-2.0rc candidate of Update Status, installed it on a 5.1 site, and instantly noticed this CSS error. Are you saying that this issue needs to be filed against a different version of Drupal, or a different version of Update Status? I don't understand.

Arancaytar wrote:

Would this be solved by adding a declaration to make the font itself dark? Does it only have to be added to tr.warning .version-recommended or also to tr.error?

I only noticed two classes in Update Status 2.0rc that were different from the 1.x version and caused this error.

table.update-status tr.warning,
table.update-status tr.warning table.version-recommended {
  color: black;
  background-color: white;
}

Arancaytar wrote:

I currently can't test it exhaustively since I deploy from CVS, but I've attached a patch that should set the text in the warning box to black (since its background is white) and that in the error box to a dark red, #800, since its background is a slightly reddish white (#fdd). If those colors should be different or there should be more declarations in other places, I'll make a new one.

That's really all this issue needs in order to be considered closed. Anytime a div or table's background color is set by a module's CSS, and the user's chosen font color is out of your control, a contrasting font color *must* be set. It's just basic human courtesy. Thanks for fixing this bug so quickly, Arancaytar!

cburschka’s picture

cburschka
they
commented
StatusFileSize
new update_status_colors_0.patch632 bytes

Whoops. I'd been completely oblivious to the existence of DRUPAL-5--2 so far.

Senpai, I believe dww meant I'd rolled against the wrong tag (HEAD), not that the bug had been filed in the wrong version. I'll make a new patch for DRUPAL-5--2.

I agree that when setting background colors in modules, foreground colors should be set as well. It's not like a theme can't override both if it wants. This way makes it easier to the theme designer to avoid mistakes: If they want to change the way these messages are displayed, they need to override both specifically; merely overriding the global colors won't do anything unintended to the message.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

@Arancaytar: thanks for clearing up Senpai's confusion -- you understood exactly what I was trying to say.

However, this needs work. Please read http://drupal.org/node/153783 before commenting here or posting another patch.

Thanks,
-Derek

senpai’s picture

senpai commented
Status: Needs work » Fixed

The linked node and resulting committed patch is the decidedly wrong way to fix this, but since the thing is already committed and closed, I ain't gonna argue. I just have to say though, that merlinofchaos is right about what I was saying regarding colors.

It's a moot point now that D6 is out. Cheers for that.

I'm signing off on this issue in order to close the chain.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.