1871514 - Improve macOS control colors

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Sam Johnson]

Some colors (Infotext, Menutext, and MozSidebartext) are set to NSColor.textColor instead of NSColor.controlTextColor, which is the color observed in native controls.

When these values were originally selected in bug 517412, the system colors were the same, but they have since diverged.

Comment 1

[Sam Johnson]

Attachment: Bug 1871514 - Improve macOS control colors. r?#mac-reviewers

Instead of NSColor.textColor, use NSColor.controlTextColor, which is the color observed in native controls.

When these values were originally selected in bug 517412, the system colors were the same, but they have since diverged.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d95b94d4f547
Improve macOS control colors. r=mac-reviewers,bradwerth

Comment 3

[Sandor Molnar[:smolnar]]

Backed out for causing wpt failures at /css/css-color/deprecated-sameas

Backout link: https://hg.mozilla.org/integration/autoland/rev/1f9c7ba2aa71e5715e14177e0fac7a6e1de48698

Push with failures

Failure log -> /css/css-color/deprecated-sameas

Comment 4

[Sam Johnson]

It looks like both InfoText and MenuText should always match CanvasText, at least to web content, per the latest spec.

CanvasText uses the default color, rather than an OS-provided value. That seems incompatible with using OS-provided values for either InfoText or MenuText within web content.

Comment 5

[Sam Johnson]

(In reply to Sam Johnson from comment #4)

It looks like both InfoText and MenuText should always match CanvasText, at least to web content, per the latest spec.

CanvasText uses the default color, rather than an OS-provided value. That seems incompatible with using OS-provided values for either InfoText or MenuText within web content.

There is existing infrastructure to provide stand-ins to web content, which is already used to make WindowText also match CanvasText. Using stand-ins for InfoText and MenuText as well should resolve this.

Comment 6

[Sam Johnson]

(In reply to Sam Johnson from comment #5)

(In reply to Sam Johnson from comment #4)

It looks like both InfoText and MenuText should always match CanvasText, at least to web content, per the latest spec.

CanvasText uses the default color, rather than an OS-provided value. That seems incompatible with using OS-provided values for either InfoText or MenuText within web content.

There is existing infrastructure to provide stand-ins to web content, which is already used to make WindowText also match CanvasText. Using stand-ins for InfoText and MenuText as well should resolve this.

I've decided to revert this approach and instead annotate the failures on Mac. These tests already fail on other platforms for the same reason it now fails on Mac, and using stand-ins has consequences for the non-native theme.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:sam, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 8

[Brad Werth [:bradwerth] | Out until July 8]

(In reply to Sam Johnson from comment #6)

I've decided to revert this approach and instead annotate the failures on Mac. These tests already fail on other platforms for the same reason it now fails on Mac, and using stand-ins has consequences for the non-native theme.

That seems fine, too. I will obsolete the patch for you, to prevent Bugzilla from pinging us with reminders.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I'm confused, Sam updated the patch that you marked as obsolete with the approach described in comment 6. It was just waiting for review aiui.

Comment 10

[Brad Werth [:bradwerth] | Out until July 8]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

I'm confused, Sam updated the patch that you marked as obsolete with the approach described in comment 6. It was just waiting for review aiui.

I apologize. I am obviously being the source of the confusion. I've un-abandoned the revision (though I did commandeer it, so sam will need to take it back) so hopefully we're back on track. I'll stay out of it now.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f78d4c24f10a
Improve macOS control colors. r=mac-reviewers,emilio

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f78d4c24f10a