1523607 - [de-xbl] Remove attachment-list-base and its consumers' binding.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 2

[Arshad Khan [:arshad]]

Magnus, I went with the approach of using is attr that we discussed in last meeting. I am getting a NotSupportedError: Operation is not supported from the line customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" });. This is strange and I am not sure why it is happening. I am attaching the patch which combines the attachmentlist-vertical && attachmentlist-horizontal into single ce and depends upon orient attr to determine the case.

Comment 3

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems_(NSError).patch

Comment 4

[Arshad Khan [:arshad]]

Comment on attachment 9042713 [details] [diff] [review]
attachmentlist-elems_(NSError).patch

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

::: mail/base/content/mailWidgets.js
@@ +674,5 @@
> +    for (let child of this._childNodes) { child.width = width + border; }
> +  }
> +}
> +
> +customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" });

This line throws NotSupportedOperation Error.

Comment 5

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 9042713 [details] [diff] [review]
attachmentlist-elems_(NSError).patch

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

::: mail/base/content/mailWidgets.js
@@ +312,5 @@
> +// <richlistbox> is lazily set up through setElementCreationCallback,
> +// i.e. put into customElements the first time it's really seen.
> +// Create a fake to ensure richlistbox exists in customElements, since otherwise
> +// we can't extend it. Then make sure this fake doesn't stay around.
> +if (!customElements.get("richlistbox")) {

This shouldn't be necessary anymore, as richlistbox.js is now loaded eagerly: https://searchfox.org/mozilla-central/rev/43bf9e7c8acf042617a1ebc5d2ce540d78f0ce91/toolkit/content/customElements.js#389

@@ +674,5 @@
> +    for (let child of this._childNodes) { child.width = width + border; }
> +  }
> +}
> +
> +customElements.define("attachmentlist", MozAttachmentlist, { extends: "richlistbox" });

Can you try putting a dash in the name? I know we loosened the restriction for dashes for xul elements in general, but maybe that's not extending to customized built-ins?

Comment 7

[Arshad Khan [:arshad]]

Can you try putting a dash in the name? I know we loosened the restriction
for dashes for xul elements in general, but maybe that's not extending to
customized built-ins?

Yes, it was the dash.

Comment 8

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

The only issue with this patch is the removal of selected attribute(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.xml#83) by this line(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#485). click(https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#878) handler is called after mousedown event. Ideally it should not remove the current selected item but something is going wrong here. Other than that, the patch seems to be working fine for vertical and horizontal attachmentList.

Comment 9

[Arshad Khan [:arshad]]

Comment on attachment 9044660 [details] [diff] [review]
attachmentlist-elems.patch

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

::: mail/base/content/mailWidgets.js
@@ +771,5 @@
> +    item.setAttribute("context", this.getAttribute("itemcontext"));
> +    item.attachment = attachment;
> +
> +    if (this.orient === "horizontal") {
> +      this.attachmentListWrapper.insertBefore(item, this.getItemAtIndex(index));

This makes sure that richlistitem elements are appended inside scrollbox>hbox. This tries to cover up for the childrent tag functionality in the xbl binding.

Comment 10

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 11

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 12

[Arshad Khan [:arshad]]

Comment on attachment 9044873 [details] [diff] [review]
attachmentlist-elems.patch

There are a few other issues with this patch. The attachmentList element doesn't get focus event due to which there are differences in the UI when richlistitem is selected/clicked. Paenglab, could you please take a look why the attachmentList is not getting focussed?

If you open an email having some attachment, and then if you click on any of the attachment, normally the attachmentItem should have a white dotted outline and hightlight background, but only when attachmentList is in focus. Not sure why attachmentItem is not getting the focussed.

Comment 13

[Richard Marti (:Paenglab)]

Sorry, I know too less about the CE. What I can say is, that the richlistitems don't stay selected="true" and that now in the message pane's attachmentlist the attachmentlist-wrapper is now a for the DOM visible children of the richlistbox. This makes selectors like richlistbox > richlistitem no more working, they'd need now to be richlistbox > attachmentlist-wrapper > richlistitem. But this can not be the solution because we'd need to double all rules of richlistbox.css and other files for this list.

Comment 14

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Paenglab, mkmelin, some of the keyboard shortcuts are broken on the trunk as well.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 9044982 [details] [diff] [review]
attachmentlist-elems.patch

When you click on a attachmentitem and then use the cursor keys, the attachmentitems keep the selected state.

ui-r- because the attachmentitems in the message pane's don't have a focus selected state. This is because the toolkit richlist.css still has the sibling selector ( > ): richlistbox:focus > richlistitem...

Isn't it possible to make the richlistbox orient=horizontal without the attachmentlist-wrapper? Then we don't need this box.

Comment 16

[Arshad Khan [:arshad]]

(In reply to Richard Marti (:Paenglab) from comment #15)

Comment on attachment 9044982 [details] [diff] [review]
attachmentlist-elems.patch

When you click on a attachmentitem and then use the cursor keys, the
attachmentitems keep the selected state.

ui-r- because the attachmentitems in the message pane's don't have a focus
selected state. This is because the toolkit richlist.css still has the
sibling selector ( > ): richlistbox:focus > richlistitem...

Isn't it possible to make the richlistbox orient=horizontal without the
attachmentlist-wrapper? Then we don't need this box.

hmm why was scrollbox their in the first place?

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9044982 [details] [diff] [review]
attachmentlist-elems.patch

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

::: mail/base/content/mailWidgets.js
@@ +486,5 @@
>  customElements.define("mail-emailheaderfield", MozMailEmailheaderfield);
> +
> +/**
> + * Attachmentlist class extends richlistbox. This component shows the attachemnts attached
> + * to a mail in horizontal layout and in vertical layout, while sending an email to someone.

typo. perhaps

/**
 * The MozAttachmentlist widget lists attachments for a mail. This is typically
 * used to show attachments while writing a new mail as well as when reading
 * mails.
 * It has two layouts, which you can set by orient="horizontal" and
 * orient="vertical" respectively.
 */

@@ +493,5 @@
> +class MozAttachmentlist extends MozElements.RichListBox {
> +  constructor() {
> +    super();
> +
> +    const isNoModifierKeyPressed = event => (

As I wrote, I think you should just use event.getModifierState()

But please don't pass any parameter to that, as that's deprecated

@@ +504,5 @@
> +
> +    /**
> +     * The spacebar should work just like the arrow keys, except that the
> +     * focused element doesn't change, so use moveByOffset here.
> +     */

please use // style for these internal comments

@@ +542,5 @@
> +      }
> +    });
> +
> +    /**
> +     * make sure we keep the focus...

Capitalize.
Also make this comment // style

@@ +620,5 @@
> +        child => child.setAttribute("context",
> +          this.getAttribute("itemcontext")
> +        )
> +      );
> +

lots of lines here. 

    children.filter(child => !child.hasAttribute("context")).forEach(
      child => child.setAttribute("context", this.getAttribute("itemcontext"));
    );

::: mail/base/content/msgAttachmentView.inc.xul
@@ +100,5 @@
>                                       defaultset="attachmentSaveAll"/>
>                            </toolbox>
>                          </hbox>
>                          <richlistbox id="attachmentList"
> +                                     is="attachment-list"

please put this like this:

<richlistbox is="attachment-list" id="attachmentList"

::: mail/components/compose/content/messengercompose.xul
@@ +2019,5 @@
>                             context=""
>                             oncommand="attachmentBucketCloseButtonOnCommand();"/>
>            </hbox>
>            <richlistbox id="attachmentBucket"
> +                       is="attachment-list"

same here

Comment 18

[Magnus Melin [:mkmelin]]

Oh, and hg cp the binding to preserve some blame

Comment 19

[Magnus Melin [:mkmelin]]

I notice I was wrong about getModifierState, must have thought about something else.

Comment 20

[Magnus Melin [:mkmelin]]

for the "any", at least as used in this code the intention probably never was that this would require no other keys pressed, so I think we can just simplify and handle the key straight off

Comment 21

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #20)

for the "any", at least as used in this code the intention probably never was that this would require no other keys pressed, so I think we can just simplify and handle the key straight off

Did you mean that I should not test the noModifierKeyPressed state?

Comment 22

[Magnus Melin [:mkmelin]]

Yes

Comment 23

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 24

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Hopefully this will add hightlight background for linux.

Comment 25

[Arshad Khan [:arshad]]

Attachment: attachmentList_phase.patch

Magnus, does this look ok?

Comment 26

[Richard Marti (:Paenglab)]

Comment on attachment 9045280 [details] [diff] [review]
attachmentlist-elems.patch

Yes, this looks now good.

Comment 27

[Arshad Khan [:arshad]]

Magnus, we can work on the selected attr issue later because that's something where we need fx guys help. Once this lands I ll open a new bug for the selected attr issue.

Comment 28

[Arshad Khan [:arshad]]

Comment on attachment 9045280 [details] [diff] [review]
attachmentlist-elems.patch

changin the review to mkmelin as this is mail stuff

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9045280 [details] [diff] [review]
attachmentlist-elems.patch

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

::: mail/base/content/mailWidgets.js
@@ +579,5 @@
> +      });
> +    }
> +  }
> +
> +  connectedCallback() {

you're not calling super.connectedCallback() here, which is also using delayConnectedCallback() which the child presumably also should use then. https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js#147

@@ +601,5 @@
> +        <scrollbox flex="1" style="overflow: auto;">
> +          <hbox flex="1" class="attachmentlist-wrapper">
> +          </hbox>
> +        </scrollbox>
> +      `));

didn't try this yet, but looks wrong to have this here? how would the children even end up inside the wrapper?

Comment 30

[Arshad Khan [:arshad]]

Comment on attachment 9045280 [details] [diff] [review]
attachmentlist-elems.patch

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

::: mail/base/content/mailWidgets.js
@@ +778,5 @@
> +    item.setAttribute("context", this.getAttribute("itemcontext"));
> +    item.attachment = attachment;
> +
> +    if (this.orient === "horizontal") {
> +      this.attachmentListWrapper.insertBefore(item, this.getItemAtIndex(index));

element will added like this.

::: mail/base/content/msgHdrView.js
@@ +2632,5 @@
>    var list = document.getElementById("attachmentList");
>    list.clearSelection();
>  
> +  while (list.attachmentListWrapper.hasChildNodes())
> +    list.attachmentListWrapper.lastChild.remove();

See this.

Comment 31

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 32

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9048031 [details] [diff] [review]
attachmentlist-elems.patch

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

Write a new message, add two attachments, hit DOWN. I get JavaScript error: chrome://messenger/content/mailWidgets.js, line 864: TypeError: box is null
 (which is in the added code).

The selection is also a mess. Once I click one of the attachments, the selection doesn't stick.

::: mail/base/content/mailWidgets.js
@@ +644,5 @@
> +      event.preventDefault();
> +    });
> +
> +    this.addEventListener("keypress", (event) => {
> +      if (event.keyCode != KeyEvent.DOM_VK_RETURN) {

please use a switch for the keypress handling, instead of 5 listeners

@@ +728,5 @@
> +    children
> +      .filter(child => !child.hasAttribute("context"))
> +      .forEach(
> +        child => child.setAttribute("context", this.getAttribute("itemcontext")
> +        ));

indention off here. shouls align with .forEach

@@ +874,5 @@
> +
> +  scrollToIndex(index) {
> +    let box = this.scrollbox;
> +    let item = this.getItemAtIndex(index);
> +    if (!item) { return; }

please put return on it's own line

@@ +885,5 @@
> +  }
> +
> +  insertItemAt(index, attachment, name) {
> +    const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +    let item = this.ownerDocument.createElementNS(XULNS, "richlistitem");

just use this.ownerDocument.createXULElement

@@ +965,5 @@
> +
> +    if (this._childNodes.length == 0) {
> +      return 0;
> +    } else if (this._childNodes.length <= itemsPerRow) {
> +      return 1;

no else after return

@@ +978,5 @@
> +  _setImageSize() {
> +    let size = this.sizes[this.view] || 16;
> +
> +    for (let i = 0; i < this._childNodes.length; i++) { this._childNodes[i].imageSize = size; }
> +  }

expand to 3 lines

@@ +991,5 @@
> +    let border = this._childNodes[0].boxObject.width -
> +      this._childNodes[0].clientWidth;
> +
> +    for (let child of this._childNodes) { width = Math.max(width, child.scrollWidth); }
> +    for (let child of this._childNodes) { child.width = width + border; }

these too

::: mail/base/content/msgAttachmentView.inc.xul
@@ +100,5 @@
>                                       defaultset="attachmentSaveAll"/>
>                            </toolbox>
>                          </hbox>
> +                        <richlistbox is="attachment-list"
> +                                     id="attachmentList"

please move it to 

 <richlistbox is="attachment-list" id="attachmentList"

And the same thing elsewhere (is and id on the same line)

::: mail/base/content/msgHdrView.js
@@ +2625,5 @@
>    var list = document.getElementById("attachmentList");
>    list.clearSelection();
>  
> +  while (list.attachmentListWrapper.hasChildNodes())
> +    list.attachmentListWrapper.lastChild.remove();

please add braces for this loop

Comment 34

[Arshad Khan [:arshad]]

The selection is also a mess. Once I click one of the attachments, the selection doesn't stick.

This is the issue that I want to tackle in next bug.

Comment 35

[Arshad Khan [:arshad]]

Write a new message, add two attachments, hit DOWN. I get JavaScript error: chrome://messenger/content/mailWidgets.js, line 864: TypeError: box is null
(which is in the added code).

If you se the console without this patch, you will find the same error their with xbl implementation. The reason is vertical orient attachmentList dont have scrollbox.

Comment 36

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 37

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 38

[Arshad Khan [:arshad]]

fixed the box is null error.

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 9049262 [details] [diff] [review]
attachmentlist-elems.patch

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

The focus bug is pretty bad, but alright, we can do that in a follow-up. 

You didn't address the review comment about eventlisteners. Please just attach one of each kind, then use a switch. 
Perhaps also move attaching them to the connectedCallback()

the connectedCallback() also needs this.hasChildNodes() bailing

::: mail/base/content/mailWidgets.js
@@ +982,5 @@
> +  /**
> +   * Only used by attachmentlist with horizontal orient.
> +   */
> +  setOptimumWidth() {
> +    if (this._childNodes.length == 0) { return; }

please put this on three rows

Comment 40

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 41

[Magnus Melin [:mkmelin]]

Comment on attachment 9050162 [details] [diff] [review]
attachmentlist-elems.patch

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

r=mkmelin with the comments below fixed. And do file the followup for focus problem

::: mail/base/content/mailWidgets.js
@@ +650,5 @@
> + *
> + * @extends {MozElements.RichListBox}
> + */
> +
> +class MozAttachmentlist extends MozElements.RichListBox {

nit: no empty line between documentation and class

@@ +725,5 @@
> +      return;
> +    }
> +
> +    this.sizes = { small: 16, large: 32, tile: 32 };
> +    this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);

messenger needs to be set up in the constructor, otherwise someone might create the object and try appending stuff to it before messenger is set up -> things blow up

@@ +854,5 @@
> +      }
> +    }
> +
> +    // If we get here, something is very wrong.
> +    Cu.reportError("Couldn't get index of first visible row for attachmentlist!\n");

I think we can remove this

Comment 42

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 43

[Arshad Khan [:arshad]]

Try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=093894cd55b74d17423e79c9242a83b75e06c797

Comment 44

[Jorg K (CEST = GMT+2)]

You need to wait for bug 1533085 because 14 attachment-related (sub)tests are currently switched off. The problem should be fixed with an upcoming M-C merge.

Comment 45

[Jorg K (CEST = GMT+2)]

All relevant tests are back, so please do another try run.

Comment 46

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ef584eb7f0ae0bad3849f594c18551f07f99cae

Comment 47

[Magnus Melin [:mkmelin]]

the mozmill attachment/test-attachment.js failures look very much related to this bug

Comment 48

[Jorg K (CEST = GMT+2)]

Wow, yes, they don't fail on trunk.

Comment 49

[Magnus Melin [:mkmelin]]

Please also adjust for bug 1535725 (don't use boxObject)

Comment 50

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Comment 51

[Arshad Khan [:arshad]]

new try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bd30281b2243ed8fa897e766504ca0b478ceb4a6

Comment 52

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

new try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d5e30954fcd65a5594d174447914019285c1336a

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 9061223 [details] [diff] [review]
attachmentlist-elems.patch

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

This seems ready to land. Let's address the comments in a followup

::: mail/base/content/mailWidgets.js
@@ +1229,5 @@
> +  }
> +
> +  _addChildren() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +      <scrollbox flex="1" style="overflow: auto;">

wonder if the list shouldn't be wrapped in a scrollbox instead...

@@ +1241,5 @@
> +    return this.querySelector("scrollbox");
> +  }
> +
> +  get attachmentListWrapper() {
> +    return this.querySelector("hbox.attachmentlist-wrapper");

do we need the attachmentListWrapper?

@@ +1368,5 @@
> +    }
> +    box.scrollTop = item.getBoundingClientRect().y - box.getBoundingClientRect().y;
> +  }
> +
> +  appendItem(attachment, name) {

I think this isn't quite correct, the setup in connectedCallback could be prevented since there are now two paths to this._addChildren. 
In practice we don't do that atm, but it should still be fixed

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4192b7f68d58
Migrate attachmentlist-* bindings to custom element. r=mkmelin

Comment 55

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/0880f94b2f3155c2500c1eabaca0e246a4ddd66a
Backed out changeset 4192b7f68d58 (bug 1523607) for test failures. a=backout DONTBUILD

This breaks mozmill/composition/test-attachment.js | test-attachment.js::test_attachment_reordering. This test doesn't run on Linux, therefore you didn't see the failure in the try run. Always run at least Linux and Mac on try, at least during the final stages.

Comment 56

[alta88]

this patch, as is, will regress this change:
https://hg.mozilla.org/comm-central/diff/d167921e42eba41654952c307cec2f0fcaa022d2/mail/base/content/mailWidgets.xml

Comment 57

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e931672fe8e7ff774568afde172416351c99a18e

Comment 58

[Arshad Khan [:arshad]]

Attachment: attachmentlist-elems.patch

Green try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3e30ed0ae1f45e7e3e4b47e711ed06d5562adf08

Comment 59

[Jorg K (CEST = GMT+2)]

Maybe we give this another quick review:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9061223&action=interdiff&newid=9065978&headers=1
Looks OK to me, mostly removal on .scrollbox and .attachmentListWrapper. Interdiff confused on messengercompose.xul.

Comment 60

[Arshad Khan [:arshad]]

https://bugzilla.mozilla.org/attachment.cgi?oldid=9061223&action=interdiff&newid=9065978&headers=1 Magnus, could you please give it a quick look?

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/38204c5426e0
Migrate attachmentlist-* bindings to custom element. r=mkmelin

Comment 62

[Magnus Melin [:mkmelin]]

Comment on attachment 9065978 [details] [diff] [review]
attachmentlist-elems.patch

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

Looks pretty good, tough I find dragging around attachments in compose result in 

JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 5909: TypeError: target.matches is not a function

::: mail/base/content/mailWidgets.js
@@ +1134,5 @@
> +  constructor() {
> +    super();
> +
> +    this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);
> +    this._setupEventListeners();

_setupEventListeners() is only called once, so I'd inline it

::: mail/base/content/msgHdrView.js
@@ +2560,5 @@
>  
>      if (updateFocus)
>        attachmentList.focus();
> +
> +    attachmentList.selectItem(attachmentList.firstChild);

not sure the first item is supposed to get selected. did the old code do this?

Comment 63

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #62)

Comment on attachment 9065978 [details] [diff] [review]
attachmentlist-elems.patch

Review of attachment 9065978 [details] [diff] [review]:

Looks pretty good, tough I find dragging around attachments in compose
result in

JavaScript error:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line
5909: TypeError: target.matches is not a function

this is not related to this patch. it was already there and it has to do with some richlistbox stuff.

::: mail/base/content/mailWidgets.js
@@ +1134,5 @@

• constructor() {
• super();
• this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);
• this._setupEventListeners();

_setupEventListeners() is only called once, so I'd inline it

all right, I ll do that in Bug 1547699.

::: mail/base/content/msgHdrView.js
@@ +2560,5 @@

 if (updateFocus)
   attachmentList.focus();

• attachmentList.selectItem(attachmentList.firstChild);

not sure the first item is supposed to get selected. did the old code do
this?

I guess I added it because when you open the attachmentlist with horzontal orient then the first attachmentlist need to be selected. I ll check it again in Bug 1547699.

Comment 64

[Jorg K (CEST = GMT+2)]

(In reply to Arshad Khan [:arshad] from comment #63)

JavaScript error:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line
5909: TypeError: target.matches is not a function

this is not related to this patch. it was already there and it has to do with some richlistbox stuff.

Confirmed. We should fix it though. Also, you can't drag an attachment to the end to be the last item in the list.

Comment 65

[Jorg K (CEST = GMT+2)]

You forgot to remove https://searchfox.org/comm-central/source/common/bindings/richlistbox.xml, right?

Comment 66

[Arshad Khan [:arshad]]

yes i ll remove it in Bug 1547699.