1525011 - [de-xbl] convert calendar-event-gripbar to custom element

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug1525011_De-XBL_calendar-event-gripbar.patch

Comment 2

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

Comment on attachment 9041185 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

Don't mind also getting feedback from Magnus/Arshad who have done more de-xbl than I have. Here are a few code comments:

::: calendar/base/content/calendar-event-gripbar.js
@@ +38,5 @@
> +
> +    connectedCallback() {
> +        this._box = document.createElement("box");
> +        this._box.setAttribute("flex", "1");
> +        this._box.setAttribute("pack", "center");

I wonder if this could be done without the extra box and image but instead a background image on the element itself.

@@ +49,5 @@
> +        this._updateAttributes();
> +
> +        /**
> +         * private
> +         */

I'm not sure what this comment means. If parentorient is supposed to be private, maybe call it _parentorient? I think you can remove the comment.

@@ +63,5 @@
> +    }
> +
> +    _updateAttributes() {
> +        this._box.setAttribute("align", this.getAttribute("whichside"));
> +        this._image.setAttribute("class", this.getAttribute("class"));

Why does the class attribute need to be propagated? Can we do this via css instead? e.g. calendar-event-gripbar > box > image ?

Comment 3

[Arshad Khan [:arshad]]

Comment on attachment 9041185 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

::: calendar/base/content/calendar-event-gripbar.js
@@ +36,5 @@
> +        });
> +    }
> +
> +    connectedCallback() {
> +        this._box = document.createElement("box");

If you can make it work without wrapping it inside box then you can get rid of observing one attribute i.e., the whichside attribute. Also if you can use css instead of observing class attr, you can completely remove whole attribute observign setup. You can easily remove the class attr by changing line like https://searchfox.org/comm-central/source/calendar/base/themes/common/calendar-views.css#782 to `calendar-event-box[orient="horizontal"].calendar-event-box-grippy-bottom image`

@@ +50,5 @@
> +
> +        /**
> +         * private
> +         */
> +

you can just remove the comment.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9041185 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

Please flag me again once you did the adjustments

::: calendar/base/content/calendar-event-gripbar.js
@@ +5,5 @@
> +"use strict";
> +
> +/* global MozXULElement */
> +
> +class MozCalendarEventGripbar extends MozXULElement {

Please add documentation as to what this widget does.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug1525011_De-XBL_calendar-event-gripbar.patch

Comment 6

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

Comment on attachment 9041781 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

::: calendar/base/content/calendar-event-gripbar.js
@@ +5,5 @@
> +"use strict";
> +
> +/* A simple gripbar that is displayed at the start and end of an
> + * event box.  Needs to handle being dragged and resizing the
> + * event, thus changing its start/end time. */

The best way to document these is to use jsdoc for the class, e.g.

```
/**
 * A simple gripbar that...
 *
 * @extends MozXULElement
 */
class MozCalendarEventGripbar extends MozXULElement {

}
```

Note the double * at the start of the comment. See http://usejsdoc.org/howto-es2015-classes.html (and the rest of that site for more documentation tips)

@@ +13,5 @@
> +class MozCalendarEventGripbar extends MozXULElement {
> +    constructor() {
> +        super();
> +
> +        this.addEventListener("mousedown", (event) => {

Do these event listeners have to be removed on destruction?

@@ +23,5 @@
> +            // but *don't* call stopPropagation(). as soon as the
> +            // enclosing event box will receive the event it will
> +            // make use of this information in order to invoke the
> +            // appropriate action.
> +            event.whichside = this.getAttribute("whichside");

I'm not totally sure, but can you look to see if this is happening early enough? My understanding is that first the capture phase runs, then the bubbling phase. The order of event listeners isn't guaranteed, therefore we might have to run this in the capturing phase to make sure it is available in the bubbling phase.

@@ +26,5 @@
> +            // appropriate action.
> +            event.whichside = this.getAttribute("whichside");
> +        });
> +
> +        this.addEventListener("click", (event) => {

This event listener could use a short comment on why stopping propagation is necessary. I know it wasn't there before, but maybe you can find out why.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #6)

•    this.addEventListener("mousedown", (event) => {
  

Do these event listeners have to be removed on destruction?

I don't think so. At least the toolkit widgets don't typically do so (only for certain events, I assume for other reasons).

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9041781 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

Will look at the patch again after you update it for Philipp's comments

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug1525011_De-XBL_calendar-event-gripbar

Comment 10

[Khushil Mistry [:khushil324]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #6)

I'm not totally sure, but can you look to see if this is happening early
enough? My understanding is that first the capture phase runs, then the
bubbling phase. The order of event listeners isn't guaranteed, therefore we
might have to run this in the capturing phase to make sure it is available
in the bubbling phase.

I found that event.whichside is getting set in enough time so need to use the capturing phase to set the event.whichside.

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug1525011_De-XBL_calendar-event-gripbar.patch

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9043035 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

I'm not sure I'm testing the right widget here. Is it the thing that puts up the up-down arrows when you move to the top of an event in multi-day view?

Trying to drag an event to make it larger, in single day view:
JavaScript error: chrome://calendar/content/calendar-dnd-listener.js, line 604: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIDragService.invokeDragSession]

Trying to drag an event in the week view:
JavaScript error: chrome://calendar/content/calendar-multiday-view.xml, line 1372: TypeError: sbo.scrollBy is not a function
JavaScript error: chrome://calendar/content/calendar-multiday-view.xml, line 1350: TypeError: scrollbox is null

... but it does move.

::: calendar/base/content/calendar-event-gripbar.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */
> +
> +"use strict";

add line after this

@@ +8,5 @@
> + * event box. Needs to handle being dragged and resizing the
> + * event, thus changing its start/end time.
> + * @extends MozXULElement
> + */
> +

no emtpy line before class here. for the documenation text, do use the full 80ch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9043035 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

But those may be unrelated. I think this is working

Comment 14

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

Comment on attachment 9043035 [details] [diff] [review]
Bug1525011_De-XBL_calendar-event-gripbar.patch

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

r=me with Magnus' comments taken into account as well.

::: calendar/base/content/calendar-event-gripbar.js
@@ +6,5 @@
> +/**
> + * A simple gripbar that is displayed at the start and end of an
> + * event box. Needs to handle being dragged and resizing the
> + * event, thus changing its start/end time.
> + * @extends MozXULElement

Leave a line before @extends

@@ +45,5 @@
> +
> +        this.parentorient = this.getAttribute("parentorient");
> +    }
> +
> +    set parentorient(val) {

This needs docs

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug1525011_De_XBL_calendar-event-gripbar.patch

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1525011_De_XBL_calendar-event-gripbar.patch

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

Push patch of Bug 1518191 before this.

Comment 17

[Jorg K (CEST = GMT+2)]

Yes, but bug 1518191 is has been removed from the landing queue.

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: Bug-1525011_De-XBL_calendar-event-gripbar.patch

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2296b934a2a7
[de-xbl] convert calendar-event-gripbar binding to custom element. r=philipp