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.

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."

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 .....

This patch: * Passes this as an argument so that it can be used to get the triggerNode.

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.

This centralises the logic to look for the trigger content. I made GetTriggerContent static to to centralise the null-check.

Comment on attachment 469603 [details] [diff] [review] Possible patch A test for this would be nice.

Only three of the tests actually fail without the patch.

Comment on attachment 471976 [details] [diff] [review] With test Seeking approval to fix this regression from bug 383930.

Comment on attachment 471976 [details] [diff] [review] With test a=beltzner, who likes tests

Pushed changeset 2754923dff6e to mozilla-central.