1531305 - [de-xbl] removal/conversion of agenda-list-bindings (used in today pane): agenda-base-richlist-item, agenda-checkbox-richlist-item, agenda-richlist-item

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

de-XBL the bindings used for the richlistbox in the Today pane: agenda-base-richlist-item, agenda-checkbox-richlist-item, agenda-richlist-item

https://searchfox.org/comm-central/source/calendar/base/content/agenda-listbox.xml#11

Only used at https://searchfox.org/comm-central/rev/10f11d2e56748e92fa3365cbe1e68a55a20d2a67/calendar/base/content/today-pane.xul#353-364

Their methods are called from agenda-listbox.js, a
https://searchfox.org/comm-central/source/calendar/base/content/agenda-listbox.js

It's possible to convert these items to Custom Element, but given that they don't have too much behaviour, and are used in a very narrow way, it's seems we can just move the functionality to be handled inside agenda-listbox.js

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

I have converted all the bindings to custom elements, did not move it to agenda-listbox because I thought it will make things hard to understand.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9048979 [details] [diff] [review]
De-XBL_agenda-list.patch

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

::: calendar/base/content/agenda-list.js
@@ +7,5 @@
> +/* global MozXULElement, MozElements */
> +{
> +    /**
> +     * MozAgendaBaseRichlistItem widget is the base widget for agenda
> +     * richlistitems: agenda-allday-richlist-item and agenda-richlist-item.

perhaps not specify which here, focus on the functionality. which items inherits could potentially change

@@ +19,5 @@
> +            super();
> +
> +            this.addEventListener("click", (event) => {
> +                if (event.detail == 1) {
> +                    agendaListbox.onSelect(this);

doesn't eslint complain about this agendaListbox coming up from nowhere?
./mach lint comm/calendar/base/content/agenda-list.js says no errors though. strange.

It really shows why I suggested a CE for this might not be super right... The widgets need to stand independently, or else it makes for very confusing code...

@@ +40,5 @@
> +
> +        get occurrence() {
> +            return this.mOccurrence;
> +        }
> +    }

mOccurrence is never used by this base class, so get occurrrence is thought to e an abstract method.
Given there is very little functionality in the MozAgendaBaseRichlistItem class, I think it's probably preferable to just remove it and live with the ~10 row code duplication (or potentially that code should live somewhere else)

@@ +44,5 @@
> +    }
> +
> +    MozXULElement.implementCustomInterface(MozAgendaBaseRichlistItem,
> +        [Ci.nsIDOMXULSelectControlItemElement]);
> +

put ); on it's own row, looks nicer

@@ +56,5 @@
> +        static get inheritedAttributes() {
> +            return { ".agenda-checkbox": "selected,label,hidden,disabled", };
> +        }
> +
> +        connectedCallback() {

probably add an if(delayConnectedCallback()) call here? see bug 1495946 comment 22

also, connectedCallback() can potentially run more than once so you should not add the child more than once. probably do a if (this.hasChildNodes()) return;

@@ +94,5 @@
> +        }
> +    }
> +
> +    MozXULElement.implementCustomInterface(MozAgendaCheckboxRichlistItem,
> +        [Ci.nsIDOMXULSelectControlItemElement]);

same as earlier, new line

@@ +118,5 @@
> +            this.addEventListener("dragstart", (event) => {
> +                invokeEventDragSession(this.mAllDayItem.occurrence.clone(), this);
> +                event.stopPropagation();
> +                event.preventDefault();
> +            }, true);

can the eventlisterer adding just move to connectedCallback() ? here and on other places

@@ +317,5 @@
> +        }
> +    }
> +
> +    customElements.define("agenda-base-richlist-item", MozAgendaBaseRichlistItem,
> +        { "extends": "richlistitem" });

just move each  customElements.define to after it's class body

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9049859 [details] [diff] [review]
De-XBL_agenda-list.patch

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

Looks pretty good. 
I think you should hg mv --after calendar/base/content/agenda-listbox.xml  calendar/base/content/agenda-list.js

There is also some slight bitrot

::: calendar/base/content/agenda-list.js
@@ +6,5 @@
> +
> +/* global MozXULElement, MozElements */
> +{
> +    /**
> +     * MozAgendaCheckboxRichlistItem widget displays the chechbox with dropdown

The ....

@@ +64,5 @@
> +    customElements.define("agenda-checkbox-richlist-item", MozAgendaCheckboxRichlistItem,
> +        { "extends": "richlistitem" });
> +
> +    /**
> +     * MozAgendaAlldayRichlistItem widget displays the information about

The...

@@ +66,5 @@
> +
> +    /**
> +     * MozAgendaAlldayRichlistItem widget displays the information about
> +     * all day event: i.e. event start time, icon and calendar-month-day-box-item.
> +     * It is shown under agenda-checkbox-richlist-item dropwon as richlistitem.

typo dropdown.
as a

@@ +185,5 @@
> +    customElements.define("agenda-allday-richlist-item", MozAgendaAlldayRichlistItem,
> +        { "extends": "richlistitem" });
> +
> +    /**
> +     * MozAgendaAlldayRichlistItem widget displays the information about

THe...

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9051740 [details] [diff] [review]
De-XBL_agenda-list.patch

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

Seems to work fine

::: calendar/base/content/agenda-list.js
@@ +8,5 @@
> +   invokeEventDragSession, setBooleanAttribute, hideElement, onMouseOverItem */
> +{
> +    /**
> +     * The MozAgendaCheckboxRichlistItem widget displays the chechbox with dropdown
> +     * of today, tomorrow and upcoming events. It is shown in Today Pane window.

in the Today Pane. (it's not a window). But I don't understand what it says. 

The MozAgendaCheckboxRichlistItem widget is typically used to display the Today, Tomorrow, and Upcoming headers of the Today Pane listing.

I have to say it's strange that a checkbox is involved here at all... Maybe we should take the opportunity to rename the element to agenda-header-richlist-item.

@@ +14,5 @@
> +     * @extends {MozElements.MozRichlistitem}
> +     */
> +    class MozAgendaCheckboxRichlistItem extends MozElements.MozRichlistitem {
> +        static get inheritedAttributes() {
> +            return { ".agenda-checkbox": "selected,label,hidden,disabled", };

would remove the trailing comma

@@ +67,1 @@
>  

I'd keep this on one line

@@ +183,5 @@
> +        MozAgendaAlldayRichlistItem, [Ci.nsIDOMXULSelectControlItemElement]
> +    );
> +
> +    customElements.define("agenda-allday-richlist-item", MozAgendaAlldayRichlistItem,
> +        { "extends": "richlistitem" });

one line (calendar allows 120ch, iirc)

@@ +259,5 @@
>              periodStartDate.isDate = true;
>              let periodEndDate = aPeriod.end.clone();
>              periodEndDate.day--;
>              let start = this.mOccurrence[cal.dtz.startDateProp(this.mOccurrence)]
> +                .getInTimezone(cal.dtz.defaultTimezone);

aligning with dots is what's commonly used, so don't change that

@@ +264,2 @@
>              let end = this.mOccurrence[cal.dtz.endDateProp(this.mOccurrence)]
> +                .getInTimezone(cal.dtz.defaultTimezone);

same here

Comment 7

[Khushil Mistry [:khushil324]]

I am getting this error on eslint in agenda-listbox.js : Identifier name 'is' is too short (< 3). id-length (eslint)
when doing this : document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });

Any suggestions ?

Comment 8

[Paul Morris [:pmorris]]

I would just disable eslint for that line. I think something like this should do it:

// eslint-disable-next-line id-length
document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });

Comment 9

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #8)

I would just disable eslint for that line. I think something like this should do it:

// eslint-disable-next-line id-length
document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });

Thanks. It worked.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

Comment 11

[Paul Morris [:pmorris]]

Glad that worked. I've compared my latest patch from bug 1534382 with the latest one here. Here are the differences I thought were worth mentioning:

• double clicks: I added an event handler to prevent double clicks from opening two dialogs -- a dialog for editing the task double clicked on, and a dialog creating a new task. I haven't tested Khushil's patch to see if this is happening, but it's worth checking.

        // Prevent double clicks from opening a dialog to create a new event.
        this.addEventListener("dblclick", (event) => {
            event.stopPropagation();
        });
  

• file names: I moved agenda-listbox.js to agenda-listbox-utils.js, and then made agenda-listbox.xml into the new agenda-listbox.js. I think this is a bit clearer than having those two files named agenda-list.js and agenda-listbox.js. It makes their relationship easier to understand.

• delayConnectedCallbacks: I noticed there were no if (this.delayConnectedCallback()) checks in two of the connectedCallbacks. I thought we were always including those.

• event listeners: I had put event listeners in the constructor, rather than in connectedCallback, which seems to be the recommended place to put them, from what I've read.

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #11)

• file names: I moved agenda-listbox.js to agenda-listbox-utils.js, and then
  made agenda-listbox.xml into the new agenda-listbox.js. I think this is a
  bit clearer than having those two files named agenda-list.js and
  agenda-listbox.js. It makes their relationship easier to understand.

True

• delayConnectedCallbacks: I noticed there were no if (this.delayConnectedCallback()) checks in two of the connectedCallbacks. I
  thought we were always including those.

I think we should yes.

• event listeners: I had put event listeners in the constructor, rather than
  in connectedCallback, which seems to be the recommended place to put them,
  from what I've read.

I read that somewhere too, but on the other hand it's good to delay setup as long as possible. Also especially for click handlers, any inline onclicks won't get the expected priority otherwise (should such exist). So I'm not really convinced.

Khuhil, can you look into the points mentioned by Paul, and update your patch accordingly, or then we could also go with Paul's patch.

Comment 13

[Khushil Mistry [:khushil324]]

Yes, I will update the patch.

Comment 14

[Paul Morris [:pmorris]]

(In reply to Khushil Mistry [:khushil324] from comment #13)

Yes, I will update the patch.

Oh, yes, better to update your patch rather than use mine. There were a number of places where yours was more polished. I just didn't mention them because I was focusing on things to add or change.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

Comment 16

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #11)

• double clicks: I added an event handler to prevent double clicks from opening two dialogs -- a dialog for editing the task double clicked on, and a dialog creating a new task. I haven't tested Khushil's patch to see if this is happening, but it's worth checking.

        // Prevent double clicks from opening a dialog to create a new event.
        this.addEventListener("dblclick", (event) => {
            event.stopPropagation();
        });
  

I didn't find any problem with this. It was opening single dialog on double clicking.

Comment 17

[Magnus Melin [:mkmelin]]

Double clicking an event in the Today Pane I get the double dialog problem Paul mentions

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9052712 [details] [diff] [review]
De-XBL_agenda-list.patch

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

::: calendar/base/content/agenda-listbox-utils.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

you didn't hg copy this file, so it's really hard to see what changed. (You can use hg cp --after to record the change)

It uses a funny way of defining the functions on the object... but that's for another bug.

::: calendar/base/content/today-pane.xul
@@ +351,4 @@
>                  </menupopup>
>                </menu>
>              </menupopup>
> +            <agenda-header-richlist-item id="today-header"

maybe it's clearer to have these be

<richlistitem is="agenda-richlist-item" 

... and class="agenda-richlist-item"

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda-list.patch

I have added EventListener for double click to stop propagation in agenda-allday-richlist-item and agenda-richlist-item.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9052922 [details] [diff] [review]
De-XBL_agenda-list.patch

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

Looks ok. Please have someone from calendar do a final review.

::: calendar/base/content/agenda-listbox-utils.js
@@ +287,5 @@
>  agendaListbox.addItemBefore = function(aNewItem, aAgendaItem, aPeriod, visible) {
>      let newelement = null;
>      if (aNewItem.startDate.isDate) {
> +        // eslint-disable-next-line id-length
> +        newelement = document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });

I think we should instead add "is" to the whitelist: https://searchfox.org/comm-central/source/calendar/.eslintrc.js#457

::: calendar/base/content/agenda-listbox.js
@@ +64,5 @@
>  
> +    /**
> +     * The MozAgendaAlldayRichlistItem widget displays the information about
> +     * all day event: i.e. event start time, icon and calendar-month-day-box-item.
> +     * It is shown under agenda-header-richlist-item dropwon as richlistitem.

It is typically shown under the the agenda-header-richlist-item dropdown.

::: calendar/base/content/today-pane.xul
@@ +351,5 @@
>                  </menupopup>
>                </menu>
>              </menupopup>
> +            <richlistitem is="agenda-header-richlist-item"
> +                          id="today-header"

would keep is and id on same line for these, so that it's easy to spot what's used where

Comment 21

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

Comment on attachment 9052922 [details] [diff] [review]
De-XBL_agenda-list.patch

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

::: calendar/base/content/agenda-listbox-utils.js
@@ +287,5 @@
>  agendaListbox.addItemBefore = function(aNewItem, aAgendaItem, aPeriod, visible) {
>      let newelement = null;
>      if (aNewItem.startDate.isDate) {
> +        // eslint-disable-next-line id-length
> +        newelement = document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });

+1 on adding `is` to the whitelist instead of using eslint-disable-next-line.

Comment 22

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda_list.patch

Comment 23

[Khushil Mistry [:khushil324]]

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

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_agenda_list.patch

Comment 25

[Jorg K (CEST = GMT+2)]

Sorry, but your try has failures:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/testTodayPane.js | testTodayPane.js::testTodayPane

Comment 27

[Khushil Mistry [:khushil324]]

Attachment: Bug-1531305_de-xbl_agenda.patch

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

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 9058106 [details] [diff] [review]
Bug-1531305_de-xbl_agenda.patch

What is the question? Yes, the Z4, X1 and X4 failures are known/expected. Z4 will go away if you rebase.

What did you do to fix the failure from
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a0d79144fab12d6ffba4263a4f72593e6298f44e&selectedJob=236162397 ?

Interdiff has gone mad, so I can't see whether this is a significant change. I also don't understand why your debug tests are totally orange with "We can't proceed without downloading symbols". That happened before, is that bad luck?

f?Magnus to check the patch again.

Comment 29

[Khushil Mistry [:khushil324]]

Last time, one of the test, testTodayPane failed. So I have updated that test in the latest patch. It passed but I am seeing lots of tests with orange so I am not sure why that is happening.

Comment 30

[Jorg K (CEST = GMT+2)]

Well, all the opt failures are expected, no idea about the debug stuff. Let's take it like this.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f57c1c1b7400
[de-xbl] convert agenda-list-bindings to custom elements. r=philipp