With nsIStandaloneNativeMenu and nsISystemStatusBar it's possible to create menus for XUL DOM menu elements that are not inside their document's DOM tree, but "detached". In order to keep the native menus up-to-date when the DOM changes, native menus use a mutation observer. However, this mutation observer is currently installed on the document, and not on the observed elements themselves, and thus is not notified for changes outside the document and doesn't catch updates to "detached" menus. This problem didn't arise with menu elements in the normal OS X menubar because the menubar is always inside the document, so getting the detached state wrong didn't matter before. Fixing this solves one of the problems Florian encountered in bug 1037433, this one: (In reply to Florian Quèze [:florian] [:flo] from bug 1037433 comment #29) > Something that caused me some anxiety until I figured out how to work around > it: > > If I do: > let menu = document.createElement("menu"); > menu.setAttribute("image", ... > > let menupopup = document.createElement("menupopup"); > menupopup.addEventListener("popupshowing", showinghandler); > menu.appendChild(menupopup); > > this._statusBar.addItem(menu); > Then all the updates to the menupopup I do in showinghandler are ignored. It > changes the DOM, but not the menu in the menubar. > > If instead I do: > let menu = document.createElement("menu"); > menu.setAttribute("image", ... > > this._statusBar.addItem(menu); > > let menupopup = document.createElement("menupopup"); > menupopup.addEventListener("popupshowing", showinghandler); > menu.appendChild(menupopup); > Then the changes I do in showinghandler have a visible effect.

This changes the mutation observers to observe the interesting elements directly, instead of observing the document. Observing just mContent is not enough because we also need to catch changes to e.g. mCommandElement (for menuitems): For example, for a menuitem with command="somecommand", if there's a command element <command id="somecommand">, setting disabled="true" on that command element needs to update the disabled state of the menu item. The first version of this patch didn't work for that case so I added a test for it.

Markus, I probably won't be able to get to this review until next week. Sorry for the delay.

No worries. This is not urgent at all since Florian was able to work around it.

Comment on attachment 8461613 [details] [diff] [review] v1 This looks fine to me. But I'm unable to think of a way to test it. - nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mDocument)); + nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mContent->GetDocument())); Since you wrote your patch, bug 314095 has landed, which gets rid of nsIContent::GetContent(). I understand we're supposed to replace instances of this with either nsINode::GetUncomposedDoc() or nsINode::GetComposedDoc(). But since the instance variable you're getting rid of (nsMenuGroupOwnerX::mDocument) was originally initialized with the value of nsINode::OwnerDoc(), I suggest you use that instead.

> which gets rid of nsIContent::GetContent() which gets rid of nsIContent::GetDocument()

(In reply to Steven Michaud from comment #5) > Comment on attachment 8461613 [details] [diff] [review] > v1 > > This looks fine to me. But I'm unable to think of a way to test it. Running the included mochitest would be one way ;-) > - nsCOMPtr<nsIDOMDocument> > domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mDocument)); > + nsCOMPtr<nsIDOMDocument> > domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mContent->GetDocument())); > > Since you wrote your patch, bug 314095 has landed, which gets rid of > nsIContent::GetContent(). I understand we're supposed to replace instances > of this with either nsINode::GetUncomposedDoc() or > nsINode::GetComposedDoc(). But since the instance variable you're getting > rid of (nsMenuGroupOwnerX::mDocument) was originally initialized with the > value of nsINode::OwnerDoc(), I suggest you use that instead. I agree that OwnerDoc() seems like the right thing to use here. https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5219e1fa26