Closed Bug 1044595 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox33 --- wontfix
firefox34 + fixed
firefox35 --- verified

People

(Reporter: danne.da, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Component: General → Widget: Cocoa
Product: Toolkit → Core
Depends on: theme-yosemite
No longer depends on: theme-yosemite
(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?
Flags: needinfo?(danne.da)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
Flags: needinfo?(danne.da)
Summary: OS X Yosemite Update buttons to match new look → [10.10] default buttons in dialogs etc. have black text instead of white
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.
(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.
(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.
Ah, I missed the "default". But from the report it looks like it's both "default" and "pressed", though.
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.
This WFM, although it took a little bit of poking to get the CSS right. Markus, Jared, what do you think? :-)
Attachment #8488329 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8488329 - Flags: review?(mstange)
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...
Attachment #8488329 - Flags: review?(mstange) → review+
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.
Attachment #8488329 - Flags: review?(jaws) → review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3646cd944abe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
I can confirm that the patch working on OS X Yosemite, the text is now white where it should be.
No longer depends on: 1068384
(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).
Depends on: 1068913
I filed bug 1068913.
Depends on: 1068702
No longer depends on: 1068702
Depends on: 1072992
Attached patch Patch for Aurora (obsolete) — Splinter Review
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
Attachment #8498102 - Flags: review+
Attachment #8498102 - Flags: approval-mozilla-aurora?
Comment on attachment 8498102 [details] [diff] [review]
Patch for Aurora

Found another small issue, namely disabled buttons...
Attachment #8498102 - Flags: approval-mozilla-aurora?
Depends on: 1076252
Attached patch Patch for auroraSplinter Review
Attachment #8488329 - Attachment is obsolete: true
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
Attachment #8501083 - Flags: review+
Attachment #8501083 - Flags: approval-mozilla-aurora?
Attachment #8498102 - Attachment is obsolete: true
Comment on attachment 8501083 [details] [diff] [review]
Patch for aurora

Aurora+
Attachment #8501083 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking as Verified on Firefox 35 based on comment 15 and comment 16.
See Also: → 1531338
You need to log in before you can comment on or make changes to this bug.