Closed Bug 1313840 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Themes, defect, P2)

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: nhnt11, Assigned: stefanh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

See screenshot
Pretty sure this used to work at some point. This might be our own regression or an incompatibility with current macOS versions.
OS: Unspecified → Mac OS X
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.
Priority: -- → P2
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #8828142 - Flags: review?(dao+bmo)
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).
Attachment #8828142 - Flags: review?(dao+bmo) → review+
Yeah, using black for notification > button and notification > button:hover:active is probably better.
Attached patch Now with hardcoded black (obsolete) — Splinter Review
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).
Attachment #8828142 - Attachment is obsolete: true
Attachment #8828411 - Flags: review?(dao+bmo)
(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).
(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.
Attachment #8828411 - Flags: review?(dao+bmo)
Attachment #8828411 - Attachment is obsolete: true
Attachment #8828876 - Flags: review?(dao+bmo)
Comment on attachment 8828876 [details] [diff] [review]
Now with  @roundButtonColor@ and findBar.css change

nit: use black rather than #000
Attachment #8828876 - Flags: review?(dao+bmo) → review+
Pushed by stefanh@inbox.com:
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.
https://hg.mozilla.org/mozilla-central/rev/8b402de2f73c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: