Closed Bug 870865 Opened 8 years ago Closed 8 years ago

-moz-mac-disabledtoolbartext is too dark

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jaws, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M?][Australis:P2])

Attachments

(3 files, 4 obsolete files)

No description provided.
No longer blocks: 869104
Whiteboard: [Australis:M7]
Assignee: nobody → gijskruitbosch+bugs
Assignee: gijskruitbosch+bugs → mdeboer
Status: NEW → ASSIGNED
Depends on: 873398
Attached patch patch (obsolete) — Splinter Review
Attachment #762129 - Flags: review?(jaws)
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.
Attachment #762129 - Flags: review?(jaws)
Attached image 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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
(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?
Status: RESOLVED → REOPENED
Flags: needinfo?(mdeboer)
Resolution: INVALID → ---
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.
Flags: needinfo?(mdeboer)
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!
Attachment #762129 - Attachment is obsolete: true
Attachment #764366 - Flags: review?(mstange)
Status: REOPENED → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Stephen, are you OK with this change? It shouldn't affect web content since it is only -moz-mac-disabledtoolbartext.
Assignee: mdeboer → jaws
Attachment #764368 - Flags: ui-review?(shorlander)
Attachment #764366 - Attachment is obsolete: true
Attachment #764366 - Flags: review?(mstange)
Attachment #764368 - Flags: review?(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.
Attachment #764368 - Flags: review?(mstange) → review+
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? :-)
Component: Theme → Widget: Cocoa
Product: Firefox → Core
Summary: (Australis) Disabled buttons in the panel and customization mode should have graytext → -moz-mac-disabledtoolbartext is too dark
(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
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
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.
Attachment #764368 - Flags: ui-review?(shorlander) → ui-review-
Would say P3 except for the impact on Cut/Copy/Paste.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
Duplicate of this bug: 885571
Duplicate of this bug: 890142
Duplicate of this bug: 890141
bluntly stealing this from ya, Jared!
Assignee: jaws → mdeboer
Attached patch Patch v2 (obsolete) — Splinter Review
Changed the color to rgb(140, 140, 140). Carrying over r=mstange
Attachment #764368 - Attachment is obsolete: true
Attachment #771249 - Flags: ui-review?(shorlander)
Attachment #771249 - Flags: review+
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 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?
(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.
Assignee: mdeboer → shorlander
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.
(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
Attached patch Patch v3Splinter Review
Markus, I'd say Stefan is right in comment 25 and I updated the patch accordingly. What do you think?
Assignee: shorlander → mdeboer
Attachment #771249 - Attachment is obsolete: true
Attachment #771249 - Flags: ui-review?(shorlander)
Attachment #780708 - Flags: review?(mstange)
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 on attachment 780708 [details] [diff] [review]
Patch v3

seems great
Attachment #780708 - Flags: review?(mstange) → review+
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M?][Australis:P2][fixed-in-ux]
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.
(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.
Flags: needinfo?(mdeboer)
I will do it. Completely slipped off my radar :)
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/0e779a0da280
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/2e0d2777ff19
Whiteboard: [Australis:M?][Australis:P2][fixed-in-ux] → [Australis:M?][Australis:P2]
You need to log in before you can comment on or make changes to this bug.