1313840 - Notification bar buttons: active foreground color is white and clashes badly with the background

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P2)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: white-foreground-button.png

See screenshot

Comment 1

[Dão Gottwald [::dao]]

Pretty sure this used to work at some point. This might be our own regression or an incompatibility with current macOS versions.

Comment 2

[Markus Stange [:mstange]]

Yeah, starting with 10.10, regular buttons turn blue when pressed, and their text turns white. But for notification buttons, the text shouldn't turn white.

Comment 3

[Stefan [:stefanh]]

Attachment: One-liner (override button.css so the color doesn't change)

Comment 4

[Dão Gottwald [::dao]]

Comment on attachment 8828142 [details] [diff] [review]
One-liner (override button.css so the color doesn't change)

I don't feel strongly about it, but I think it would make more sense to use black rather than ButtonText, because @roundButtonPressedBackground@ is a hard-coded color as well. It might even be worth introducing @roundButtonColor@ for this (and using it in findBar.css too).

Comment 5

[Stefan [:stefanh]]

Yeah, using black for notification > button and notification > button:hover:active is probably better.

Comment 6

[Stefan [:stefanh]]

Attachment: Now with hardcoded black

I don't think it's worth adding a @roundButtonColor@, findbar buttons are slightly different in the sense that they're xul toolbarbuttons (and they don't need any color styling).

Comment 7

[Dão Gottwald [::dao]]

(In reply to Stefan [:stefanh] from comment #6)
> Created attachment 8828411 [details] [diff] [review]
> Now with hardcoded black
> 
> I don't think it's worth adding a @roundButtonColor@, findbar buttons are
> slightly different in the sense that they're xul toolbarbuttons (and they
> don't need any color styling).

Toolbarbuttons inherit the color by default, right? So it would still be wise to set a color to be on the safe side, and indeed toolkit/themes/osx/global/findBar.css does set a color (ButtonText, again black would be better).

Comment 8

[Stefan [:stefanh]]

(In reply to Dão Gottwald [:dao] from comment #7)
> and indeed
> toolkit/themes/osx/global/findBar.css does set a color (ButtonText, again
> black would be better).

Oh, you're right - I missed that. Then I guess @roundButtonColor@ makes sense. I'll put up a new patch tonight.

Comment 9

[Stefan [:stefanh]]

Attachment: Now with @roundButtonColor@ and findBar.css change

Comment 10

[Dão Gottwald [::dao]]

Comment on attachment 8828876 [details] [diff] [review]
Now with  @roundButtonColor@ and findBar.css change

nit: use black rather than #000

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b402de2f73c
Don’t use the ’ButtonText’ foreground color in findbar/notification buttons and make text color in pressed notification buttons black.r=dao.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b402de2f73c