1522473 - [de-xbl] Migrate attendees-list to custom element.

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Conversion like this a little bit tricky.. This element uses a method of Richlistbox but only under certain conditon which depends upon layout of the attendees-list..

Comment 3

[Arshad Khan [:arshad]]

Comment on attachment 9041709 [details] [diff] [review]
attendees-list.patch

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

hey I am adding a lot of css here.. don't know why the xul layout wasn't working.. I tried some css options but this one gives the most appropriate look.

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 9041709 [details] [diff] [review]
attendees-list.patch

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

Only tried briefly but it looks good.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +610,5 @@
> +    flex: 1;
> +    display: flex;
> +}
> +
> +#attendees-list hbox:nth-child(1), #attendees-list hbox:nth-child(2) {

Please every selector on its own line.

@@ +619,5 @@
> +#attendees-list hbox:nth-child(3) {
> +    flex: 1;
> +}
> +
> +#attendees-list hbox textbox {

Instead of this line, could you try: .textbox-addressingWidget {

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9041709 [details] [diff] [review]
attendees-list.patch

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

Seems to work, but there are some things that should be cleaned up

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +10,5 @@
> +if (!customElements.get("richlistbox")) {
> +    delete document.createElement("richlistbox");
> +}
> +
> +class MozAttendeesList extends customElements.get("richlistbox") {

Let's add documentation to these as we go, and see which thing is which. 
Like https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1521016&attachment=9040701

@@ +19,5 @@
> +            if (event.button != 0) {
> +                return;
> +            }
> +
> +            function cycle(values, current) {

need to use 
let cycle = function(.....

... not to get strict js errors.

@@ +81,5 @@
> +            }
> +            if (event.originalTarget.localName == "input") {
> +                switch (event.key) {
> +                    case "Delete":
> +                    case "Backspace":

While we're here
case KeyEvent.DOM_VK_DELETE:
case KeyEvent.DOM_VK_BACK_SPACE:

@@ +133,5 @@
> +                return;
> +            }
> +            if (event.originalTarget.localName == "input") {
> +                switch (event.key) {
> +                    case "ArrowUp":

these too

@@ +172,5 @@
> +        this.mPopupOpen = false;
> +
> +        this.mMaxAttendees = 0;
> +
> +        window.addEventListener("load", this.onLoad.bind(this), true);

ugh!

I don't think this is the sort of thing a custom element should be allowed to do. 
I wonder if you're in luck though. This is in the (delayed) connectedCallback, so perhaps everything is ready?

@@ +226,5 @@
> +        return attendees;
> +    }
> +
> +    get organizer() {
> +        const { MailServices } = ChromeUtils.import("resource:///modules/MailServices.jsm");

MailServices is bascially universal in mail. You can just put it on top of the file
Besides, it's now used in attendees() too, where it would not be defined.

@@ +287,5 @@
> +
> +        // this trigger the continous update chain, which
> +        // effectively calls this.onModify() on predefined
> +        // time intervals [each second].
> +        let self = this;

no need for self here I think, as you're now using =>

@@ +296,5 @@
> +        callback();
> +    }
> +
> +    onInitialize() {
> +        let args = window.arguments[0];

ugh!

I don't know why the onLoad and the onInitialize() are split into two. 

onInitialize() should not be allowed to dig into the window arguments though. I think there should be some outer code that sets the element up with the needed information at the right time.

@@ +958,5 @@
> +        this.mMaxAttendees--;
> +    }
> +}
> +
> +customElements.define("attendees-list", MozAttendeesList);

perhaps calendar-event-attendees-list?

Comment 6

[Magnus Melin [:mkmelin]]

Oh, and do use hg cp --after, to record the blame.

Comment 7

[Arshad Khan [:arshad]]

onInitialize() should not be allowed to dig into the window arguments
though. I think there should be some outer code that sets the element up
with the needed information at the right time.

Why not?

Comment 8

[Arshad Khan [:arshad]]

Instead of this line, could you try: .textbox-addressingWidget {

This will target every element with this class which I dont want.

Comment 9

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 10

[Arshad Khan [:arshad]]

There are some issues with the css of the whole attendees dialog. fixed position is used which doesn't work well with overflow property. Some elements gets a bit chopped when dialog height is resized. I think it should be fixed in different bug.

Comment 11

[Magnus Melin [:mkmelin]]

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

onInitialize() should not be allowed to dig into the window arguments
though. I think there should be some outer code that sets the element up
with the needed information at the right time.

Why not?

That totally destroys the idea that this would be a component that could be reused (well, it's not, but that's a different issue). It leads to magic behaviour that is hard to understand. If I put a component into a page I expect it to be configurable at that place through attributes, or to be configured through global loading code. NOT that it would partly configure itself depending on what the window arguments happen to be, and certainly not somewhere deep inside the basically private methods of the component.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9042019 [details] [diff] [review]
attendees-list.patch

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

Please reflag for feedback once the comments have been addressed.

Comment 13

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 14

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 15

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 16

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 17

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9044567 [details] [diff] [review]
attendees-list.patch

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

Please 
hg cp calendar/base/content/dialogs/calendar-event-dialog-attendees.xml calendar/base/content/dialogs/calendar-event-dialog-attendees.js to preserve some blame, and easier review

Trying to close the dialog I get
JavaScript error: chrome://calendar/content/calendar-event-attendees-custom-elements.js, line 211: ReferenceError: MailServices is not defined

There is something wrong with the deletion of rows of invited (using backspace at least) - it doesn't quite work, not "removing the icon" like it used to

When loading the dialog I now see a slight visual hickup: the horizontal lines of the grid are showing up after maybe half a seconds delay. I didn't see that before, and not sure if it's due to this patch but thought I'd note it down anyway.

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Class extending RichListBox. AttendeesList is used to add/remove attendee
> + * to an event.

MozAttendeesList is a widget allowing adding and removing of attendees of an event. 
It shows if attendee if required or optional, the attendee status, type and adddress. 
It is typically found in the Invite Attendees dialog.

@@ +722,5 @@
> +    }
> +
> +    /**
> +     * Gets the next row from the top down.
> +     */

there's a bunch of places  like this where the documentation indention seems one space off

@@ +813,5 @@
> +                    : '"' + aMsgIAddressObject.name + '"'
> +                ) + " <" + aMsgIAddressObject.email + ">";
> +            } else {
> +                return aMsgIAddressObject.toString();
> +            }

while here, fix the else after return (remove the else and just return)

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +1004,5 @@
> +    // assume we're the organizer [in case that the calendar
> +    // does not support the concept of identities].
> +    let organizerID = ((organizer && organizer.id) ?
> +        organizer.id
> +        : calendar.getProperty("organizerId"));

: on the previous line (I think, unless calendar had something special wrt this)

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +31,5 @@
>  
>    <!-- Javascript includes -->
> +  <script type="application/javascript"
> +          src="chrome://calendar/content/calendar-event-attendees-custom-elements.js"/>
> +  <script type="application/javascript" 

trailing whitespace

Comment 19

[Arshad Khan [:arshad]]

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

Comment on attachment 9044567 [details] [diff] [review]
attendees-list.patch

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

Please
hg cp calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
calendar/base/content/dialogs/calendar-event-dialog-attendees.js to preserve
some blame, and easier review

calendar-event-dialog-attendees-custom-elements.js?

Comment 20

[Arshad Khan [:arshad]]

@@ +722,5 @@

• }
• /**

• * Gets the next row from the top down.
  

• */
  

there's a bunch of places like this where the documentation indention seems
one space off

how is it off?

Comment 21

[Arshad Khan [:arshad]]

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

@@ +722,5 @@

• }
• /**

• * Gets the next row from the top down.
  

• */
  

there's a bunch of places like this where the documentation indention seems
one space off

how is it off?

I mean to say that I dont see any other option better than this. I see this identation used everywhere.

Comment 22

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

The ui hickup is already there and not caused by this patch. I have fixed the backspace buttona and arrow button issue.

Comment 23

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 9045269 [details] [diff] [review]
attendees-list.patch

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

Seem to be working now.

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +888,2 @@
>  
> +customElements.define("calendar-event-attendees-list", MozAttendeesList);

Please make the class correspond to the name. MozCalendarEventAttendeesList

Comment 25

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9045906 [details] [diff] [review]
attendees-list.patch

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

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +31,5 @@
> +        this.mPopupOpen = false;
> +
> +        this.mMaxAttendees = 0;
> +
> +        this.addEventListener("click", (event) => {

Maybe we can move the actual function into a method on the custom element, make sure to bind those methods to the instance on the constructor, and then addEventListener the instance method? This would avoid us doing all the work in the constructor. From a separation perspective, it would also in theory allow to extend this binding and extend the click handler easily.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +612,5 @@
> +    display: flex;
> +}
> +
> +#attendees-list hbox:nth-child(1),
> +#attendees-list hbox:nth-child(2) {

Can we give these a class instead of using nth-child?

Comment 28

[Arshad Khan [:arshad]]

Attachment: part-2-attendees-list-nits.patch

Comment 29

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9047750 [details] [diff] [review]
part2-attendees-list-nits.patch

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

Removing review until comments are fixed, as discussed via IRC.

Comment 31

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 32

[Arshad Khan [:arshad]]

Comment on attachment 9047929 [details] [diff] [review]
part2-attendees-list-nits.patch

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

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +910,1 @@
>      setFocus(row) {

Nog messing with this - https://stackoverflow.com/questions/779379/why-is-settimeoutfn-0-sometimes-useful . This timeout was already there so probably it should be there.

Comment 33

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 34

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 35

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 36

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 37

[Arshad Khan [:arshad]]

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

Comment 38

[Arshad Khan [:arshad]]

I haven used {object} instead of {node} at some places so please dont mind that, I ll fix it in next patch.

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9047953 [details] [diff] [review]
part2-attendees-list-nits.patch

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

Sorry for all the comments. I tried to explain the concept behind it instead of just flagging it, hope this helps to understand. Let me know if you have questions.

::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js
@@ +112,5 @@
>  
> +    /**
> +     * Sets mIsReadOnly property.
> +     *
> +     * @param {Boolean} val       New mIsReadOnly value

When writing jsdoc, consider the class a black box. If you generate this and people only read the generated jsdoc, they won't know what mIsReadOnly means.

You should rather describe what it means. The attendee list can be in a readonly state, so you'd have something like "Controls the read-only state of the attendee list, not allowing XYZ when set".

The same goes for the other properties.

@@ +146,5 @@
>          return this.mIsInvitation;
>      }
>  
> +    /**
> +     * @returns {array}       Array of attendee objects for an event

These should have a description, whereas the @returns line can be short. Also, with array you rather want to use the type of the array elements and []. I guess it is a calIAttendee?

```
/**
 * The attendees shown in this attendee list.
 *
 * @returns {calIAttendee[]}    The attendees of the list.
 */
```


Same goes for the others.

@@ +268,5 @@
>          return val;
>      }
>  
> +    /**
> +     * Handler for click event.

This needs more description, what does the handler do? Not in very much detail, but a high level description.

@@ +337,5 @@
>  
>      /**
>       * Appends a new row using an existing attendee structure.
> +     *
> +     * @param {Object} attendee                  Attendee object

Is this an object or calIAttendee?

@@ +338,5 @@
>      /**
>       * Appends a new row using an existing attendee structure.
> +     *
> +     * @param {Object} attendee                  Attendee object
> +     * @param {Object} templateNode              Template node that need to be cloned

If there is a more specialized type, use it. Using Object is too broad and should only be done if it is a custom object. Custom objects need to be described as well.

If this is a DOM element, then you'll want Element. N.B. this shouldn't be Node, since that is the more general type. A Node can be an Element, an attribute node, etc. More details on MDN.

Check the remaining file as well.

@@ +509,5 @@
>          return newNode;
>      }
>  
> +    /**
> +     * Resolves list by the value that is passed.

You are going to have to do more than just describe the name of the function in other words. What does resolving mean here?

@@ +557,5 @@
>          return null;
>      }
>  
> +    /**
> +     * Finds attendees in the address lists.

This is much better, great!

@@ +561,5 @@
> +     * Finds attendees in the address lists.
> +     *
> +     * @param {Object} mailingList      Mailing list having address lists
> +     * @param {array} attendees         List of attendees
> +     * @param {array} allListsUri       URI of all lists

Same comment about using type[] instead of array

@@ +647,5 @@
>          userTypeIcon.setAttribute("cutype", newAttendee.userType);
>      }
>  
> +    /**
> +     * Resolves list.

This needs to be described better

@@ +683,5 @@
>          }
>      }
>  
> +    /**
> +     * Emits modify method with list having attendees data.

This is the right kind of description for event handlers, great! Emits a |modify| DOM event...

@@ +715,5 @@
>          this.dispatchEvent(event);
>      }
>  
> +    /**
> +     * Method setting the tooltip of attendee icons based on their role.

This one is great as well!

@@ +717,5 @@
>  
> +    /**
> +     * Method setting the tooltip of attendee icons based on their role.
> +     *
> +     * @param {node} targetIcon       TargetIcon node

Node should be uppercase. Any objects should be uppercase. I've recently learned that only primitive types should be lowercase, I've been doing this wrong in other calendar code. https://github.com/gajus/eslint-plugin-jsdoc#why-not-capital-case-everything

As per earlier comment, this should be Element instead if it is a DOM Element.

@@ +755,5 @@
>          }
>      }
>  
> +    /**
> +     * Fits dummy rows in the attendee list.

Dummy rows isn't very clear, maybe describe what it actually is? I think those were the extra rows that are added to fill out space, so you could say it fills out the empty space in the attendee list with rows that do ... ?

@@ +805,5 @@
>          }
>      }
>  
> +    /**
> +     * Creates dummy item.

Same here, not sure what a dummy item is?

@@ +843,5 @@
>          return this.getElementsByTagName("richlistitem")[row - 1];
>      }
>  
> +    /**
> +     * @returns {Object}       <xul:textbox> at first row

The fact that this is a xul:textbox is an implementation detail. Describe what the method does instead.

@@ +877,5 @@
>          return this.getListItem(row).querySelector(".textbox-addressingWidget");
>      }
>  
> +    /**
> +     * @param {Object} row       Row element

Missing description here.

@@ +910,2 @@
>      setFocus(row) {
> +        // See https://stackoverflow.com/questions/779379/why-is-settimeoutfn-0-sometimes-useful to

This is a pretty long answer and very generic. You're either going to have to write why it is specifically helpful here, or if you don't know I'm fine to just drop the comment.

@@ +925,5 @@
>          }, 0);
>      }
>  
> +    /**
> +     * Creates attendee.

...with default values set.

@@ +983,5 @@
>          }
>      }
>  
> +    /**
> +     * Method that gets called when arrow key is pressed.

Describe what it does, not when it is called.

Comment 40

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 41

[Arshad Khan [:arshad]]

Some of the comments might be wrong. I have tried to write the best I could. TBH while converting an element, I understand the code technically and converts xbl/xul related things into ce stuff.. Many times I dont know some of the methods present and what they do, you guys know the widgets holistically and technically better than me so please guide me when I am wrong. I wont complain how many times I am flagged for re review.

Comment 42

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9050243 [details] [diff] [review]
part2-attendees-list-nits.patch

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

Can you apply the review comments from other patches on this first? I see various comments with mTheThing in them.

Comment 43

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 44

[Arshad Khan [:arshad]]

Attachment: part2-attendees-list-nits.patch

Comment 45

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9056402 [details] [diff] [review]
part2-attendees-list-nits.patch

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

This looks much better than the other patches, keep it up :)

Comment 46

[Arshad Khan [:arshad]]

Attachment: attendees-list-nits.patch

Comment 47

[Jorg K (CEST = GMT+2)]

Hi Arshad, looking at
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0bfcfb8422e4a34fcf25be4a49a2d96df40dd961
The X1, X4 failures are expected. If you rebase, the Z4 should be green. Z1, eventDialog/testEventDialogModificationPrompt.js looks like something you caused unless it's intermittent.

Comment 48

[Jorg K (CEST = GMT+2)]

Almost ready to go?

Comment 49

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 50

[Arshad Khan [:arshad]]

Attachment: part-2-attendees-list-nits.patch

Comment 51

[Arshad Khan [:arshad]]

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

Comment 52

[Arshad Khan [:arshad]]

Jorgk, looks like the try is ok?

Comment 53

[Jorg K (CEST = GMT+2)]

I'm confused as to what you want landed here. Both patches have the same size, so are they the same?

Th try run doesn't look good since Z2 failed completely, although that may/will be unrelated. The X1 failure has been resolved. So best to do another try. You can add Mac as well as per the e-mail I've sent you on Monday. Add one of these patches to the push:
https://hg.mozilla.org/try-comm-central/rev/ec7d128491d3e1bcc2c5519679d8ee94dec1cdd5
https://hg.mozilla.org/try-comm-central/rev/f680a8802b10c7c365ad360e5a8b607d14432cd0

Comment 54

[Arshad Khan [:arshad]]

Attachment: attendees-list.patch

Comment 55

[Arshad Khan [:arshad]]

Attachment: part-1-attendees-list.patch

Comment 56

[Arshad Khan [:arshad]]

(In reply to Jorg K (GMT+2) from comment #53)

I'm confused as to what you want landed here. Both patches have the same size, so are they the same?

Th try run doesn't look good since Z2 failed completely, although that may/will be unrelated. The X1 failure has been resolved. So best to do another try. You can add Mac as well as per the e-mail I've sent you on Monday. Add one of these patches to the push:
https://hg.mozilla.org/try-comm-central/rev/ec7d128491d3e1bcc2c5519679d8ee94dec1cdd5
https://hg.mozilla.org/try-comm-central/rev/f680a8802b10c7c365ad360e5a8b607d14432cd0

Sorry I uploaded same patch. I have fixed that now. Also I have pushed another try run with first patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c330d5c396af8ea16399c024b73e062329ae5515

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: attendees-list.patch (merged)

Merged patch. What you've done here made no sense. In the first patch you did:

copy from calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
copy to calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js

which is not a good idea to start with since and XML file is not a JS file. Moving a file pr preserve history might be OK. However, the first patch doesn't make any changes to the new file calendar-event-attendees-custom-elements.js.

In the second patch, you delete the new file again:
deleted file mode 100644
--- a/calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js

So that's really confusing and unnecessary churn. I've folded the two patches. Now that stuff that is removed from the XML file goes into an existing custom elements JS file.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1552305b49b0
Migrate attendees-list binding to custom-element. r=philipp