870865 - -moz-mac-disabledtoolbartext is too dark

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 762129 [details] [diff] [review]
patch

Why is this patch needed?

What's breaking <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/windows/global/toolbarbutton.css#l60>, <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/osx/global/toolbarbutton.css#l30> and <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/linux/global/toolbarbutton.css#l53>?

Comment 3

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

Comment on attachment 762129 [details] [diff] [review]
patch

I filed the bug but I don't see the issue anymore. And bug 873398 made this state better anyways. I think we can RESO-INVALID OR RESO-WORKSFORME now.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: No GrayText!

Jared: great that you don't see this anymore, but I still do! So I'm not inclined to WORKSFORME this yet... Got this on MB Pro Retina.

Dao: you are completely right, I posted the patch too soon (before investigating the cause). Will do that and post back my findings! Thanks.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #2)
> Why is this patch needed?
> 
> What's breaking
> <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> windows/global/toolbarbutton.css#l60>,
> <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> osx/global/toolbarbutton.css#l30> and
> <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> linux/global/toolbarbutton.css#l53>?

On OSX, the color difference between disabled and enabled state is roughly 5%.

Enabled color: rgb(51, 51, 51)
Disabled color: rgb(63, 63, 63)

There's probably a reason for this, and not worth touching unless the current state of affairs confuses ppl after landing Australis.

RESO-INVALID it is.

Comment 6

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

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > Why is this patch needed?
> > 
> > What's breaking
> > <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> > windows/global/toolbarbutton.css#l60>,
> > <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> > osx/global/toolbarbutton.css#l30> and
> > <http://hg.mozilla.org/mozilla-central/annotate/b197bed90a98/toolkit/themes/
> > linux/global/toolbarbutton.css#l53>?
> 
> On OSX, the color difference between disabled and enabled state is roughly
> 5%.
> 
> Enabled color: rgb(51, 51, 51)
> Disabled color: rgb(63, 63, 63)
> 
> There's probably a reason for this, and not worth touching unless the
> current state of affairs confuses ppl after landing Australis.
> 
> RESO-INVALID it is.

Those colors seem too close to me. Just because it's been broken for a while doesn't mean we need to persist that :)

Mike, can you check other applications and see what color difference they have between enabled and disabled text?

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Color difference between enabled and disabled text OSX

The actual disabled text color in OSX apps is roughly rgb(114, 114, 114) vs. rgb(51, 51, 51). That's almost 25% difference.

I say roughly, because - perhaps you can see this in the screenshot - button captions use a gradient text fill with (usually) a contrasting dropshadow downward.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Correct the value of moz_mac_disabledtoolbartext to rgb(114, 114, 114)

Markus, pleased to meet you! I wanted to drop this by you. The patch is small as can be, but fixes the appearance of text inside disabled toolbarbuttons.

For more background, please read comment 5 and comment 6.

Thanks!

Comment 9

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

Attachment: Patch

Stephen, are you OK with this change? It shouldn't affect web content since it is only -moz-mac-disabledtoolbartext.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 764368 [details] [diff] [review]
Patch

Looks like the right color actually depends on the OS version and on the window's focus state, but that's probably not worth worrying about.

Comment 11

[:Gijs (he/him)]

Regarding the summary, I think we only want to fix this for panel buttons, though, not for customization mode? We're busy making sure things never look disabled in customization mode in bug 882306... Can we make this only affect the menupanel, not customization mode? :-)

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #11)
> Regarding the summary, I think we only want to fix this for panel buttons,
> though, not for customization mode? We're busy making sure things never look
> disabled in customization mode in bug 882306... Can we make this only affect
> the menupanel, not customization mode? :-)

...what Dão said :P

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block us from landing on m-c.

Comment 14

[Stephen Horlander [:shorlander]]

Comment on attachment 764368 [details] [diff] [review]
Patch

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

This is an improvement, but the difference between this and regular text is pretty subtle. Could you please make it a little lighter? rgb(140, 140, 140) might work.

Comment 15

[Justin Dolske [:Dolske]]

Would say P3 except for the impact on Cut/Copy/Paste.

Comment 19

[Mike de Boer [:mikedeboer]]

bluntly stealing this from ya, Jared!

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2

Changed the color to rgb(140, 140, 140). Carrying over r=mstange

Comment 21

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

Note that I hadn't uploaded a new version because I was focused on trying to address this concern:

(In reply to :Gijs Kruitbosch from comment #11)
> Regarding the summary, I think we only want to fix this for panel buttons,
> though, not for customization mode? We're busy making sure things never look
> disabled in customization mode in bug 882306... Can we make this only affect
> the menupanel, not customization mode? :-)

It doesn't look like your new patch addresses this.

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #21)
> Note that I hadn't uploaded a new version because I was focused on trying to
> address this concern:
> 
> (In reply to :Gijs Kruitbosch from comment #11)
> > Regarding the summary, I think we only want to fix this for panel buttons,
> > though, not for customization mode? We're busy making sure things never look
> > disabled in customization mode in bug 882306... Can we make this only affect
> > the menupanel, not customization mode? :-)
> 
> It doesn't look like your new patch addresses this.

In the mean time Dão updated the summary to cover the issue more appropriately. If we keep this issue focused around the menupanel, we will end up with the same patch I submitted the first round. The patch in its current state fixes the problem at its source.

If we want to make the items in cust mode look NOT disabled at all times, it's an exception that will need to be dealt with in bug 882306. This bug is about proper platform integration now, right?

Comment 23

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

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> (In reply to Jared Wein [:jaws] from comment #21)
> > Note that I hadn't uploaded a new version because I was focused on trying to
> > address this concern:
> > 
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > Regarding the summary, I think we only want to fix this for panel buttons,
> > > though, not for customization mode? We're busy making sure things never look
> > > disabled in customization mode in bug 882306... Can we make this only affect
> > > the menupanel, not customization mode? :-)
> > 
> > It doesn't look like your new patch addresses this.
> 
> In the mean time Dão updated the summary to cover the issue more
> appropriately. If we keep this issue focused around the menupanel, we will
> end up with the same patch I submitted the first round. The patch in its
> current state fixes the problem at its source.
> 
> If we want to make the items in cust mode look NOT disabled at all times,
> it's an exception that will need to be dealt with in bug 882306. This bug is
> about proper platform integration now, right?

OK, carry on. I just wish I would have noticed the redirection better.

Comment 24

[Stefan [:stefanh]]

I havent checked what color disabledControlTextColor (https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSColor_Class/Reference/Reference.html) returns, but that seems like a better color to use than a hardcoded one.

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Stefan [:stefanh] from comment #24)
> I havent checked what color disabledControlTextColor
> (https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/
> ApplicationKit/Classes/NSColor_Class/Reference/Reference.html) returns, but
> that seems like a better color to use than a hardcoded one.

It does seem to be better! The RGB value I get on Mountain Lion is rgb(127, 127, 127). I'll do a try push to see if we get any variation on other OSX versions running widget/tests/test_platform_colors.xul

Comment 26

[Mike de Boer [:mikedeboer]]

https://tbpl.mozilla.org/?tree=Try&rev=bb9de80d5bfa

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3

Markus, I'd say Stefan is right in comment 25 and I updated the patch accordingly. What do you think?

Comment 28

[Stefan [:stefanh]]

Interesting, GrayText is (127, 127, 127)... When I filed bug 468498 back in 2008, the toolbar was darker - see https://bugzilla.mozilla.org/attachment.cgi?id=351958 for how it looked with GrayText then. But now, with a lighter toolbar gradient, I'd say that text with (127, 127, 127) looks better.

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 780708 [details] [diff] [review]
Patch v3

seems great

Comment 30

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/ux/rev/2e0d2777ff19

Comment 31

[Stefan [:stefanh]]

I wouldn't mind if this could be landed on m-c right away since I think the current -moz-mac-disabledtoolbartext is too dark.

Comment 32

[:Gijs (he/him)]

(In reply to Stefan [:stefanh] from comment #31)
> I wouldn't mind if this could be landed on m-c right away since I think the
> current -moz-mac-disabledtoolbartext is too dark.

I don't see any reason not to do just that (well, land on fx-team/inbound first, obviously, but otherwise...). Mike? The patch will have bitrotted slightly (I had a merge conflict with it a few days ago) but that should be trivial to solve.

Comment 33

[Mike de Boer [:mikedeboer]]

I will do it. Completely slipped off my radar :)

Comment 34

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/0e779a0da280

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0e779a0da280

Comment 36

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/2e0d2777ff19