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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Themes
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

Trunk
mozilla1.9.3a5
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
Created attachment 447102 [details] [diff] [review]
Fix pinstripe's menu.css patch v0.1

This patch:
* Modifies selected="true" not to bother if menuitem has type="radio".
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #447102 - Flags: feedback?(stefanh)
(Assignee)

Updated

8 years ago
Attachment #447102 - Flags: feedback?(mstange)
(Assignee)

Updated

8 years ago
Attachment #447102 - Flags: feedback?(stefanh)
Attachment #447102 - Flags: feedback?(mstange)
(Assignee)

Comment 2

8 years ago
Created attachment 447106 [details] [diff] [review]
Fix pinstripe's menu.css patch v0.1a

Patch without extraneous logging.
Attachment #447102 - Attachment is obsolete: true
Attachment #447106 - Flags: feedback?(stefanh)
(Assignee)

Updated

8 years ago
Attachment #447106 - Flags: feedback?(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".
(Assignee)

Comment 4

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #447106 - Flags: feedback?(stefanh)
Attachment #447106 - Flags: feedback?(mstange)
(Assignee)

Comment 5

8 years ago
Created attachment 447129 [details] [diff] [review]
Fix pinstripe's menu.css (checked true) patch v0.1b

Changes since v0.1a:
* Use :not on checked="true" now.
Attachment #447106 - Attachment is obsolete: true
Attachment #447129 - Flags: feedback?(mstange)
(Assignee)

Updated

8 years ago
Attachment #447129 - Flags: feedback?(stefanh)
Comment on attachment 447129 [details] [diff] [review]
Fix pinstripe's menu.css (checked true) patch v0.1b

thanks
Attachment #447129 - Flags: feedback?(mstange) → feedback+
(Assignee)

Updated

8 years ago
Attachment #447129 - Flags: review?(dao)

Updated

8 years ago
Attachment #447129 - Flags: review?(dao) → review+

Comment 7

8 years ago
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?
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

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #447129 - Flags: feedback?(stefanh)
(Assignee)

Comment 10

8 years ago
Created attachment 447585 [details] [diff] [review]
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.
Attachment #447129 - Attachment is obsolete: true
Attachment #447585 - Flags: review+
Attachment #447585 - Flags: feedback?(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.
(Assignee)

Updated

8 years ago
Attachment #447585 - Flags: feedback?(mstange)
(Assignee)

Comment 12

8 years ago
Created attachment 447615 [details] [diff] [review]
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?
Attachment #447585 - Attachment is obsolete: true
Attachment #447615 - Flags: review+
Attachment #447615 - Flags: feedback?(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.
Attachment #447615 - Flags: feedback?(mstange) → feedback+

Updated

8 years ago
Attachment #447615 - Flags: review+
(Assignee)

Comment 14

8 years ago
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.
Attachment #447615 - Flags: review?(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.
Attachment #447615 - Flags: review?(dao) → review+
(Assignee)

Comment 16

8 years ago
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
Attachment #447615 - Attachment description: Fix pinstripe's menu.css (menulist vs :not) patch v0.1d → Fix pinstripe's menu.css (menulist vs :not) patch v0.1d [Checkin: Comment 16]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

Comment 17

8 years ago
>-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

Updated

8 years ago
Depends on: 571567
You need to log in before you can comment on or make changes to this bug.