1523424 - [de-xbl] Migrate selection-bar to custom element.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 4

[Arshad Khan [:arshad]]

Comment on attachment 9040732 [details] [diff] [review]
selection-bar.patch

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

Hey, I am not sure why the time doens't change and the cyan colored box doesn't change position on mousemove.. could you please take a look/?

Comment 5

[Arshad Khan [:arshad]]

The reason why I didn't try extending richlistbox here, is that MozElements.RichListBox is not available in scope and there are other parts of code which are trying to call selection-bar methods so delaying the decalaration or selection-bar custom element untill the Richlistbox isn't loaded in scope, will throw error from those other parts.

Comment 6

[Magnus Melin [:mkmelin]]

You could probably make RichListBox available by creating one. Same thing as https://searchfox.org/comm-central/source/chat/content/conversation-browser.js#13
Dunno if it's wanted though.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9040732 [details] [diff] [review]
selection-bar.patch

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

To preserve some blame:
hg cp --after calendar/base/content/dialogs/calendar-event-dialog-attendees.xml calendar/base/content/dialogs/calendar-event-dialog-custom-elements.js

::: calendar/base/content/dialogs/calendar-event-dialog-custom-elements.js
@@ +2,5 @@
> + * 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/. */
> +
> +ChromeUtils.import("resource://gre/modules/Preferences.jsm");
> +

these now need to be
var {Preferences} = ChromeUtils.import("resource://gre/modules/Preferences.jsm");

@@ +397,5 @@
> +        return newTime;
> +    }
> +}
> +
> +customElements.define("selection-bar", MozSelectionBar);

calendar-event-timeselection-bar perhaps?

Comment 8

[Arshad Khan [:arshad]]

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

You could probably make RichListBox available by creating one. Same thing as https://searchfox.org/comm-central/source/chat/content/conversation-browser.js#13
Dunno if it's wanted though.

Tried that. But overall result is not different. :(

Comment 9

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 10

[Arshad Khan [:arshad]]

Comment on attachment 9040967 [details] [diff] [review]
selection-bar.patch

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

Richard, could you please check why after the first time I am not able to move the selection-bar? I think it might be focus issue but I also feel very less possibility of that.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 9040967 [details] [diff] [review]
selection-bar.patch

Sorry, I don't know why. For me it's never movable.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9040967 [details] [diff] [review]
selection-bar.patch

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

I just get this:

JavaScript error: chrome://calendar/content/calendar-event-dialog-attendees.js, line 1237: TypeError: selectionbar.boxObject is undefined

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +26,5 @@
>  var gForce24Hours = false;
>  var gZoomFactor = 100;
>  
> +/*
> + * the 'selection-bar' custom element implements the vertical bar that provides

The

There are also other comments in the patch. If it's a full sentence, make sure it's Capitalized and ends with a period.

@@ +157,5 @@
> +        this.mZoomFactor = 100;
> +
> +        /**
> +         * constant that defines at which ratio an event is clipped, when moved or resized
> +         */

Just // comments inside please

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +141,5 @@
> +          <scrollbox anonid="scrollbox" width="0" orient="horizontal" flex="1">
> +            <box class="selection-bar" anonid="selection-bar">
> +              <box class="selection-bar-left" anonid="leftbox"/>
> +              <spacer class="selection-bar-spacer" flex="1"/>
> +              <box class="selection-bar-right" anonid="rightbox"/>

can we remove the anonid's?

Comment 13

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 14

[Arshad Khan [:arshad]]

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

Comment 15

[Arshad Khan [:arshad]]

Try failures don't look like related to this bug.

Comment 16

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 17

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

Comment on attachment 9058056 [details] [diff] [review]
selection-bar.patch

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

Similar documentation issues as flagged in other previous reviews, please fix.

Comment 18

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

I have tried to remove all the issues mentioned in other patches, I think the comments in the patch are quite explanatory.

Comment 19

[Arshad Khan [:arshad]]

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

Comment 20

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

Comment on attachment 9058916 [details] [diff] [review]
selection-bar.patch

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

I'm not going to look at these patches if you aren't fixing the issues we've talked about. There are still issues in this patch.

Comment 21

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 22

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 23

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 24

[Arshad Khan [:arshad]]

Comment on attachment 9059164 [details] [diff] [review]
selection-bar.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +2001,5 @@
> +     *
> +     * @param {Number} val      Decimal number between 0 and 1
> +     * @returns {Number}        Decimal number between 0 and 1
> +     */
> +    set ratio(val) {

I tried checking the use this property but I dont see any differnece in ui or anything else. Please suggest something else if you dont like the comment. Sorry.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9059164 [details] [diff] [review]
selection-bar.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +1932,5 @@
> +            window.setCursor("auto");
> +        });
> +    }
> +
> +    connectedCallback() {

this needs protection from running more than once. also add the delayConnectedCallback

@@ +1996,5 @@
> +        return val;
> +    }
> +
> +    /**
> +     * Sets ratio and updates selection bar properties.

Sets the ratio. The ratio is the factor which says by how big part of the total width is the scroll width is offset.

@@ +1998,5 @@
> +
> +    /**
> +     * Sets ratio and updates selection bar properties.
> +     *
> +     * @param {Number} val      Decimal number between 0 and 1

@param ..... The ratio: a number between 0 and 1.

@@ +1999,5 @@
> +    /**
> +     * Sets ratio and updates selection bar properties.
> +     *
> +     * @param {Number} val      Decimal number between 0 and 1
> +     * @returns {Number}        Decimal number between 0 and 1

@returns .... The ratio.

@@ +2025,5 @@
> +
> +    /**
> +     * Gets start date of the selection bar.
> +     *
> +     * @returns {calIDateTime}       startDate value

for these instead of
 @returns {calIDateTime} startDate value

use something like

 @returns {calIDateTime} - the start date

Comment 26

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 27

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 28

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 29

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

Comment on attachment 9059636 [details] [diff] [review]
selection-bar.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +1975,5 @@
> +        return val;
> +    }
> +
> +    /**
> +     * Gets force24Hours property of selection bar.

Getter description is not in line with setter.

Comment 30

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

If this patch is pushed first then scroll-container and freebusy-day patch needs rebase.

Comment 31

[Arshad Khan [:arshad]]

Attachment: selection-bar.patch

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6212f5166af1
Migrate selection-bar binding to custom element. r=philipp