Arshad Khan [:arshad] Assignee
	Comment 4 • 6 years ago

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.

	Brian Grinstead [:bgrins]
	Comment 6 • 6 years ago

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?

	Arshad Khan [:arshad] Assignee
	Comment 9 • 6 years ago

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.

	Arshad Khan [:arshad] Assignee
	Comment 12 • 6 years ago

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.

	Richard Marti (:Paenglab)
	Comment 15 • 6 years ago

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.

	Magnus Melin [:mkmelin]
	Comment 17 • 6 years ago

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

	Richard Marti (:Paenglab)
	Comment 26 • 6 years ago

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

Yes, this looks now good.

	Arshad Khan [:arshad] Assignee
	Comment 28 • 6 years ago

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

changin the review to mkmelin as this is mail stuff

	Magnus Melin [:mkmelin]
	Comment 29 • 6 years ago

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?

	Arshad Khan [:arshad] Assignee
	Comment 30 • 6 years ago

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.

	Magnus Melin [:mkmelin]
	Comment 33 • 6 years ago

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

	Magnus Melin [:mkmelin]
	Comment 39 • 6 years ago

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

	Magnus Melin [:mkmelin]
	Comment 41 • 6 years ago

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

	Magnus Melin [:mkmelin]
	Comment 53 • 5 years ago

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

	Magnus Melin [:mkmelin]
	Comment 62 • 5 years ago

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?