531537 - menulist.removeAllItems destroys menulist.menupopup instead of removing all items, breaks onmenupopup handler of menulist

Status: NEW

(Core :: XUL, defect)

Description

[Thomas D. (:thomas8)]

Attachment: Interactive Test case

OK folks, here's my first bug against XUL...
(thanks to Ben Bucksch who gently pushed me into examining this, although I'm really still learning XUL... ;)

menulist.removeAllItems destroys the entire <menupopup> child of the menulist, where it should really only "remove all items" _within_ that menupopup (the children of menupopup). Not only are we destroying all attributes of the original <menupopup...> tag (like id etc.), but also this actually breaks the menulist's onpopupshowing handler, if you call menulist.removeAllItems and then re-append items from within the handler (with the intent of updating/recreating the list of menuitems before it pops up). So the popup doesn't show up :(

This is the bad code:

function removeAllItems() {
  this.selectedItem = null;
  var popup = this.menupopup;
  if (popup) {
    this.removeChild(popup) // this is too destructive and breaks the popup
  }
} 

And this should fix it (I hope):

function removeAllItems() {
  this.selectedItem = null;
  var itemCount = this.itemCount;
  for( var i = (itemCount-1); i >= 0; i-- ) this.removeItemAt(i);
} 

Other people have seen this bug as well:
http://bugs.code.downthemall.net/trac/changeset/1555 (line 404 :)))

Comment 1

[Thomas D. (:thomas8)]

Link to the source code causing this bug:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/menulist.xml#372

Comment 2

[Ben Bucksch (:BenB)]

Thanks a lot, Thomas, for filing the bug and the minimal testcase!

I hope somebody will pick it up?

Comment 3

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336