1044595 - [10.10] default buttons in dialogs etc. have black text instead of white

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[d.a.]

Attachment: Screen Shot 2014-07-27 at 7.31.56 PM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140727030204

Steps to reproduce:

Currently any button element is Firefox Nightly doesn't match the look in OS X Yosemite. Most noticeable are the "default" button and active state where the text color is black when really should be white. 

Attached are screenshots of how the buttons should look and how it currently looks in Firefox.

Comment 1

[d.a.]

Attachment: Current look in Firefox nightly

Comment 2

[:Gijs (he/him)]

(In reply to d.a. from comment #0)
> Currently any button element is Firefox Nightly doesn't match the look in OS
> X Yosemite. Most noticeable are the "default" button and active state where
> the text color is black when really should be white. 

Forgive me for not being the most visually oriented person, but you wrote "most noticeable" - what else, if anything, besides the text color on the default/focused/active button are we not getting right wrt the button style on 10.10?

Comment 3

[d.a.]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to d.a. from comment #0)
> > Currently any button element is Firefox Nightly doesn't match the look in OS
> > X Yosemite. Most noticeable are the "default" button and active state where
> > the text color is black when really should be white. 
> 
> Forgive me for not being the most visually oriented person, but you wrote
> "most noticeable" - what else, if anything, besides the text color on the
> default/focused/active button are we not getting right wrt the button style
> on 10.10?

I should have used a period instead of the the word and, as in it is most noticeable in the "default" button.

Should be noted that the black text only appears on buttons. All other controls (selects, radiobuttons and checkboxes) have white colored symbols.

Comment 4

[Stefan [:stefanh]]

I belive the easiest way to fix this is to add style rules in button.css so (when on Yosemite) we change the label color to white when the button is pressed. I see that we have -moz-buttonhover, so I guess one could also add a -moz-buttonactive color and (if it exists) use a system NSColor (if we're lucky it'll stay black on < 10.10)- not sure if it's worth it, though.

Comment 5

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #4)
> I see that we have -moz-buttonhover

Oops, it's "-moz-buttonhovertext" so it should be -moz-buttonactivetext.

Comment 6

[:Gijs (he/him)]

(In reply to Stefan [:stefanh] from comment #5)
> (In reply to Stefan [:stefanh] from comment #4)
> > I see that we have -moz-buttonhover
> 
> Oops, it's "-moz-buttonhovertext" so it should be -moz-buttonactivetext.

But it's not the active state - which is a "pressed" button - but the 'default' state. Otherwise, I agree that we need another -moz-* thing here.

Comment 7

[Stefan [:stefanh]]

Ah, I missed the "default". But from the report it looks like it's both "default" and "pressed", though.

Comment 8

[d.a.]

Some clarification:

It's both the "default" and pressed/active state that have the wrong text-color (as in anytime the button turns blue). 

But unless you keep the button pressed for long you're not that likely to notice that the text color is wrong in the active/pressed state.

Comment 9

[:Gijs (he/him)]

Attachment: fix default/active button text on OS X 10.10,

This WFM, although it took a little bit of poking to get the CSS right. Markus, Jared, what do you think? :-)

Comment 10

[:Gijs (he/him)]

For the record, this was based on some IRC discussion with Markus ( http://logs.glob.uno/?c=mozilla%23fx-team&s=11+Sep+2014&e=11+Sep+2014#c165962 and later) where he suggested having two constants - even if they are currently implemented the same way, it makes sense to me to define them separately. Likewise, it seemed best to evil-hardcode the values in nsLookAndFeel instead of having a media query in toolkit, but one could argue differently, I suppose, if we're comfortable sticking this in CSS instead...

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8488329 [details] [diff] [review]
fix default/active button text on OS X 10.10,

Review of attachment 8488329 [details] [diff] [review]:
-----------------------------------------------------------------

Markus, in test_platform_colors.xul there is a check for colors[c].length == 8. Why should that cause a test failure?

::: widget/tests/test_platform_colors.xul
@@ +65,5 @@
>    "-moz-dragtargetzone": ["rgb(199, 208, 218)", "rgb(198, 198, 198)", "rgb(180, 213, 255)", "rgb(250, 236, 115)", "rgb(255, 176, 139)", "rgb(255, 209, 129)", "rgb(194, 249, 144)", "rgb(232, 184, 255)"],
>    "-moz-hyperlinktext": ["rgb(0, 0, 238)"],
>    "-moz-html-cellhighlight": ["rgb(212, 212, 212)"],
>    "-moz-html-cellhighlighttext": ["rgb(0, 0, 0)"],
> +  "-moz-mac-buttonactivetext": ["rgb(0, 0, 0)", "rgb(255, 255, 255)"],

It would be nice if this could only be #fff when on Yosemite and later, and #000 when earlier than Yosemite.

Comment 12

[:Gijs (he/him)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=0d01038c6240
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0d01038c6240

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/3646cd944abe

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3646cd944abe

Comment 15

[d.a.]

I can confirm that the patch working on OS X Yosemite, the text is now white where it should be.

Comment 16

[Sören Hentzschel]

(In reply to d.a. from comment #15)
> the text is now white where it should be.

… and white where it should not be white (alerts).

Comment 17

[Sören Hentzschel]

I filed bug 1068913.

Comment 18

[:Gijs (he/him)]

Attachment: Patch for Aurora

Comment 19

[:Gijs (he/him)]

Comment on attachment 8498102 [details] [diff] [review]
Patch for Aurora

So I've pulled together this patch, and those for bug 1072992 and bug 1068913, and would like to uplift the lot to 34.

Approval Request Comment
[Feature/regressing bug #]: Yosemite
[User impact if declined]: button colors will look odd on OS X Yosemite when released (rumored for October)
[Describe test coverage new/current, TBPL]: little, mostly styling only patch (some minor widget additions which have parsing tests)
[Risks and why]: we might still have missed buttons somewhere in the UI that override the button styling but don't override the foreground color (like the issues from bug 1068913, bug 1072992). Fixing those kinds of issues is pretty trivial, however, and this has been in Nightly for 2 weeks now, so I expect this to be OK.
[String/UUID change made/needed]: nope

Comment 20

[:Gijs (he/him)]

Comment on attachment 8498102 [details] [diff] [review]
Patch for Aurora

Found another small issue, namely disabled buttons...

Comment 21

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 22

[:Gijs (he/him)]

Comment on attachment 8501083 [details] [diff] [review]
Patch for aurora

Once more: I've pulled together this patch, and those for bug 1076252, bug 1072992 and bug 1068913, and would like to uplift the lot to 34.

Approval Request Comment
[Feature/regressing bug #]: Yosemite
[User impact if declined]: button colors will look odd on OS X Yosemite when released (rumored to be in the next two weeks)
[Describe test coverage new/current, TBPL]: little, mostly styling only patch (some minor widget additions which have parsing tests)
[Risks and why]: we might still have missed buttons somewhere in the UI that override the button styling but don't override the foreground color (like the issues from bug 1068913, bug 1072992, bug 1076252). Fixing those kinds of issues is pretty trivial, however, and this has been in Nightly for 2 weeks now, so I expect this to be OK. 2 out of the 3 regressions were also 10.10-only, so to a certain degree, the people most affected by this are also where the risk is, and I think that "with this patch" is a better state than "without this patch", even if there are more not-correctly-styled buttons lurking in odd places in the UI, because it'd still be fixing the large majority.
[String/UUID change made/needed]: nope

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501083 [details] [diff] [review]
Patch for aurora

Aurora+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5c8203fa93b

Comment 25

[Florin Mezei, QA (:FlorinMezei)]

Marking as Verified on Firefox 35 based on comment 15 and comment 16.