69710 - Disabled menu items need more three-dimensional appearance

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Blake Ross]

Disabled menuitems need a more three-dimensional appearance, like they have on 
Windows; they're currently just gray, which isn't quite enough visual 
feedback.  Giving this to Ben, since he said he once had a stack binding that 
achieved a similar effect.

Comment 1

[Warner Young]

Blake, I assume you mean that std Windows disabled items are essentially painted
twice (once with COLOR_3DHILIGHT and once with COLOR_3DSHADOW, with a slight
offset)?

Comment 2

[Bamm Gabriana]

Please see comments in Bug 137350. I believe this relates to the implementation
of the CSS value GrayText. Windows only.

Comment 3

[Bamm Gabriana]

This will have a positive effect on the IE theme I am developing.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Mass reassign of my non-Firefox bugs to [e-mail]

Comment 7

[[e-mail]]

Attachment: Proposed patch

As per groupboxes this isn't enabled for Luna/Aero but is enabled on W2k.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 341264 [details] [diff] [review]
Proposed patch

>:not(:-moz-system-metric(windows-default-theme)) {

:-moz-system-metric(windows-classic)

>+  text-shadow: 1px 1px ThreeDHighlight;

Is ThreeDHighlight what native apps actually use?

>+checkbox[disabled="true"]:not(:-moz-system-metric(windows-default-theme)) {
>+  text-shadow: 1px 1px ThreeDHighlight !important;

Why !important?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> >+  text-shadow: 1px 1px ThreeDHighlight;
> 
> Is ThreeDHighlight what native apps actually use?

This probably needs to be tested with different color schemes, esp. high contrast ones.

Comment 10

[[e-mail]]

Attachment: Updated for review comments

* Replaced :not(:-moz-system-metric(windows-default-theme)) with :-moz-system-metric(windows-classic) (including some existing rules)
* Removed !important (including some existing rules)
* Added color: ThreeDShadow; after verifying against high contrast themes

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 341284 [details] [diff] [review]
Updated for review comments

It might be a good idea to update the same files in gnomestripe for consistency (except the windows-specific stuff).

>diff -r 8f53fcb8bde5 toolkit/themes/winstripe/global/menu.css

>+menulist > menupopup > menuitem[disabled="true"]:not([_moz-menuactive="true"]):-moz-system-metric(windows-classic) {
>+  text-shadow: none;
>+}

Shouldn't the color be reset to GrayText here as well?

Comment 12

[[e-mail]]

(In reply to comment #11)
>(From update of attachment 341284 [details] [diff] [review])
>It might be a good idea to update the same files in gnomestripe for consistency
>(except the windows-specific stuff).
Sorry, I'm not following you here... do you mean that all those widgets should have shadows in gnomestripe at all times?

>>diff -r 8f53fcb8bde5 toolkit/themes/winstripe/global/menu.css
>>+menulist > menupopup > menuitem[disabled="true"]:not([_moz-menuactive="true"]):-moz-system-metric(windows-classic) {
>>+  text-shadow: none;
>>+}
>Shouldn't the color be reset to GrayText here as well?
I thought that the previous rule had a higher weight, but I was wrong.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Sorry, I'm not following you here... do you mean that all those widgets should
> have shadows in gnomestripe at all times?

No, I was referring to the !important flags cleanup.

Comment 14

[[e-mail]]

Attachment: Fixed patch

I duplicated the style rules in gnomestripe to stop the hover rule overriding the disabled rule (winstripe has no hover text colour).

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 341424 [details] [diff] [review]
Fixed patch

I think this would be more straightforward:

radio:not([disabled="true"]):hover {
  color: -moz-buttonhovertext;
}

Comment 16

[[e-mail]]

Pushed changeset 6251e69516c9 to mozilla-central.

Comment 17

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screenshot

change width and height of xul:button, xul:checkbox, xul:menulist, xul:radio...
This is very strange.

Comment 18

[[e-mail]]

That's not quite true; only checkboxes and radio buttons are affected.
(Bug 458231 if you're interested.)

dao, as a workaround I could add transparent shadows to all classic checkboxes and radios so that they don't change size when disabled, if you like?

Comment 19

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #18)
> That's not quite true; only checkboxes and radio buttons are affected.
> (Bug 458231 if you're interested.)

Ok, It was my misunderstanding. Thank you.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #18)
> dao, as a workaround I could add transparent shadows to all classic checkboxes
> and radios so that they don't change size when disabled, if you like?

Depends on if you think bug 458231 makes 1.9.1...

Comment 21

[[e-mail]]

Attachment: Work around bug 458213

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 347524 [details] [diff] [review]
Work around bug 458213

Why -2px? Wouldn't -1 compensate the shadow correctly?

458213 is the wrong bug number...

Comment 23

[[e-mail]]

Attachment: Work around bug 458231

Confirmed by screen capture, the appearance with a transparent shadow and bottom margin of -2px matches the appearance without either styles. The disabled appearance with this patch matches the disabled appearance with bug 458231. (Of course, combining both patches results in an incorrect appearance!)

I had a look in layout code but I couldn't work out why it was -2 and not -1.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347530 [details] [diff] [review]
Work around bug 458231

a=beltzner

Comment 25

[[e-mail]]

Pushed changeset aca67792c001 to mozilla-central.