1554646 - [de-xbl] convert the calendar-editable-item binding and derivatives calendar-month-day-box-item + calendar-event-box

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin] (away, back Aug 4)]

calendar-editable-item: https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/calendar/base/content/calendar-view-core.xml#16

Child classes:

• calendar-month-day-box-item - https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/calendar/base/content/calendar-month-view.xml#16
• calendar-event-box - https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/calendar/base/content/calendar-multiday-view.xml#2036

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl_calendar-editable-item.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22f422a094a3e6e02d6b2c9ec639ebf1635c50e2
I have updated the patch with corrections on test failures.

Comment 3

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9079758 [details] [diff] [review]
Bug-1554646_de-xbl_calendar-editable-item.patch

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

::: calendar/base/content/agenda-listbox.js
@@ +164,4 @@
>              let multiDayImage = this.querySelector(".agenda-multiDayEvent-image");
>              multiDayImage.setAttribute("type", iconType);
>              // class wrap causes allday items to wrap its text in today-pane
> +            let addWrap = this.mAllDayItem.querySelector(".calendar-event-box-container");

heh, let's rename this variable to eventBoxContainer

@@ +168,2 @@
>              addWrap.classList.add("wrap");
> +            addWrap = this.mAllDayItem.querySelector("event-detail-box");

and this (a new var), eventDetailBox

::: calendar/base/content/calendar-multiday-view.js
@@ +156,5 @@
> +    /**
> +     * The MozCalendarMonthDayBoxItem widget is used as event item in the
> +     * Day and Week views of the calendar. It displays the event name,
> +     * alarm icon and the category type color. It also displays the gripbar
> +     * components on hovering over the event.It is used to change the event

missing a space

@@ +363,5 @@
> +        }
> +
> +        /**
> +         * methods/properties
> +         */

please fix (or maybe just remove this comment, as it's not about this method)

@@ +367,5 @@
> +         */
> +        setAttribute(aAttr, aVal) {
> +            if (!this.hasChildNodes()) {
> +                this.connectedCallback();
> +            }

this doesn't look like something we should do.
connectedCallback() is a life cycle function and not to be called manually

::: calendar/base/content/calendar-views.js
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global MozElements, MozXULElement, Services, timeIndicator, invokeEventDragSession,

Should instead import Services.

@@ +486,5 @@
> +                if (this.mEditing) {
> +                    event.stopPropagation();
> +                }
> +            };
> +            // while editing, single click positions cursor, so don't propagate.

Capitalize these sentences.

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

Comment 5

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9082594 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

::: calendar/base/content/calendar-event-column.js
@@ +1287,4 @@
>                  // the packets. that's why we need to make sure at least the currently
>                  // dragged event is contained in the set of selected items.
>                  let selectedItems = this.getSelectedItems({});
> +                if (item && !selectedItems.some(aItem => aItem.hashId == item.hashId)) {

when is dragOccurrence (item) null?

::: calendar/base/content/calendar-multiday-base-view.js
@@ +872,4 @@
>              this.mSelectedItems = items || [];
>  
>              for (const item of this.mSelectedItems) {
> +                if (item) {

how can one of the selected items be null?

Comment 6

[Khushil Mistry [:khushil324]]

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

when is dragOccurrence (item) null?

When calendar-muldiday-view i.e. Week and Day views, is not focused and you try to create an event with click and drag. And that call in trace will create problem for this.mSelectedItems.

Comment 7

[Magnus Melin [:mkmelin] (away, back Aug 4)]

But it wasn't a problem in the xbl code, so why is it a problem now? That sounds like something is wrong.

Comment 8

[Khushil Mistry [:khushil324]]

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

But it wasn't a problem in the xbl code, so why is it a problem now? That sounds like something is wrong.

I found this issue on Trunk. So tried to solve it with this patch.
Calendar Views involves lots of calls of connectedCallback. In XBL, when we constructor anything, child elements append at the time of construction. Where in CE, connectedCallback runs when you append it to another element. Generally, we append child elements of CE during the connectedCallbacks. I find many similar problems which I needed to consider in this conversion so I guess, this can be a problem and it has created such a situation. We can look into it after converting this element as this will be the last CE in this view.

Comment 9

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9082594 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

::: calendar/base/content/calendar-event-column.js
@@ +56,4 @@
>              });
>  
>              this.addEventListener("click", (event) => {
> +                if (event.button == 0 || event.button == 2) {

there was an early return for this. I like that better, so please drop this change

::: calendar/base/content/calendar-month-view.js
@@ +345,5 @@
> +        set occurrence(val) {
> +            cal.ASSERT(!this.mOccurrence, "Code changes needed to set the occurrence twice", true);
> +            this.mOccurrence = val;
> +            if (cal.item.isEvent(val)) {
> +                if (!val.startDate.isDate) {

looks like you can combine these two ifs

::: calendar/base/content/calendar-multiday-base-view.js
@@ +872,4 @@
>              this.mSelectedItems = items || [];
>  
>              for (const item of this.mSelectedItems) {
> +                if (item) {

I think you need to figure out how one of the selectedItems end up being null, and fix that so it never happens

@@ +874,5 @@
>              for (const item of this.mSelectedItems) {
> +                if (item) {
> +                    for (const occ of this.getItemOccurrencesInView(item)) {
> +                        const cols = this.findColumnsForItem(occ);
> +                        if (cols.length > 0) {

we could change this to 
if (cols.length == 0) {
  continue;
}

::: calendar/base/content/calendar-multiday-view.js
@@ +377,5 @@
> +
> +        setAttribute(aAttr, aVal) {
> +            if (!this.hasChildNodes()) {
> +                this.appendItems();
> +            }

This looks wrong. setting attributes shouldn't cause the element to get additional children.

So this is overriding the standard element setAttribute method? 
It looks like this should all be set up in initializeAttributeInheritance instead and it would do it automatically?

@@ +408,5 @@
> +                    getSummarizedStyleValues(this, ["border-bottom-width", "border-top-width"]);
> +                this.setAttribute("minheight", minHeight);
> +                this.setAttribute("minwidth", "1");
> +                return minHeight;
> +            } else {

no else after return please

@@ +421,5 @@
> +        setEditableLabel() {
> +            let evl = this.eventNameLabel;
> +            let item = this.mOccurrence;
> +
> +            if (item.title && item.title != "") {

if (item.title) will do ;)

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

Comment 11

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9084502 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

::: calendar/base/content/calendar-multiday-view.js
@@ +376,5 @@
> +            if (aAttr == "orient") {
> +                if (this.getAttribute("orient") != aVal) {
> +                    needsrelayout = true;
> +                }
> +            }

we can just simplify this, see below

@@ +378,5 @@
> +                    needsrelayout = true;
> +                }
> +            }
> +
> +            // This should be done using lookupMethod(), see bug 286629.

you can remove this now I think

@@ +381,5 @@
> +
> +            // This should be done using lookupMethod(), see bug 286629.
> +            let ret = super.setAttribute(aAttr, aVal);
> +
> +            if (needsrelayout) {

if (aAttr == "orient" && this.getAttribute("orient") != aVal) { // needs relayout

@@ +388,5 @@
> +                let gb1 = this.querySelector(".calendar-event-box-grippy-top");
> +                gb1.parentorient = aVal;
> +                let gb2 = this.querySelector(".calendar-event-box-grippy-bottom");
> +                gb2.parentorient = aVal;
> +            }

But something quite fishy with this. 

E.g. the calendar-event-box-grippy-top has xbl:inherits="parentorient=orient"
So, this is all looping (child orient get sets -> that sets the parent orient, and then that causes the child orient to get sat)? Is any of this needed?

@@ +418,5 @@
> +                // Use <description> textContent so it can wrap.
> +                evl.textContent = item.title;
> +            } else {
> +                evl.textContent = cal.l10n.getCalString("eventUntitled");
> +            }

change these four lines to

evl.textContent = item.title || cal.l10n.getCalString("eventUntitled");

::: calendar/base/content/calendar-views.js
@@ +557,5 @@
> +            this.eventNameTextbox.onkeypress = (event) => {
> +                // Save on enter.
> +                if (event.key == "Enter") {
> +                    this.stopEditing(true);
> +                    // Abort on escape.

these comments are in an odd place. I'd put

if (event.key == "Enter") { // Save on enter.

} else if (event.key == "Escape") { //Abort on escape.

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

Comment 13

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9084971 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

Seems to work fine AFAIKT, but it's hard to test everything.

Comment 14

[Paul Morris [:pmorris]]

Comment on attachment 9084971 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

Looking good.  I have some comments, mostly small things.

::: calendar/base/content/calendar-multiday-view.js
@@ +221,5 @@
> +                let deltaX = Math.abs(event.screenX - this.mMouseX);
> +                let deltaY = Math.abs(event.screenY - this.mMouseY);
> +                // More than a 3 pixel move?
> +                if ((deltaX * deltaX + deltaY * deltaY) > 9) {
> +                    if (this.parentColumn) {

You can combine these two ifs:

const movedMoreThan3Pixels = (deltaX * deltaX + deltaY * deltaY) > 9;
if (movedMoreThan3Pixels && this.parentColumn) {

@@ +249,5 @@
> +
> +                    this.mEditing = false;
> +
> +                    this.parentColumn.startSweepingToModifyEvent(this, this.mOccurrence, "middle", this.mMouseX, this.mMouseY);
> +                    this.mInMouseDown = false;

Optional suggestion: instead of duplicating code in the body of the mouseout and mousemove handlers, put it into its own function and call it.  Something like `this.startItemDrag`.  Just a suggestion, I don't insist on it.

@@ +258,5 @@
> +                if (this.mEditing) {
> +                    return;
> +                }
> +
> +                this.mInMouseDown = false;

Simplify to:
if (!this.mEditing) {
  this.mInMouseDown = false;
}

@@ +342,5 @@
> +            this.setAttribute("mousethrough", "never");
> +            this.setAttribute("tooltip", "itemTooltip");
> +
> +            this.orient = this.getAttribute("orient");
> +            this.addEventNameTextboxListner();

listner -> listener

@@ +347,5 @@
> +            this.initializeAttributeInheritance();
> +        }
> +
> +        set parentColumn(val) {
> +            return (this.mParentColumn = val);

I find separating these things like this to be clearer:

this.mParentColumn = val;
return val;

@@ +373,5 @@
> +
> +        setAttribute(aAttr, aVal) {
> +            let ret = super.setAttribute(aAttr, aVal);
> +            return ret;
> +        }

Delete this setAttribute function since all it's doing now is deferring to super.setAttribute (which is what will happen if we delete it).

@@ +392,5 @@
> +            return minWidth;
> +        }
> +
> +        setEditableLabel() {
> +            let evl = this.eventNameLabel;

Please rename "evl" to "label" so it's less cryptic.

::: calendar/base/content/calendar-views.js
@@ +328,5 @@
> +     * the respective view of the calendar.
> +     *
> +     * @extends MozXULElement
> +     */
> +    class MozCalendarEditableItem extends MozXULElement {

I think we should put this CE in its own separate file ("calendar-editable-item.js"), rather than adding it to this file.  Better to keep this file just for the 4 main view CEs (which are part of a non-trivial class hierarchy that's good to keep separate from other things).

It's tempting to think about moving the other CEs (calendar-event-box and calendar-month-day-box-item) out into their own files, so it's one CE per file, with file names corresponding to the CE name.  But lets do that in a follow-up if we do.

@@ +360,5 @@
> +                }
> +
> +                if (this.mEditing) {
> +                    return;
> +                }

I'd combine these two ifs.

@@ +478,5 @@
> +                    </hbox>
> +                </vbox>
> +            `));
> +
> +            this.addEventListener("dragstart", (event) => {

It's odd that there are two 'dragstart' event listeners, can we combine them?

@@ +541,5 @@
> +        get eventNameTextbox() {
> +            return this.querySelector(".title-desc");
> +        }
> +
> +        addEventNameTextboxListner() {

listner -> listener

@@ +567,5 @@
> +            };
> +        }
> +
> +        setEditableLabel() {
> +            let evl = this.eventNameLabel;

evl -> label

@@ +578,5 @@
> +            let locationLabel = this.querySelector(".location-desc");
> +            let location = this.mOccurrence.getProperty("LOCATION");
> +            let showLocation = Services.prefs.getBoolPref("calendar.view.showLocation", false);
> +
> +            locationLabel.value = showLocation && location ? location : "";

We usually add parens in cases like this for easier reading:

(showLocation && location) ? location : "";

@@ +615,5 @@
> +            }
> +
> +            // Set up event box attributes for use in css selectors. Note if
> +            // something is added here, it should also be xbl:inherited correctly
> +            // in the <content> section of this binding, and all that inherit it.

Let's update this comment, since we're no longer in an xbl binding world.

Comment 15

[Khushil Mistry [:khushil324]]

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

Optional suggestion: instead of duplicating code in the body of the mouseout
and mousemove handlers, put it into its own function and call it. Something
like this.startItemDrag. Just a suggestion, I don't insist on it.

Nice suggestion.

I think we should put this CE in its own separate file
("calendar-editable-item.js"), rather than adding it to this file. Better
to keep this file just for the 4 main view CEs (which are part of a
non-trivial class hierarchy that's good to keep separate from other things).

It's tempting to think about moving the other CEs (calendar-event-box and
calendar-month-day-box-item) out into their own files, so it's one CE per
file, with file names corresponding to the CE name. But lets do that in a
follow-up if we do.

Agree.

It's odd that there are two 'dragstart' event listeners, can we combine them?

One is Bubbling phase and another is capturing phase. So I have kept it two event listeners.

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

Comment 17

[Paul Morris [:pmorris]]

Comment on attachment 9085769 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

Looking good!  A few more little nits.  I still need to give it a test drive, but lunch first.

::: calendar/base/content/calendar-editable-item.js
@@ +93,5 @@
> +                    onMouseOverItem(event);
> +                }
> +            });
> +
> +            this.addEventListener("dragstart", (event) => {

For good measure, can you add a comment about there being two dragstart listeners, one for bubbling and one for capturing?

@@ +164,5 @@
> +                    </hbox>
> +                </vbox>
> +          `));
> +
> +            this.addEventListener("dragstart", (event) => {

Again just for good measure, a comment about there being two dragstart listeners, one for bubbling and one for capturing.

@@ +256,5 @@
> +        setEditableLabel() {
> +            let label = this.eventNameLabel;
> +            let item = this.mOccurrence;
> +            label.value = (item.title ? item.title.replace(/\n/g, " ")
> +                : cal.l10n.getCalString("eventUntitled"));

Lets drop the outer parens here so it's just:

label.value = item.title ? item.title.replace(/\n/g, " ")
                : cal.l10n.getCalString("eventUntitled");

@@ +300,5 @@
> +                classificationBox.setAttribute("classification", item.privacy || "PUBLIC");
> +            }
> +
> +            // Set up event box attributes for use in css selectors. Note if
> +            // something is added here, it should also be xbl:inherited correctly

Drop the "xbl" here too, so it's just "also be inherited correctly".

::: calendar/base/content/calendar-multiday-view.js
@@ +222,5 @@
> +                let deltaY = Math.abs(event.screenY - this.mMouseY);
> +                // More than a 3 pixel move?
> +                const movedMoreThan3Pixels = (deltaX * deltaX + deltaY * deltaY) > 9;
> +                if (movedMoreThan3Pixels && this.parentColumn) {
> +                    if (this.parentColumn) {

An extra 'if' accidentally left in here.

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

Comment 19

[Paul Morris [:pmorris]]

Comment on attachment 9085959 [details] [diff] [review]
Bug-1554646_de-xbl-calendar-editable-item-bindings.patch

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

r+ Thanks for making the changes.  Seems to work just fine when I tested it.  Down to the last few bindings left!

Comment 20

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=790af8a62330c18a01f92d9d11243ebfd1f36f5c&selectedJob=262037393

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/40db4b5c31ff
[de-xbl] convert calendar-editable-item binding and derivatives to custom elements. r=pmorris

Comment 22

[Jorg K (CEST = GMT+2)]

Down to the last few bindings left!

With these three gone, Calendar looks XBL-free. Left are:

chat/content/imtooltip.xml
13 <binding id="tooltip" extends="chrome://global/content/bindings/popup.xml#panel">
mail/base/content/search.xml
34 <binding id="glodaSearch"

Bug 1506529 and bug 1542720. I believe there's also still work in bug 1565075 (waiting for bug 1534455) and hopefully fixing bug 1569492.