567782 - Fix theme for menuitems of type radio that can be both checked and selected

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Ian Neal]

Menuitems with type="radio" can have their attributes checked and selected both set to true causing a double checkmark to appear, see http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/menu.css#199 onwards

Comment 1

[Ian Neal]

Attachment: Fix pinstripe's menu.css patch v0.1

This patch:
* Modifies selected="true" not to bother if menuitem has type="radio".

Comment 2

[Ian Neal]

Attachment: Fix pinstripe's menu.css patch v0.1a

Patch without extraneous logging.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 447106 [details] [diff] [review]
Fix pinstripe's menu.css patch v0.1a

People shouldn't be setting both checked and selected on a menuitem, so this bug is about graceful error handling, right? Then using :not([checked="true"]) instead of :not([type="radio"]) would protect us even in cases where the author didn't set type="radio".

Comment 4

[Ian Neal]

(In reply to comment #3)
> (From update of attachment 447106 [details] [diff] [review])
> People shouldn't be setting both checked and selected on a menuitem, so this
> bug is about graceful error handling, right? Then using :not([checked="true"])
> instead of :not([type="radio"]) would protect us even in cases where the author
> didn't set type="radio".

Menuitems in a menupopup cannot avoid having selected, it's type="radio" that introduces the ability to have checked as well. You are correct that having :not([checked="true"]) would have greater protection.

Comment 5

[Ian Neal]

Attachment: Fix pinstripe's menu.css (checked true) patch v0.1b

Changes since v0.1a:
* Use :not on checked="true" now.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 447129 [details] [diff] [review]
Fix pinstripe's menu.css (checked true) patch v0.1b

thanks

Comment 7

[Stefan [:stefanh]]

Comment on attachment 447129 [details] [diff] [review]
Fix pinstripe's menu.css (checked true) patch v0.1b

Shouldn't we always display the checkmark character in checked/selected menulist menuitems?

Comment 8

[Markus Stange [:mstange]]

Stefan showed me that bug 521927 adds an actual use case for checked="true" in menulists, which I hadn't realized. So this bug is not just about graceful error handling.
I agree with Stefan: in order to make this look correct, we should extend the ::before-checkmarker to apply for checked="true", too, and add :not([selected="true"]) for the checkmark image.

Comment 9

[Stefan [:stefanh]]

Ian, I won't be able to give feedback on a new patch since I'll be away from now on. But that shouldn't be a problem.

Comment 10

[Ian Neal]

Attachment: Fix pinstripe's menu.css (checked/selected true) patch v0.1c

Changes since v0.1b:
* Extend to include checked="true" and :not selected="false"

Requesting feedback to confirm this is what is needed.

Comment 11

[Markus Stange [:mstange]]

Hm, looks like I had something different in mind than what I said. I think something like this would be good:

-menuitem[checked="true"][_moz-menuactive="true"] > .menu-iconic-left,
+:not(menulist) > menupopup > menuitem[checked="true"][_moz-menuactive="true"] > .menu-iconic-left,
 :not(menulist) > menupopup > menuitem[selected="true"][_moz-menuactive="true"] > .menu-iconic-left {
   background-image: -moz-image-rect("chrome://global/skin/menu/menu-check.png", 0, 20, 11, 10);
 }
 
-menulist:not([editable="true"]) > menupopup > menuitem[selected="true"]::before {
+menulist:not([editable="true"]) > menupopup > menuitem[selected="true"]::before,
+menulist:not([editable="true"]) > menupopup > menuitem[checked="true"]::before {
   content: '\2713'; /* a checkmark */
   display: block;
   width: 15px;
   -moz-margin-start: -15px;
 }

In other words: For menulists, use ::before, regardless of whether checked or selected was used; and for non-menulists use the image for both. Sorry for being unclear.

Comment 12

[Ian Neal]

Attachment: Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]

Changes since v0.1c:
* As per feedback only ones in :not menulists use the image, ones in menulists use the checkmark.

Is it okay what happens to ones that are in editable menulists?

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 447615 [details] [diff] [review]
Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]

Thanks, this looks perfect. As far as I know, editable menulists should never have any checkmarks.

Comment 14

[Ian Neal]

Comment on attachment 447615 [details] [diff] [review]
Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]

I was carrying forward the r= but I guess you want a new request.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 447615 [details] [diff] [review]
Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]

Yeah, this is quite different from the first patch, reviews don't work like that. However you can just count mstange's "feedback" as a review with moa=me.

Comment 16

[Ian Neal]

Comment on attachment 447615 [details] [diff] [review]
Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]

http://hg.mozilla.org/mozilla-central/rev/8b5cbcb481bd

Comment 17

[Philip Chee]

>-menuitem[checked="true"] > .menu-iconic-left,
>+:not(menulist) > menupopup > menuitem[checked="true"] > .menu-iconic-left,
> :not(menulist) > menupopup > menuitem[selected="true"] > .menu-iconic-left {

You could have simplified all this by using -moz-any()

 :not(menulist) > menupopup > -moz-any(menuitem[checked="true"], menuitem[selected="true"]) > .menu-iconic-left,

See http://dbaron.org/log/20100424-any