Closed Bug 1524456 Opened 6 years ago Closed 6 years ago

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

Categories

(Calendar :: General, enhancement)

Product:
Calendar
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Screen Shot 2019-02-08 at 11.33.09.png
(deleted), image/png
Details
1524456-datepicker-customelement-5.diff
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review
1524456-datepicker-customelement-5-inter.diff
(deleted), patch
Details | Diff | Splinter Review

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.

Something for the de-XBL team.

Flags: needinfo?(mkmelin+mozilla)

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.

Blocks: tb-war-on-xbl
Flags: needinfo?(mkmelin+mozilla)
Summary: Replace datepicker widgets with non-XBL elements → [de-xbl] Replace datepicker widgets (timepicker, datetimepicker-base, datepicker, datepicker-grid, datepicker-popup) with non-XBL elements
Blocks: 1520781
Attached patch 1524456-datepicker-customelement-1.diff (obsolete) (deleted) — — Splinter Review

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.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attached image Screen Shot 2019-02-08 at 11.33.09.png (deleted) —

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.

Checked on Windows and it looks good.

Depends on: 1526552

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.

Attachment #9042308 - Flags: ui-review?(richard.marti)
Attachment #9042308 - Flags: review?(philipp)

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.

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

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 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.
Attachment #9042308 - Flags: ui-review?(richard.marti) → ui-review+

(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.

Attached patch 1524456-datepicker-customelement-2.diff (obsolete) (deleted) — — Splinter Review

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?

Flags: needinfo?(richard.marti)
Attachment #9042872 - Flags: review?(philipp)
Attachment #9042308 - Attachment is obsolete: true
Attachment #9042308 - Flags: review?(philipp)

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.

Flags: needinfo?(richard.marti)

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

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
Attached patch 1524456-datepicker-customelement-3.diff (obsolete) (deleted) — — Splinter Review
Attachment #9042872 - Attachment is obsolete: true
Attachment #9042872 - Flags: review?(philipp)
Attachment #9044078 - Flags: review?(philipp)
Attached patch 1524456-datepicker-customelement-4.diff (obsolete) (deleted) — — Splinter Review

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

Attachment #9044078 - Attachment is obsolete: true
Attachment #9044078 - Flags: review?(philipp)
Attachment #9045084 - Flags: review?(philipp)
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 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?
Attachment #9045084 - Flags: review?(philipp) → review+

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.

Attachment #9045084 - Attachment is obsolete: true
Attachment #9045141 - Flags: review?(philipp)

Here, this might make it easier.

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 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!
Attachment #9045141 - Flags: review?(philipp) → review+

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.

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.

(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?

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/753ca1edaf65
Convert datepicker and timepicker classes to custom elements; r=Fallen

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 6.9
Depends on: 1529433
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.

Could this change be responsible for Bug 1536517?

Depends on: 1538588
Regressions: 1551081
Blocks: 1526552
No longer depends on: 1526552
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: