Closed Bug 571567 Opened 10 years ago Closed 10 years ago

Toolbar menu missing checkmarks for enabled items

Categories

(Toolkit :: Themes, defect, major)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- final+

People

(Reporter: aaronmt, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(2 files)

Currently on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre all enabled items are missing their associative checkmark to indicate that they are enabled. See attached screenshot where all my items are enabled.
Summary: Toolbar missing checkbox for enabled items → Toolbar menu missing checkmarks for enabled items
regression from bug 567782
Blocks: 567782
Component: Toolbars → Themes
Keywords: regression
Product: Firefox → Toolkit
QA Contact: toolbars → themes
Severity: normal → major
The toolbar context menu is using a <popup> element, which is obsolete. All these should be <menupopup>s:

http://mxr.mozilla.org/mozilla-central/search?string=%3Cpopup+&find=%2Fbrowser%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
I'm guessing some/all of the ones under toolkit should be too?
There's also DOM Inspector and various tests under layout/
Yeah, I don't think there's a reason to use <popup> anywhere.
Duplicate of this bug: 572133
blocking2.0: --- → ?
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Attachment #452670 - Flags: review?(gavin.sharp)
Comment on attachment 452670 [details] [diff] [review]
patch

>diff --git a/toolkit/obsolete/content/globalOverlay.xul b/toolkit/obsolete/content/globalOverlay.xul

>-    <popup id="aTooltip" class="tooltip" onpopupshowing="return FillInTooltip(document.tooltipNode);" > 
>+    <menupopup id="aTooltip" class="tooltip" onpopupshowing="return FillInTooltip(document.tooltipNode);">
>       <label id="TOOLTIP-tooltipText" class="tooltip-label" flex="1"/>
>-    </popup>
>+    </menupopup>

This isn't a menu - is menupopup really appropriate here?

I don't really know offhand whether there are any other differences between popups/menupopups that we'd need to be concerned about in the general case, but I bet Neil would, so running this by him is probably a good idea...
Attachment #452670 - Flags: review?(gavin.sharp) → review?(enndeakin)
Comment on attachment 452670 [details] [diff] [review]
patch

> I don't really know offhand whether there are any other differences between
popups/menupopups

Only in that features added in the last few years would have only handled menupopups if they were comparing tag names.

The code here should be using <tooltip> of course. Or, is this not being used?
Attachment #452670 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/1be071f3b9cb

I made toolkit/obsolete/content/globalOverlay.xul use <tooltip>, but I don't think that code is being used.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
> I made toolkit/obsolete/content/globalOverlay.xul use <tooltip>, but I don't
> think that code is being used.

SeaMonkey uses globalOverlay.xul. But as far as I can tell only so that we pull in chrome://global/content/globalOverlay.js. We don't use any of the XUL markup in SeaMonkey.
(In reply to comment #3)
> I'm guessing some/all of the ones under toolkit should be too?
> There's also DOM Inspector and various tests under layout/

Ian, do you know if we have bugs around to do this change in comm-central?
Well Bug 572682 <popupset> is for suite/. This should cover the majority of cases there.
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.