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

RESOLVED FIXED in 7.0

Status

enhancement
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: mkmelin, Assigned: khushil324)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Details

Attachments

(1 attachment, 8 obsolete attachments)

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

Status: NEW → ASSIGNED
Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review

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.

Attachment #9048979 - Flags: review?(philipp)
Attachment #9048979 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9048979 - Flags: review?(philipp)
Attachment #9048979 - Flags: feedback?(mkmelin+mozilla)
Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review
Attachment #9048979 - Attachment is obsolete: true
Attachment #9049859 - Flags: feedback?(mkmelin+mozilla)
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...
Attachment #9049859 - Flags: feedback?(mkmelin+mozilla) → feedback+
Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review
Attachment #9049859 - Attachment is obsolete: true
Attachment #9051740 - Flags: review?(mkmelin+mozilla)
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
Attachment #9051740 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9051740 - Flags: feedback?(paul)

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 ?

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" });

(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.

Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review
Attachment #9051740 - Attachment is obsolete: true
Attachment #9051740 - Flags: feedback?(paul)
Attachment #9052660 - Flags: review?(mkmelin+mozilla)
Attachment #9052660 - Flags: feedback?(paul)

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.

Attachment #9052660 - Flags: feedback?(paul)

(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.

Attachment #9052660 - Flags: review?(mkmelin+mozilla)

Yes, I will update the patch.

(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.

Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review
Attachment #9052660 - Attachment is obsolete: true
Attachment #9052712 - Flags: review?(mkmelin+mozilla)
Attachment #9052712 - Flags: feedback?(paul)

(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.

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

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"
Attachment #9052712 - Flags: review?(mkmelin+mozilla)
Attachment #9052712 - Flags: feedback?(paul)
Posted patch De-XBL_agenda-list.patch (obsolete) — Splinter Review

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

Attachment #9052712 - Attachment is obsolete: true
Attachment #9052922 - Flags: review?(mkmelin+mozilla)
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
Attachment #9052922 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9052922 - Flags: review?(philipp)
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.
Attachment #9052922 - Flags: review?(philipp) → review+
Posted patch De-XBL_agenda_list.patch (obsolete) — Splinter Review
Attachment #9052922 - Attachment is obsolete: true
Attachment #9053693 - Flags: review+
Posted patch De-XBL_agenda_list.patch (obsolete) — Splinter Review
Attachment #9053693 - Attachment is obsolete: true
Attachment #9053697 - Flags: review+
Keywords: checkin-needed

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

Keywords: checkin-needed
Duplicate of this bug: 1534382
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.
Flags: needinfo?(jorgk)
Attachment #9058106 - Flags: feedback?(mkmelin+mozilla)

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.

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

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.0
Attachment #9058106 - Flags: feedback?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.