1075883 - Populate and teardown listbox and menulist elements in account manager using XUL methods, not direct DOM manipulation

Status: ASSIGNED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #1045791 comment 6 +++

:aceman 2014-08-11 14:32:02 CEST

9In some of the account manager menulists,) we clear the items using DOM commands on the menupopup and then add new elements (repopulate the list of servers) using .appendItem method of the XUL element of the parent menulist. That could create confusion for the widget.

Sometimes we even clear children of the menulist (even though there is a menupopup under it and above the menuitems), sometimes we clear the children of the menupopup directly. I think this inconsistency should be cleaned up. Maybe it does not create problems today, but it could in the future.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[[e-mail]]

Comment on attachment 8498383 [details] [diff] [review]
patch

> function buildServerFilterMenuList()
> {
>   const KEY_ISP_DIRECTORY_LIST = "ISPDL";
>   let ispHeaderList = document.getElementById("useServerFilterList");
>   // Ensure the menulist is empty.
>-  while (ispHeaderList.hasChildNodes()) {
>-    ispHeaderList.lastChild.remove();
>-  }
>+  ispHeaderList.removeAllItems();
This change is OK but I don't like the others, unless you can get a removeAllItems API added to listboxes.

Comment 3

[:aceman]

Why? Is the idea incorrect?

Comment 4

[:aceman]

See e.g. radio.xml in toolkit. It maintains an internal list of its <radio> children. Would it work correctly if we started to remove its child nodes via DOM commands? I want to avoid such potential problems in the future.

Also, in some of the cases we do not want a removeAllItems method as we do not remove all items.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #4)
> See e.g. radio.xml in toolkit. It maintains an internal list of its <radio>
> children. Would it work correctly if we started to remove its child nodes
> via DOM commands? I want to avoid such potential problems in the future.
> 
> Also, in some of the cases we do not want a removeAllItems method as we do
> not remove all items.

Comment 6

[:aceman]

Comment on attachment 8498383 [details] [diff] [review]
patch

Squib, what do you think of this?

Comment 7

[Jim Porter (:squib)]

Comment on attachment 8498383 [details] [diff] [review]
patch

Review of attachment 8498383 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review here, since I've forgotten most of what I know about the TB codebase and wouldn't be able to give a proper review. Sorry about that... :(