1512942 - [de-xbl] Convert timepicker-minute to custom element.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9030416 [details] [diff] [review]
timepicker-minute.patch

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

::: common/bindings/datetimepicker.js
@@ +83,5 @@
> +            return;
> +        }
> +
> +        if (this.hasAttribute("label")) {
> +            this.label.setAttribute("value", this.getAttribute("label"));

should handle label removal too

::: 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://messenger/content/datetimepicker.js",

-> lightning

::: mail/base/jar.mn
@@ +38,5 @@
>  *   content/messenger/viewSource.xul                (../../common/src/viewSource.xul)
>  *   content/messenger/datetimepicker.xml            (../../common/bindings/datetimepicker.xml)
>      content/messenger/generalBindings.xml           (../../common/bindings/generalBindings.xml)
>      content/messenger/generalBindings.js            (../../common/bindings/generalBindings.js)
> +    content/messenger/datetimepicker.js             (../../common/bindings/datetimepicker.js)

-> lightning

Comment 3

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #2)
> Comment on attachment 9030416 [details] [diff] [review]
> timepicker-minute.patch
> 
> Review of attachment 9030416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: common/bindings/datetimepicker.js
> @@ +83,5 @@
> > +            return;
> > +        }
> > +
> > +        if (this.hasAttribute("label")) {
> > +            this.label.setAttribute("value", this.getAttribute("label"));
> 
> should handle label removal too
> 

I know we have done this earlier, but recently I was workin on a bug where I landed it some problems when I removed the attr on inheriting elment when parent element's attr was removed.

Comment 4

[Magnus Melin [:mkmelin]]

That's something to watch out for. Dunno if it happens in reality for this case, but if someone does remove the label you'd have unexpected behaviour unless you handle it.

Comment 5

[Arshad Khan [:arshad]]

> 
> ::: mail/base/jar.mn
> @@ +38,5 @@
> >  *   content/messenger/viewSource.xul                (../../common/src/viewSource.xul)
> >  *   content/messenger/datetimepicker.xml            (../../common/bindings/datetimepicker.xml)
> >      content/messenger/generalBindings.xml           (../../common/bindings/generalBindings.xml)
> >      content/messenger/generalBindings.js            (../../common/bindings/generalBindings.js)
> > +    content/messenger/datetimepicker.js             (../../common/bindings/datetimepicker.js)
> 
> 

why is this in common/bindings, if they were lightning specific only.

Comment 6

[Magnus Melin [:mkmelin]]

It's not, that's from one of your other unlanded patches.

Comment 7

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 9

[Arshad Khan [:arshad]]

Comment on attachment 9030699 [details] [diff] [review]
timepicker-minute.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +29,5 @@
>          style="padding-top: 8px; padding-bottom: 10px; padding-inline-start: 8px; padding-inline-end: 10px;"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>  
>    <!-- Javascript includes -->
> +  <script type="application/javascript"

Adding script in files where datetimepicker is used because datetimepicker has timepicker as anon content, which has timpicker-grids inside it.. https://searchfox.org/comm-central/search?q=datetimepicker&case=false&regexp=false&path=calendar

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9030699 [details] [diff] [review]
timepicker-minute.patch

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

Doesn't seem to work. Clicking a minute I get
JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.js, line 62: ReferenceError: clickMinute is not defined

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +5,5 @@
> +"use strict";
> +
> +// This is loaded into all XUL windows. Wrap in a block to prevent
> +// leaking to window scope.
> +{

I think you should skip the indent for this though
It's also questionable if we really care. It's not all XUL windows, it's just the ones we use it in.

Comment 11

[Arshad Khan [:arshad]]

See Bug 1512941 for the timepicker-minute patch changes.

Comment 12

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 13

[Arshad Khan [:arshad]]

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

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patch

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

(no reviewed yet!)

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@
> +            this.appendChild(spacer.cloneNode());
> +            this.appendChild(minutebox);
> +            this.appendChild(spacer);
> +
> +            this.pixelScrollDelta = 0;

why does all this dom building happen in connecedCallback() and not the constructor?

Comment 15

[Arshad Khan [:arshad]]

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

Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patch

Review of attachment 9047947 [details] [diff] [review]:

(no reviewed yet!)

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@

•        this.appendChild(spacer.cloneNode());
  

•        this.appendChild(minutebox);
  

•        this.appendChild(spacer);
  

•        this.pixelScrollDelta = 0;
  

why does all this dom building happen in connecedCallback() and not the
constructor?

if it is not shadow dom then dom should be build in connectedCallback. It will throw NSERrror if you append child in constructor and try to extend it by some other ce. We experienced this in one of the very first ce conversion.

Comment 16

[Arshad Khan [:arshad]]

Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patch

Review of attachment 9047947 [details] [diff] [review]:

(no reviewed yet!)

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@

•        this.appendChild(spacer.cloneNode());
  

•        this.appendChild(minutebox);
  

•        this.appendChild(spacer);
  

•        this.pixelScrollDelta = 0;
  

Fallen reviewed it in https://bugzilla.mozilla.org/show_bug.cgi?id=1512941#c24(In reply to Magnus Melin [:mkmelin] from comment #14)

Comment 17

[Magnus Melin [:mkmelin]]

You really need to put that kind of information into this bug when marking a patch r+

I'm not convinced you should build in connectedCallback. That can run more than once.

Comment 18

[Arshad Khan [:arshad]]

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

You really need to put that kind of information into this bug when marking a patch r+

Yes you are right, I should have put it here also. I forgot about it after conveying it to you on IRC.

I'm not convinced you should build in connectedCallback. That can run more than once.
when you clone node, or move nodes..

Comment 19

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

is it ok now?

Comment 20

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9047989 [details] [diff] [review]
timepicker-minute.patch

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

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +602,5 @@
>                  if ((preString && this.amRegExp.test(preString)) ||
>                      (postString && this.amRegExp.test(postString))) {
>                      ampmCode = "AM";
>                  } else if ((preString && this.pmRegExp.test(preString)) ||
> +                    (postString && this.pmRegExp.test(postString))) {

strange intention change here

@@ +761,5 @@
>                      this.ampmIndex = POST_INDEX;
>                  }
>              }
>              if (this.ampmIndex) {
> +                let makeFormatRegExp = function (string) {

no space after function (leave this line as it was)

Comment 22

[Arshad Khan [:arshad]]

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

Comment 23

[Arshad Khan [:arshad]]

Attachment: timepicker-minute.patch

Comment 24

[Arshad Khan [:arshad]]

Try-
https://bugzilla.mozilla.org/show_bug.cgi?id=1512942#c22

Comment 25

[Magnus Melin [:mkmelin]]

Just a note that bugzilla autolinks bug comments when you write something like bug 1512942 comment 22

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9385db6fc2ae
Convert timepicker-minute binding to custom element. r=philipp