1512941 - [de-xbl] Migrate timepicker-grids to custom element.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Comment 1

[Arshad Khan [:arshad]]

Attachment: timepicker-grids_WIP.patch

still trying to figure out the errors that I get..

Comment 2

[Arshad Khan [:arshad]]

Attachment: timepicker-grids.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

This patch has code for timepicker-minute, timepicker-hour and timepicker-grids, so just test this out for checking those bindings as well.

Comment 4

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

patch for timepicker-minute, timepicker-hour and timepicker-grids.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9031888 [details] [diff] [review]
timepicker-grids-combined.patch

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

> applying timepicker-grids-combined.patch
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh timepicker-grids-combined.patch

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +79,5 @@
> +            this._updateAttributes();
> +        }
> +
> +        attributeChangedCallback() {
> +            this._updateAttributes();

add if (!this.isConnectedAndReady) { return; }
per https://searchfox.org/comm-central/rev/85923675941e2dad376a1ecfdb26e3cf824ea7fc/mozilla/toolkit/content/customElements.js#95

@@ +85,5 @@
> +
> +        _updateAttributes() {
> +            if (!this.label) {
> +                return;
> +            }

shouldn't need this

@@ +101,5 @@
> +            }
> +        }
> +    }
> +
> +    customElements.define("timepicker-minute", MozTimepickerMinute);

same thing as above for these

::: mail/base/content/customElements.js
@@ +10,5 @@
>    "chrome://messenger/content/mailWidgets.js",
>    "chrome://messenger/content/generalBindings.js",
>    "chrome://messenger/content/statuspanel.js",
>    "chrome://messenger/content/foldersummary.js",
> +  // "chrome://calendar/content/datetimepickers/datetimepickers.js",

remove

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9031888 [details] [diff] [review]
timepicker-grids-combined.patch

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

for MozTimepickerGrids - why is the hasChildrenAppended needed?

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +95,5 @@
> +            }
> +
> +            if (this.hasAttribute("selected")) {
> +                this.label.setAttribute("selected", this.getAttribute("selected"));
> +            } else {

https://searchfox.org/comm-central/rev/85923675941e2dad376a1ecfdb26e3cf824ea7fc/mozilla/toolkit/content/customElements.js#95

::: calendar/resources/content/datetimepickers/datetimepickers.xml
@@ +686,5 @@
>            event.preventDefault();
>        ]]></handler>
>      </handlers>
>    </binding>
>  

Don't understand what this was meant to work aound, but we should find a proper fix.

Comment 7

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9032788 [details] [diff] [review]
timepicker-grids-combined.patch

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

Still something wrong with the patch. Can't apply it

hg qpush -v
applying timepicker-grids-combined.patch
patching file calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
patching file calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
patching file calendar/base/content/dialogs/calendar-event-dialog-reminder.xul
patching file calendar/base/content/dialogs/calendar-event-dialog-timezone.xul
patching file calendar/base/content/dialogs/calendar-print-dialog.xul
patching file calendar/base/content/dialogs/calendar-summary-dialog.xul
patching file calendar/lightning/content/lightning-item-iframe.xul
patching file calendar/lightning/content/messenger-overlay-sidebar.xul
patching file calendar/lightning/jar.mn
patching file calendar/resources/content/datetimepickers/datetimepickers.css
patching file calendar/resources/content/datetimepickers/datetimepickers.js
patching file calendar/resources/content/datetimepickers/datetimepickers.xml
patching file mail/base/content/customElements.js
patching file calendar/resources/content/datetimepickers/datetimepickers.css
patching file calendar/resources/content/datetimepickers/datetimepickers.js
adding calendar/resources/content/datetimepickers/datetimepickers.js
cannot patch calendar/resources/content/datetimepickers/datetimepickers.js: file is not tracked
committing files:
calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
calendar/base/content/dialogs/calendar-event-dialog-reminder.xul
calendar/base/content/dialogs/calendar-event-dialog-timezone.xul
calendar/base/content/dialogs/calendar-print-dialog.xul
calendar/base/content/dialogs/calendar-summary-dialog.xul
calendar/lightning/content/lightning-item-iframe.xul
calendar/lightning/content/messenger-overlay-sidebar.xul
calendar/lightning/jar.mn
calendar/resources/content/datetimepickers/datetimepickers.css
calendar/resources/content/datetimepickers/datetimepickers.js
calendar/resources/content/datetimepickers/datetimepickers.xml
mail/base/content/customElements.js
committing manifest
committing changelog
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh timepicker-grids-combined.patch

Comment 9

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9032791 [details] [diff] [review]
timepicker-grids-combined.patch

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

Still get 
JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.js, line 62: ReferenceError: clickMinute is not defined

... just by trying to add an event, and choose time (minutes)

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +64,5 @@
> +            spacer.setAttribute("flex", "1");
> +
> +            const minutebox = document.createElement("vbox");
> +            minutebox.addEventListener("click", () => {
> +                this.clickMinute(this, this.getAttribute("value"));

perhaps from here, looks like minutebox is a just a vbox, so woudln't hva a clickMinute method

Comment 11

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #10)
> Comment on attachment 9032791 [details] [diff] [review]
> timepicker-grids-combined.patch
> 
> Review of attachment 9032791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still get 
> JavaScript error:
> chrome://calendar/content/datetimepickers/datetimepickers.js, line 62:
> ReferenceError: clickMinute is not defined
> 
> ... just by trying to add an event, and choose time (minutes)
> 
> ::: calendar/resources/content/datetimepickers/datetimepickers.js
> @@ +64,5 @@
> > +            spacer.setAttribute("flex", "1");
> > +
> > +            const minutebox = document.createElement("vbox");
> > +            minutebox.addEventListener("click", () => {
> > +                this.clickMinute(this, this.getAttribute("value"));
> 
> perhaps from here, looks like minutebox is a just a vbox, so woudln't hva a
> clickMinute method
Not sure what you meant.

I can't reproduce it.

Comment 12

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 13

[Arshad Khan [:arshad]]

Comment on attachment 9033389 [details] [diff] [review]
timepicker-grids-combined.patch

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

updated the patch, but I dont think it has any changes. I can't reproduce the errors that you were getting. :/

Comment 14

[Arshad Khan [:arshad]]

can you try one more time?

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9033389 [details] [diff] [review]
timepicker-grids-combined.patch

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

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +85,5 @@
> +            this._updateAttributes();
> +        }
> +
> +        attributeChangedCallback() {
> +            this._updateAttributes();

what about the this.isConnectedAndReady() check here

@@ +91,5 @@
> +
> +        _updateAttributes() {
> +            if (!this.label) {
> +                return;
> +            }

instead of this?

Same thing for the other elements

Comment 16

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #15)
> Comment on attachment 9033389 [details] [diff] [review]
> timepicker-grids-combined.patch
> 
> Review of attachment 9033389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: calendar/resources/content/datetimepickers/datetimepickers.js
> @@ +85,5 @@
> > +            this._updateAttributes();
> > +        }
> > +
> > +        attributeChangedCallback() {
> > +            this._updateAttributes();
> 
> what about the this.isConnectedAndReady() check here
> 
> @@ +91,5 @@
> > +
> > +        _updateAttributes() {
> > +            if (!this.label) {
> > +                return;
> > +            }
> 
> instead of this?
> 
> Same thing for the other elements

i felt there is not point in checking anything other than label element, if there is no label then we can't set anny attr on it. No point in checking isConnectedReady and using delayconnectedcaloback here..

Comment 17

[Magnus Melin [:mkmelin]]

But you *are* using delayConnectedCallback already, and per the documentation then you should use isConnectedReady in combination

Comment 18

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #17)
> But you *are* using delayConnectedCallback already, and per the
> documentation then you should use isConnectedReady in combination

Yes, I should remove that. Sorry, my bad.

Comment 19

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 20

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 21

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9033400 [details] [diff] [review]
timepicker-grids-combined.patch

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

Seems to work fine. f+ with the below addressed

::: calendar/resources/content/datetimepickers/datetimepickers.xml
@@ +566,5 @@
> +                if(timepickerGrid.changeTo12HoursFormat) {
> +                    timepickerGrid.changeTo12HoursFormat(amLabel, pmLabel);
> +                    clearInterval(tgInterval);
> +                }
> +              }, 100);

Instead of this hack, I think you should do

window.customElements.upgrade(timepickerGrid);
timepickerGrid.changeTo12HoursFormat(amLabel, pmLabel);

Comment 23

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 24

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9033534 [details] [diff] [review]
timepicker-grids-combined.patch

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

Looks ok to me. There are a lot more comments I could do, but they were also issues before. We should rewrite some of these bindings someday (tm).

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +421,5 @@
> +            this.kMINUTE_VIEW_ONE = 1;
> +        }
> +
> +        /**
> +         * Set up the picker, called when the popup pops

I'd appreciate if we could fill in the docs for this and other functions, e.g. full description, use of @param and @returns.

Comment 25

[[:MakeMyDay]]

Please don't land any changes involving datetimepickers.xml until we have finally landed bug sorted out bug 1503731.

Comment 26

[Philipp Kewisch [:Fallen][☁️📆]]

Oh another thing. Instead of the scoped block, can we use a <script type="module"> instead?

Comment 27

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combinedWIP.patch

Comment 28

[Arshad Khan [:arshad]]

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

Oh another thing. Instead of the scoped block, can we use a <script
type="module"> instead?
modules dont load synchronously and can't be loaded dynamically(atleast at the time when script[type="module"] landed)

Comment 29

[Philipp Kewisch [:Fallen][☁️📆]]

Do the modules need to be loaded synchronously? It seems to me this is something we should avoid requiring. Dynamic loading is indeed not possible, but iiuc we wouldn't need to?

Comment 30

[Magnus Melin [:mkmelin]]

It's pretty unclear to me how type=module would/could/should work for calendar, given it's all used through the overlaying hack :/

Comment 31

[Arshad Khan [:arshad]]

Philipp, could you please help me out with proper comments for the methods? I am not good at writing comments plus I dont have much knowledge how they work?

Comment 32

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Need to just rebase this patch after my build finishes. Right now I am on an old commit and for me the datetimepicker popup is not openin when I click on arrow buttons.

Comment 33

[Arshad Khan [:arshad]]

Comment on attachment 9045962 [details] [diff] [review]
timepicker-grids-combined.patch

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

Hey could you please take a look at the comments and just lemme know better ones if any?

Comment 34

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9045962 [details] [diff] [review]
timepicker-grids-combined.patch

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

Hey Arshad, some feedback here:

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +8,5 @@
> +// leaking to window scope.
> +{
> +    /* global MozXULElement */
> +
> +    class MozTimepickerMinute extends MozXULElement {

Would it make sense to not make minute/hour separate bindings but just use normal elements and then do any necessary processing in the timepicker element? I don't think we need to translate this 1:1 here from XBL.

@@ +421,5 @@
> +            this.kMINUTE_VIEW_ONE = 1;
> +        }
> +
> +        /**
> +         * Set up the picker, called when the popup pops.

We usually leave an empty line after the description and before the arguments.

@@ +469,5 @@
> +            }
> +        }
> +
> +        /**
> +         * Called when the more tab is clicked, and possibly at startup.

Focus on what the method does, not when or how it is called. When it is called may potentially change and we may miss updating all comments where it is referenced.

This is the same for the method name. Maybe it would make more sense to call it selectMinuteBox and then describe it accordingly.

@@ +483,5 @@
> +            this.selectMinuteItem(oneMinuteItem);
> +        }
> +
> +        /**
> +         * Called when the less tab is clicked, and possibly at startup.

Same here, you should focus on what this method does instead.

@@ +650,5 @@
> +        }
> +
> +        /**
> +         * Helper function to move hours.
> +         * @param {number} number - value by which hours should be moved.

The other calendar files don't use a - to split number, but instead align to a tab stop so that all param descriptions start in the same column.

Objects such as String, Number should be with an uppercase first letter.

@@ +679,5 @@
> +        }
> +
> +        /**
> +         * Helper function to calulate the nearest even five minutes.
> +         * @param {calDateTime object} time - time near to which new time will be calculated.

Check the jsdoc documentation on how to specify objects. This should probably just be {calDateTime}.

Just a note in case you come across this in the docs: we don't create extra comments to define how calDateTime is structured. You are not doing this, but it is mentioned in the jsdoc docs.

Comment 35

[Arshad Khan [:arshad]]

Attachment: timepicker-grids-combined.patch

Comment 36

[Arshad Khan [:arshad]]

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

Comment 37

[Arshad Khan [:arshad]]

Attachment: timepicker-grids.patch

Comment 38

[Arshad Khan [:arshad]]

This is the same for the method name. Maybe it would make more sense to call
it selectMinuteBox and then describe it accordingly.

Not sure what you meant by this ^?

Comment 39

[Arshad Khan [:arshad]]

Attachment: timepicker-grids.patch

Comment 40

[Arshad Khan [:arshad]]

Attachment: timepicker-grids.patch

Comment 41

[Arshad Khan [:arshad]]

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

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 9048060 [details] [diff] [review]
timepicker-grids.patch

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

Seems to be working, but please fix some nits as per below

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +206,5 @@
>                  this.label.removeAttribute("selected");
>              }
>          }
>      }
>  

/**
     * The MozTimepickerGrids widget displays he grid of times to select, e.g.
     * for an event. Typically it represents the popup content that let's the
     * user select a time, in a <timepicker> widget.
     * @extends MozXULElement
     */

@@ +457,5 @@
> +        onPopupShowing() {
> +            // select the hour item
> +            let hours24 = this.mSelectedTime.getHours();
> +            let hourItemId = `.time-picker-hour-box-class[value="${hours24}"]`;
> +            let hourItem = this.querySelector(hourItemId);

would inline hourItemId and not declare it for a one time usage

@@ +480,5 @@
> +            this.switchMinuteView(this.kMINUTE_VIEW_ONE);
> +
> +            // select minute box corresponding to the time
> +            let minutes = this.mSelectedTime.getMinutes();
> +            let oneMinuteItemId = `.time-picker-one-minute-class[value="${minutes}"]`;

here too

@@ +498,5 @@
> +            // BUT leave the selected time at what may NOT be an even five minutes
> +            // So that If they click more again the proper non-even-five minute
> +            // box will be selected
> +            let minutesByFive = this.calcNearestFiveMinutes(this.mSelectedTime);
> +            let fiveMinuteItemId = `.time-picker-five-minute-class[value="${minutesByFive}"]`;

and here

@@ +570,5 @@
> +
> +        /**
> +         * Selects an hour item.
> +         *
> +         * @param {object} hourItem      Hour item node to be selected

these should be @param {Node}

@@ +587,5 @@
> +        /**
> +         * Selects a minute item.
> +         *
> +         * @param {object} minuteItem        Minute item node to be selected
> +         */

{Node}

Comment 43

[Arshad Khan [:arshad]]

need to fix one test and this patch will be ready to land

Comment 44

[Arshad Khan [:arshad]]

(In reply to Arshad Khan [:arshad] from comment #43)

need to fix one test and this patch will be ready to land

testalarmDialog.js test is only failign for linux. I have pulled latest changesets for my linux machine and will test out the patch as soon as my build finishes.

Comment 45

[Arshad Khan [:arshad]]

There ll be sometimes error like mPicker is null which I am not handling here. See https://bugzilla.mozilla.org/show_bug.cgi?id=1524456#c28 and https://bugzilla.mozilla.org/show_bug.cgi?id=1524456#c29 .

Comment 46

[Arshad Khan [:arshad]]

Attachment: timepicker-grids.patch

Comment 47

[Arshad Khan [:arshad]]

Just ran the testAlarmDialog.js test on ubuntu and the failed try tesst passed. Not sure wther the test failure was intermittent or not, so I have pushed another try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7829aa18efdd4071a4eb1864c4407518e29353bd&selectedJob=232640559 . Lets see what happens now. Certainly there will be some testFailures due to recent issues.

Comment 48

[Arshad Khan [:arshad]]

See the try - https://bugzilla.mozilla.org/show_bug.cgi?id=1512941#c41 . The one test that's failing didn't fail again that's why I am adding checkin-needed keyword.

Comment 49

[Jorg K (CEST = GMT+2)]

Looking at the history here, there was never an r+, only f+ from Magnus and Philipp. Am I too strict?

Comment 50

[Magnus Melin [:mkmelin]]

Yeah I don't see a review yet.

Comment 51

[Jorg K (CEST = GMT+2)]

Good try here though ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=04ae5deefbeacae6eebdcc0db47ca7101d05d3e4

Comment 52

[Arshad Khan [:arshad]]

(In reply to Jorg K (GMT+1) from comment #49)

Looking at the history here, there was never an r+, only f+ from Magnus and Philipp. Am I too strict?

https://bugzilla.mozilla.org/show_bug.cgi?id=1512941#c24

Comment 53

[Jorg K (CEST = GMT+2)]

Very confusing, so an earlier version had r+ and a later f+? Maybe Philipp or even Magnus can just take another look.

Comment 54

[Magnus Melin [:mkmelin]]

Comment on attachment 9049481 [details] [diff] [review]
timepicker-grids.patch

Oh yes it was from that review. For future reference, please don't move patches and reviews between bugs. Way too confusing

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4a9f709716ad
Migrate timepicker-grids binding to custom element. r=philipp