1222797 - listbox.selectedItems is a ChromeNodeList since bug 120684

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[aleth [:aleth]]

Fix the usages of listbox.selectedItems in c-c.

Comment 1

[aleth [:aleth]]

Attachment: Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684

Comment 2

[aleth [:aleth]]

Attachment: Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684, suite/ part

Comment 3

[aleth [:aleth]]

Comment on attachment 8684652 [details] [diff] [review]
Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684

Didn't want this r? to be cancelled, it's just bzexport being annoying.

Comment 4

[Philip Chee]

Comment on attachment 8684654 [details] [diff] [review]
Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684, suite/ part

The large amount of whitespace changes makes this patch hard to review. Please leave off all the whitespace changes or put them in a separate patch.

> -  var languages = aListbox.selectedItems;
> -  var addedLang = Array.map(languages, function(e) { return e.value; });
> +  var languages = [...aListbox.selectedItems];
> +  var addedLang = languages.map(function(e) { return e.value; });

This appears to be unnecessary.

>  function awKeyDown(event, listboxElement)
>  {
>    switch(event.keyCode) {
>    case KeyEvent.DOM_VK_DELETE:
>    case KeyEvent.DOM_VK_BACK_SPACE:
>      /* Warning, the listboxElement.selectedItems will change everytime we delete a row */
> -    var selItems = listboxElement.selectedItems;
Good catch. You can keep this change but nuke all the whitespace adjustments please!

> -                       .forEach(this.selectedItems.push, this.selectedItems);
> +                       .forEach(this.selectedItems.append, this.selectedItems);
OK.

>  function onShowAttachmentContextMenu()
>  {
>    // if no attachments are selected, disable the Open and Save...
>    var attachmentList = document.getElementById('attachmentList');
> -  var selectedAttachments = attachmentList.selectedItems;
> +  var selectedAttachments = [...attachmentList.selectedItems];
OK.

>  function handleAttachmentSelection(commandPrefix)
>  {
>    var attachmentList = document.getElementById('attachmentList');
> -  var selectedAttachments = attachmentList.selectedItems;
> +  var selectedAttachments = [...attachmentList.selectedItems];
OK

review- needs a new patch without the whitespace only changes.

Comment 5

[Philip Chee]

Comment on attachment 8684652 [details] [diff] [review]
Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684

> -  list.selectedItems.length = 0;
> +  while (list.selectedItems.length) {
> +    let item = list.selectedItems[0];
> +    item.selected = false;
Why?
> +    list.selectedItems.remove(item);

Uh? Try this:

while(list.selectedItems.lastChild) {
  list.selectedItems.lastChild.remove();
}

Comment 6

[Philip Chee]

> -  list.selectedItems.length = 0;
> +  while (list.selectedItems.length) {
> +    let item = list.selectedItems[0];
> +    item.selected = false;
> +    list.selectedItems.remove(item);

I read the code in Bug 120684 again. I think you can do:

while (list.selectedItems.length) {
  list.removeItemFromSelection(list.selectedItems[0]);
}

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8684652 [details] [diff] [review]
Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684

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

::: mail/base/content/msgHdrViewOverlay.js
@@ +2570,5 @@
>  function HandleSelectedAttachments(action)
>  {
>    let attachmentList = document.getElementById("attachmentList");
>    let selectedAttachments = [];
> +  for (let item of attachmentList.selectedItems)

please add braces for the loop while you're here

@@ +2682,5 @@
> +  while (list.selectedItems.length) {
> +    let item = list.selectedItems[0];
> +    item.selected = false;
> +    list.selectedItems.remove(item);
> +  }

go with Philip's suggestion if that works out

Comment 8

[aleth [:aleth]]

Attachment: Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684

Comment 9

[aleth [:aleth]]

Attachment: Remove some gratuitous whitespace from suite/

Comment 10

[aleth [:aleth]]

Attachment: Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684, suite/ part

Comment 11

[aleth [:aleth]]

(In reply to Philip Chee from comment #6)
> > -  list.selectedItems.length = 0;
> > +  while (list.selectedItems.length) {
> > +    let item = list.selectedItems[0];
> > +    item.selected = false;
> > +    list.selectedItems.remove(item);
> 
> I read the code in Bug 120684 again. I think you can do:
> 
> while (list.selectedItems.length) {
>   list.removeItemFromSelection(list.selectedItems[0]);
> }

I did list.clearSelection(), it seems the most future-proof.

Comment 12

[Philip Chee]

Comment on attachment 8684703 [details] [diff] [review]
Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684, suite/ part

Also a=me for SeaMonkey bustage fix CLOSED TREE

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/88072f1a437435dea3fa6498679ee7479d2a2615
Bug 1222797 - Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684. r=mkmelin

https://hg.mozilla.org/comm-central/rev/22aefa4709d1c9cb1d7a40f9fc54d9aeb388b407
Bug 1222797 - Remove some gratuitous whitespace from suite/. r=IanN

https://hg.mozilla.org/comm-central/rev/554c45f6c0787943d71beb48fafe67665a67d59a
Bug 1222797 - Fix usage of listbox.selectedItems, which is a ChromeNodeList since bug 120684, suite/ part. r=philip.chee a=bustage fix CLOSED TREE DONTBUILD