Closed Bug 586408 Opened 14 years ago Closed 14 years ago

Can't use context menu on toolbar to customize icons/text

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: stefanh, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

If you right-click on the PT and, from the context menu, open the sub-menu "Settings for this toolbar", none of the selections has any effect. You just get an error message in the console:

Error: toolbar is null
Source File: chrome://communicator/content/utilityOverlay.js
Line: 393
It's the same with the Navigation Toolbar so I guess it's not a Places Bookmarks regression. I'd rather guess it's the <popup> change since utilityOverlay.js has this:

var toolbar = document.popupNode;
On WinXP, so Platform should probably be All/All (haven't checked Linux, though).
Aah, ok. Yeah, my bad.
No longer blocks: SMPlacesBMarks
Component: Bookmarks & History → UI Design
OS: Mac OS X → All
QA Contact: bookmarks → ui-design
Hardware: x86 → All
Summary: Can't use context menu on Personal Toolbar to customize icons/text → Can't use context menu on toolbar to customize icons/text
Same for all other toolbars I've checked (MailNews: main, standalone and Compose windows, address book window).

Maybe it's because we have nested menupopups?

/me wants to have (a working) Venkman back. :-(
Neil Deakin recently deprecated document.popupNode for popup.triggerNode.
/me does some datamining via TortoiseHg....

Changeset http://hg.mozilla.org/comm-central/rev/ed8906789d08
Bug 383930/552341, allow usage of a property on popups instead of using document.

Bug 383930 - Replace document.popupNode with another mechanism
Bug 552341 - leak on closing tab if an image on the page had a context menu opened on it

"This patch adds popup.triggerNode which is valid while the popup is open.
document.popupNode may still be and is cleared when the popup is closed"

"This broke the copy link (cmd_copyLink) and various copy image (cmd_copyImage,
cmd_copyImageLocation, cmd_copyImageContents) commands. They only try to
retrieve the popup node from the window root, and not the popup manager."
Depends on: 383930
That change should have been compatible though. Or is the attempt to retrieve document.popupNode being done after the popup has already closed?
> Maybe it's because we have nested menupopups?

Do you mean you have a oncommand attribute on the toplevel element handling all descendants?
We have:

  <menupopup id="toolbar-context-menu"
         onpopupshowing="onViewToolbarsPopupShowing(event);"
         oncommand="onViewToolbarCommand(event);">
    <menuseparator id="toolbarmode-sep"/>
    <menu id="toolbarmode-context-menu"
          label="&customizeToolbar.toolbarmode.label;"
          accesskey="&customizeToolbar.toolbarmode.accesskey;">
      <menupopup id="toolbarmodePopup"
                 onpopupshowing="event.stopPropagation();"
                 oncommand="goSetToolbarState(event);">

goSetToolbarState() then does:
  aEvent.stopPropagation();
  var toolbar = document.popupNode; <- fails here
.....
Attached patch Use triggerNode patch v0.1 (obsolete) — Splinter Review
This patch:
* Passes this as an argument so that it can be used to get the triggerNode.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #469069 - Flags: superreview?(neil)
Attachment #469069 - Flags: review?(neil)
Ah, I was looking for this bug, since I tried debugging it last night.

What happens here is that the submenu has no trigger node. Now GetTriggerNode just walks the parent chain until it finds a trigger node, but document.popupNode only looks at the opening or last open popup.
Attached patch Possible patch (obsolete) — Splinter Review
This centralises the logic to look for the trigger content.

I made GetTriggerContent static to to centralise the null-check.
Attachment #469603 - Flags: review?(enndeakin)
Comment on attachment 469603 [details] [diff] [review]
Possible patch

A test for this would be nice.
Attachment #469603 - Flags: review?(enndeakin) → review+
Attached patch With testSplinter Review
Only three of the tests actually fail without the patch.
Assignee: iann_bugzilla → neil
Attachment #469069 - Attachment is obsolete: true
Attachment #469603 - Attachment is obsolete: true
Attachment #471976 - Flags: review?(enndeakin)
Attachment #469069 - Flags: superreview?(neil)
Attachment #469069 - Flags: review?(neil)
Attachment #471976 - Flags: review?(enndeakin) → review+
Comment on attachment 471976 [details] [diff] [review]
With test

Seeking approval to fix this regression from bug 383930.
Attachment #471976 - Flags: approval2.0?
Blocks: 383930
Component: UI Design → XUL
No longer depends on: 383930
Product: SeaMonkey → Core
QA Contact: ui-design → xptoolkit.widgets
Comment on attachment 471976 [details] [diff] [review]
With test

a=beltzner, who likes tests
Attachment #471976 - Flags: approval2.0? → approval2.0+
Pushed changeset 2754923dff6e to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.