1524456 - [de-xbl] Replace datepicker widgets (timepicker, datetimepicker-base, datepicker, datepicker-grid, datepicker-popup) with non-XBL elements

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Geoff Lankow (:darktrojan)]

The widgets are pretty broken right now on account of bug 1518932. I think now is the time to replace them (and possibly the minimonth) completely without XBL.

Comment 1

[Jorg K (CEST = GMT+2)]

Something for the de-XBL team.

Comment 2

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/source/common/bindings/datetimepicker.xml

Note toolkit has DateTimePickerPanel.jsm and DateTimePickerParent.jsm. Need to investigate if we can/want/should move to using that.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-1.diff

This is a work in progress. I haven't made any attempt to check what it looks like on Windows or Mac yet (although most of the styling is in common CSS files). It does pass the tests.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: Screen Shot 2019-02-08 at 11.33.09.png

As I said to Philipp, I've had to remove the month/year picker popups as the menuitems in them interfere with the menulist. I think this is a reasonable replacement, and it's the same as seen on Ubuntu and probably other Linux variants.

Comment 5

[Richard Marti (:Paenglab)]

Checked on Windows and it looks good.

Comment 6

[Geoff Lankow (:darktrojan)]

Okay, now that I've discovered the timepicker grid can't handle keyboard input, I won't try to fix that here. This is ready, I think.

Comment 7

[[:MakeMyDay]]

Even if the dropdwon in the menulist isn't supported anymore out of the box, we should find a different way to retain the capability to quickly switch to a distant month. Having to press the arrow buttons multiple times is a pita.

Comment 8

[Jorg K (CEST = GMT+2)]

How do you switch to a distant month? I use "Lightning Calendar Tabs" to at least switch a few months back and forward.

Comment 9

[[:MakeMyDay]]

This bug is about the datepicker (see the screenshot Geoff posted), not the calendar view (and deferring to an addon is not appropriate here - a datepicker is an essential part of a calendar apprlication). The month and year labels are dropdown controls.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9042308 [details] [diff] [review]
1524456-datepicker-customelement-1.diff

When the popup menu is no more possible then this solution is okay for me. To switch the month it's also possible with the mouse scroll wheel, and this can be fast.

Please could you try to move the month- and year label a bit upwards? They don't look centered like the arrow buttons.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Richard Marti (:Paenglab) from comment #10)

Please could you try to move the month- and year label a bit upwards? They don't look centered like the arrow buttons.

Yeah, it does look a bit wrong. There's also something odd going on with the styling of the header I haven't got to the bottom of yet.

When the popup menu is no more possible then this solution is okay for me. To switch the month it's also possible with the mouse scroll wheel, and this can be fast.

Although I notice that scrolling the year changes months, I'd better fix that.

(In reply to [:MakeMyDay] from comment #7)

Even if the dropdwon in the menulist isn't supported anymore out of the box, we should find a different way to retain the capability to quickly switch to a distant month. Having to press the arrow buttons multiple times is a pita.

I'm thinking about a click-and-hold behaviour, although I haven't got further than thinking about it yet.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-2.diff

Richard, I know you already looked at this, but I was looking at mail/themes/*/mail/datetimepicker.css and can't see anything of any value. Is there any reason I shouldn't eliminate those stylesheets?

Comment 13

[Richard Marti (:Paenglab)]

The mail/themes/*/mail/datetimepicker.css are for the datetimepicker in common/bindings/datetimepicker.xml. Mostly used for the Birthday picker in addressbook and for extensions. We forked it from toolkit when they removed this bindings. It would be great if we could move it to use the calendar datetimepicker and delete this one.

Comment 14

[Magnus Melin [:mkmelin]]

<input type="date"> actually has a pretty similar picker. But bug 1527615 :(

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9042872 [details] [diff] [review]
1524456-datepicker-customelement-2.diff

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

Might want to hg mv --after to preserve some blame

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-3.diff

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-4.diff

Bug 1524457 now has review, and this needs to land with it.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9045084 [details] [diff] [review]
1524456-datepicker-customelement-4.diff

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

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +1,2 @@
> +/* eslint-disable indent-legacy */
> +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");

this file is missing the MPL boileplate

Comment 19

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

Comment on attachment 9045084 [details] [diff] [review]
1524456-datepicker-customelement-4.diff

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

r=me with these changes and what Magnus said

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +1,1 @@
> +/* eslint-disable indent-legacy */

Why is the indent disabled here? I'd prefer to just add the indent.

@@ +2,5 @@
> +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +customElements.whenDefined("menulist-editable").then(() => {
> +class CalendarDatePicker extends MozXULElement {
> +    connectedCallback() {

Do we need to call super here as well?

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-5.diff

Requested changes and all the other stuff I've discovered today. :(

The interdiff would work if it ignored white-space. I'd intended to do the indent, just not at the point ESLint started complaining, and then I forgot.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: 1524456-datepicker-customelement-5-inter.diff

Here, this might make it easier.

Comment 22

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

Comment on attachment 9045145 [details] [diff] [review]
1524456-datepicker-customelement-5-inter.diff

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

::: b/calendar/resources/content/datetimepickers/datetimepickers.js
@@ +1,4 @@
> +/**
> + * 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/. */

Not sure where you got this one from, but it should be like this:

```
/* 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/. */
```

Comment 23

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

Comment on attachment 9045141 [details] [diff] [review]
1524456-datepicker-customelement-5.diff

r+ taking the super comment from above into account, and fixed license header. Thanks for the interdiff!

Comment 24

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

I'd appreciate a followup bug for restoring the functionality the dropdowns provided. It doesn't have to be a dropdown, but something to make it easier to go to distant dates is good. I said I would not complain if Richard ui-r+'d it, but since MakeMyDay mentions as well I think it is worth considering to bring it back.

Comment 25

[Geoff Lankow (:darktrojan)]

taking the super comment from above into account

There is no super function to call.

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

I'd appreciate a followup bug for restoring the functionality the dropdowns provided. It doesn't have to be a dropdown, but something to make it easier to go to distant dates is good. I said I would not complain if Richard ui-r+'d it, but since MakeMyDay mentions as well I think it is worth considering to bring it back.

You can scroll the months and years, and as I mentioned I'll add click-and-hold on the arrows. Having a popup on a popup is … problematic.

Comment 26

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

(In reply to Geoff Lankow (:darktrojan) from comment #25)

I'd appreciate a followup bug for restoring the functionality the dropdowns provided. It doesn't have to be a dropdown, but something to make it easier to go to distant dates is good. I said I would not complain if Richard ui-r+'d it, but since MakeMyDay mentions as well I think it is worth considering to bring it back.

You can scroll the months and years, and as I mentioned I'll add click-and-hold on the arrows. Having a popup on a popup is … problematic.

What about click to turn the year into a textbox?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/753ca1edaf65
Convert datepicker and timepicker classes to custom elements; r=Fallen

Comment 28

[Arshad Khan [:arshad]]

Comment on attachment 9045141 [details] [diff] [review]
1524456-datepicker-customelement-5.diff

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

::: calendar/resources/content/datetimepickers/datetimepickers.xml
@@ -1052,3 @@
>        <!-- Set up the picker, called when the popup pops -->
>        <method name="onPopupShowing">
> -        <parameter name="aPicker"/>

Why ws picker and popup removed? there are some places where picker and popup are used but since they are not being set, error `mpicker is null` thrown.

Comment 29

[Arshad Khan [:arshad]]

Ah I read the https://bugzilla.mozilla.org/show_bug.cgi?id=1524456#c6 and https://bugzilla.mozilla.org/show_bug.cgi?id=1524456#c4
now. Example of mpicker usuage is https://searchfox.org/comm-central/source/calendar/resources/content/datetimepickers/datetimepickers.xml#738 . As of now, I ll leave it as it is in Bug 1512941.

Comment 30

[Stefan Sitter [:ssitter]]

Could this change be responsible for Bug 1536517?