1459184 - Port |Bug 1456611 - Remove insertItemAt and removeItemAt methods from XUL widgets|

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Bug 1456611 is already on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ea1b6b6dfb

https://dxr.mozilla.org/comm-central/search?q=insertItemAt
https://dxr.mozilla.org/comm-central/search?q=removeItemAt

Comment 1

[:aceman]

Attachment: 1459184.patch

I manually tested most of the places, I just didn't test the editor changes and the cloudfile account.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56f86a10cc336ed49ee2bd06f687bce9b4f2e235

Comment 2

[:aceman]

Attachment: 1459184-cal.patch

Replacements in calendar.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8973368 [details] [diff] [review]
1459184-cal.patch

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

r+wc

::: calendar/base/content/widgets/calendar-widgets.xml
@@ +136,4 @@
>  
>              if (!item || item.getAttribute("value") != category) {
>                  // The item doesn't exist, insert it at the correct spot.
>                  item = categoryListbox.insertItemAt(newIndex, category, category);

The new code still has insertItemAt, maybe you missed removing this?

@@ +141,5 @@
> +                item = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "listitem");
> +                item.setAttribute("label", category);
> +                item.setAttribute("value", category);
> +                let beforeItem = categoryListbox.getItemAtIndex(newIndex);
> +                if (beforeItem)

brackets for if and else here. That aside, I believe calling node.insertBefore(item, null) is the same as calling appendChild. So you could drop the if block entirely. Please test before you do though :)

Comment 4

[Jorg K (CEST = GMT+2)]

The biggest problem is that the Gecko decision task is busted and we can't build anything. Not on try and not elsewhere :-(

Comment 5

[Jorg K (CEST = GMT+2)]

Try with M-C at d0ea1b6b6dfb:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=363b4781eba992fbbd139a9ddbee12667479cc74

So the build bustage is somewhere between d0ea1b6b6dfb and 5207b1392b11:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d0ea1b6b6dfb&tochange=5207b1392b11

Comment 6

[Jorg K (CEST = GMT+2)]

Still working at f5a77c38b072, so down to
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f5a77c38b072&tochange=5207b1392b11

For some reason the link doesn't work :-( It's these:
f124a57ef00a	Joel Maher — Bug 1455316 - sometimes when test-verify finds a failure, all future tests are marked as failing also. r=gbrown
64703ce328ea	Joel Maher — Bug 1442893 - disable ts_paint_heavy on osx due to length of time to unpack profile. r=rwood
7ba78b394fcb	Cosmin Sabou — Merge mozilla-central to inbound. a=merge CLOSED TREE
4065e9400f2b	Olli Pettay — Bug 1458694 - (android) ensure context menu works with ShadowDOM, r=JanH
f529be04c5a7	Nathan Froyd — Bug 1459035 - micro-optimize refcounting for nsSocket{Input,Output}Stream; r=valentin
92a50917efbc	vidit23 — Bug 1453957 - Stop setting the "unread" attribute on tabs. r=dao
b95fbd8db183	J.C. Jones — Bug 1445731 - land NSS NSS_3_37_RTM UPGRADE_NSS_RELEASE, r=me
af05b5813fc7	Gabriel Luong — Bug 1459257 - Check the inspector is visible before showing the 3 pane onboarding tooltip. r=jdescottes
0eade0cff0d6	Aaron Klotz — Bug 1459085: Prevent mutex reentry in mscom::Interceptor::GetInterceptorForIID; r=Jamie

I've looked through these and the only build related changes are in
64703ce328ea	Joel Maher — Bug 1442893 - disable ts_paint_heavy on osx due to length of time to unpack profile. r=rwood

Comment 7

[Jorg K (CEST = GMT+2)]

Confirmed with further tries that bug 1442893 caused the build failure.
So the question is, how to port a one line deletion to TB:
https://hg.mozilla.org/mozilla-central/rev/64703ce328ea
--- a/taskcluster/ci/test/test-sets.yml
-    - talos-h1
+    # - talos-h1 # too long to unpack profile- Bug 1442893

Strangely enough M-C f529be04c5a7 worked and 64703ce328ea and 7ba78b394fcb(??) failed. Strange.

Comment 8

[Jorg K (CEST = GMT+2)]

One test failure:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=363b4781eba992fbbd139a9ddbee12667479cc74&selectedJob=177098521

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973367 [details] [diff] [review]
1459184.patch

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

::: mail/base/content/ABSearchDialog.js
@@ -63,5 @@
>    // Initialize globals, see abCommon.js , InitCommonJS()
>    abList = document.getElementById("abPopup");
> -  if (abList.getItemAtIndex(0) != (kAllDirectoryRoot + "?"))
> -    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> -                        kAllDirectoryRoot + "?");

Removed without replacement?

@@ -78,5 @@
>  function searchOnUnload()
>  {
> -  let abPopup = document.getElementById('abPopup');
> -  if (abPopup.getItemAtIndex(0) == (kAllDirectoryRoot + "?"))
> -    document.getElementById('abPopup').removeItemAt(0);

Removed without replacement?

::: mail/components/accountcreation/content/emailWizard.js
@@ +203,5 @@
>        _hide("provisioner_button");
>  
> +    // Add the entry for the new host to the menulist
> +    let menuitem = menulist.appendItem("", "-new-"); // label,value
> +    menuitem.serverKey = null;

Could "prepend" be used here?

::: mail/components/addrbook/content/abCommon.js
@@ -359,5 @@
> -
> -  // Make an entry for "All Address Books".
> -  if (abList) {
> -    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> -                        kAllDirectoryRoot + "?");

Removed without replacement?

::: mail/components/preferences/attachmentReminder.js
@@ +42,5 @@
>                                      this.bundle.getString("attachmentReminderNewText"),
>                                      input, null, {value:0});
> +    if (ok && input.value) {
> +      let newKey = this.keywordListBox.appendItem(input.value, input.value);
> +      this.keywordListBox.ensureElementIsVisible(newKey);

Please explain.

::: mail/components/preferences/display.js
@@ +24,5 @@
> +      // Prepend menuitem with empty name and value.
> +      let item = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem");
> +      item.setAttribute("label", "");
> +      item.setAttribute("value", "");
> +      menulist.menupopup.prepend(item);

prepend? Where is that documented? And can't it be used in case of insert at 0?

::: mailnews/addrbook/content/addrbookWidgets.xml
@@ +83,5 @@
> +            listItem.setAttribute("IsNone", "true");
> +          }
> +
> +          if (this.hasAttribute("alladdressbooks")) {
> +            // Insert a menuitem representing All addressbooks.

Nit: "All Addressbooks".

Comment 10

[:aceman]

(In reply to Jorg K (GMT+1) from comment #9)
> ::: mail/base/content/ABSearchDialog.js
> @@ -63,5 @@
> >    // Initialize globals, see abCommon.js , InitCommonJS()
> >    abList = document.getElementById("abPopup");
> > -  if (abList.getItemAtIndex(0) != (kAllDirectoryRoot + "?"))
> > -    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> > -                        kAllDirectoryRoot + "?");
> 
> Removed without replacement?

Replaced by alladdressbooks="true" in ABSearchDialog.xul which invokes the common implementation of adding this menu entry in addrbookWidgets.xml which is also touched in this patch. The effect from this is that this item also gets the addressbook icon as other proper addressbooks. You can compare e.g. at Options-> Composition->Addressing->Default startup directory. This merges the different implementations of adding this "All addressbooks" item and styles them consistently. If the icon isn't wanted, we can change the class of the item (and again it will change consistently at all users) [needs UI review]

> @@ -78,5 @@
> >  function searchOnUnload()
> >  {
> > -  let abPopup = document.getElementById('abPopup');
> > -  if (abPopup.getItemAtIndex(0) == (kAllDirectoryRoot + "?"))
> > -    document.getElementById('abPopup').removeItemAt(0);
> 
> Removed without replacement?

The same.
 
> ::: mail/components/accountcreation/content/emailWizard.js
> @@ +203,5 @@
> >        _hide("provisioner_button");
> >  
> > +    // Add the entry for the new host to the menulist
> > +    let menuitem = menulist.appendItem("", "-new-"); // label,value
> > +    menuitem.serverKey = null;
> 
> Could "prepend" be used here?

Yes, but I avoided .prepend() where possible as it is marked experimental. Instead I reordered code where possible to use plain appendItem instead of .prepend() (.insertItemAt(0)).
 
> ::: mail/components/addrbook/content/abCommon.js
> @@ -359,5 @@
> > -
> > -  // Make an entry for "All Address Books".
> > -  if (abList) {
> > -    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> > -                        kAllDirectoryRoot + "?");
> 
> Removed without replacement?

Replaced by alladdressbooks="true" in abContactsPanel.xul which invokes the common implementation of adding this menu entry in addrbookWidgets.xml.

> ::: mail/components/preferences/attachmentReminder.js
> @@ +42,5 @@
> >                                      this.bundle.getString("attachmentReminderNewText"),
> >                                      input, null, {value:0});
> > +    if (ok && input.value) {
> > +      let newKey = this.keywordListBox.appendItem(input.value, input.value);
> > +      this.keywordListBox.ensureElementIsVisible(newKey);
> 
> Please explain.

The user added new keyword and it was appended to the list at the end outside of the visible part of the list. So I thought moving to the place to show that the keyword was actually added would be more sane. [needs UI review]

Similarly I improved this:
-      this.keywordListBox.removeItemAt(this.keywordListBox.selectedIndex);
-      this.keywordListBox.appendItem(input.value, input.value);
+      this.keywordListBox.selectedItem.value = input.value;
+      this.keywordListBox.selectedItem.label = input.value;

So instead of removing the current item the user is editing from the list and throwing it silently at the end of the list outside of user's visibility, I just change the current item in place. That seems more sane to me. [needs UI review]

You can try these 2 cases manually in Options->Composing->General->Keywords.
 
> ::: mail/components/preferences/display.js
> @@ +24,5 @@
> > +      // Prepend menuitem with empty name and value.
> > +      let item = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem");
> > +      item.setAttribute("label", "");
> > +      item.setAttribute("value", "");
> > +      menulist.menupopup.prepend(item);
> 
> prepend? Where is that documented? And can't it be used in case of insert at
> 0?

https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/prepend
Yes it can be used, but I chose to avoid it where possible.
 
> ::: mailnews/addrbook/content/addrbookWidgets.xml
> @@ +83,5 @@
> > +            listItem.setAttribute("IsNone", "true");
> > +          }
> > +
> > +          if (this.hasAttribute("alladdressbooks")) {
> > +            // Insert a menuitem representing All addressbooks.
> 
> Nit: "All Addressbooks".

Thanks.

Comment 11

[:aceman]

Attachment: 1459184.patch v1.1

I have fixed the nits and also added one SM occurrence (untested, but same code as in editor).

I have marked the potential behaviour changes by the patch in previous comment by [needs ui review]. Paenglab, please look at those places.

Thanks.

Comment 12

[Jorg K (CEST = GMT+2)]

New try with the non-Calendar patch only:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=769566c587adaaf275d8ec0aecaf55111ff2704f
M-C at rev 51d020e4c2cb to about Gecko decision bustage from bug 1459397.

Comment 13

[:aceman]

Attachment: 1459184-cal.patch v1.1

Removed the forgotten insert line.

Comment 14

[Jorg K (CEST = GMT+2)]

Green try :-)

As for the prepend: What would be the replacement of |menulist.menupopup.prepend(item);| ?

Since we're busted anyway due to bug 1459397, but have a green try, we can deliberate here a bit more.

(In reply to Philipp Kewisch [:Fallen]  from comment #3)
> That aside, I believe calling
> node.insertBefore(item, null) is the same as calling appendChild. So you
> could drop the if block entirely. Please test before you do though :)
How exactly can we test this with Calender not working :-(

That aside, Philipp asked for braces {} and the indentation in Calendar is four spaces. But that all won't be necessary since
https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore says:
If the reference node is null, the specified node is added to the end of the list of children of the specified parent node.

So that code becomes just:
categoryListbox.insertBefore(item, beforeItem);  // Will append if 'beforeItem' is null.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8973431 [details] [diff] [review]
1459184.patch v1.1

This makes sense to jump to the new element. With this the user sees it was added. Would it be possible to select it too? Only when it's not too much work.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973431 [details] [diff] [review]
1459184.patch v1.1

Thanks, this looks like a lot of work.

Comment 17

[:aceman]

(In reply to Jorg K (GMT+1) from comment #14)
> As for the prepend: What would be the replacement of
> |menulist.menupopup.prepend(item);| ?

Maybe menulist.menupopup.insertBefore(item, menulist.menupopup.firstChild);
Should I use that for safety?
 
> So that code becomes just:
> categoryListbox.insertBefore(item, beforeItem);  // Will append if
> 'beforeItem' is null.

OK, I understand now.

(In reply to Richard Marti (:Paenglab) from comment #15)
> This makes sense to jump to the new element. With this the user sees it was
> added. Would it be possible to select it too? Only when it's not too much
> work.

Should be easy (one line).

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1459184-cal.patch v1.2

Removed the if/else when inserting.

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #17)
> Maybe menulist.menupopup.insertBefore(item, menulist.menupopup.firstChild);
> Should I use that for safety?
Up to you.

> > categoryListbox.insertBefore(item, beforeItem);  // Will append if
> > 'beforeItem' is null.
> OK, I understand now.
Done in v1.2.

> Should be easy (one line).
OK, I'll wait for another patch.

Comment 20

[:aceman]

Attachment: 1459184.patch v1.2

I included the improvements.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973467 [details] [diff] [review]
1459184.patch v1.2

Thanks again.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02e4c2f6655f
Port bug 1456611: Replace use of insertItemAt and removeItemAt methods (mail part). r=jorgk, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/c870df0b09fa
Port bug 1456611: Replace use of insertItemAt and removeItemAt methods (calendar part). r=philipp