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

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Themes
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: nhnt11, Assigned: stefanh)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8805752 [details]
white-foreground-button.png

See screenshot
(Reporter)

Updated

2 years ago
Blocks: 1313838
Pretty sure this used to work at some point. This might be our own regression or an incompatibility with current macOS versions.
Keywords: regressionwindow-wanted
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.

Updated

2 years ago
Keywords: regressionwindow-wanted
Priority: -- → P2
(Assignee)

Comment 3

2 years ago
Created attachment 8828142 [details] [diff] [review]
One-liner (override button.css so the color doesn't change)
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+
(Assignee)

Comment 5

2 years ago
Yeah, using black for notification > button and notification > button:hover:active is probably better.
(Assignee)

Comment 6

2 years ago
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).
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).
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8828411 - Flags: review?(dao+bmo)
(Assignee)

Comment 9

2 years ago
Created attachment 8828876 [details] [diff] [review]
Now with  @roundButtonColor@ and findBar.css change
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+

Comment 11

2 years ago
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.

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b402de2f73c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
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.