1283385 - Implement UI for <input type="date">

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1283385 - (1/2) Implement date picker UI;

Review commit: https://reviewboard.mozilla.org/r/92636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92636/

Comment 2

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/1-2/

Comment 3

[Scott Wu [:scottwu]]

New WIP updated, almost ready for a review.

Done:
- Improved calendar rendering to only update the changes
- Stop updating calendar if month year picker is covering the calendar
- Stop updating month year spinners if the month year picker is not visible
- DateKeeper reuses days, months, years arrays if possible

TODOs:
- Replace `new Date` calls with `DateUTC` and `DateKeeper` methods. Also fix end-of-month bug, where from 1/31 to next month should be 2/28 or 2/29 instead of 3/2 or 3/3
- Update documentations

Comment 4

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/2-3/

Comment 5

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/3-4/

Comment 6

[Scott Wu [:scottwu]]

I think I'm ready for the first review. Even though visually there are more elements, I've maintained the same structure as the month picker.

Since the DOM & input box portion is still being worked on, this patch only contains the xhtml portion. You can test it at chrome://global/content/datePicker.xhtml

The initialization process should happen in datetimepopup.xml but for testing purpose I'm placing it in datepicker.xhtml for now. Once initialized, the DatePicker creates a Calendar and a MonthYear component. The MonthYear component in turn creates Spinners for years and months.

The DateKeeper object handles date changes and prepares days, months, and years arrays for display in calendar and spinners.

I'll keep working on styling the month-year picker button and replacing arrows with svg, as well as implementing min/max & step functionalities.

Thanks!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Thanks scottwu, I'll have a review for you this week as soon as I can (probably tomorrow?).

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review95578

This looks pretty good, and I'm willing to sign off on this with the following questions answered (see below).

I also have a larger, architectural question:

It looks like you're doing a lot of work to have these classes that respond appropriately to state updates on things like the selected date within your datekeeper.

Is it worth investigating whether using a library like React would reduce some of the boilerplate here for monitoring state and constructing / updating the DOM when necessary? Have you experience working with React?

::: toolkit/content/datepicker.xhtml:52
(Diff revision 4)
> +    monthYear: document.querySelector(".month-year"),
> +    monthYearView: document.querySelector(".month-year-view"),
> +    buttonLeft: document.querySelector(".left"),
> +    buttonRight: document.querySelector(".right"),
> +    weekHeader: document.querySelector(".week-header"),
> +    daysView: document.querySelector(".days-view")

Since these are all contained within the root, I wonder if it'd make more sense to call querySelector on the root itself instead of the document.

::: toolkit/content/datepicker.xhtml:59
(Diff revision 4)
> +  // TODO: initialize from datetimepopup.xml
> +  // Debugging start
> +  const now = new Date();
> +  window.postMessage({
> +    name: "DatePickerInit",
> +    detail: {
> +      year: now.getFullYear(),
> +      month: now.getMonth(),
> +      date: now.getDate(),
> +      locale: "en-US"
> +    }
> +  }, "*");

Should this debugging stuff still be in here?

::: toolkit/content/widgets/datepicker.js:59
(Diff revision 4)
> +        // TODO: use calendar terms when available (Bug 1287677)
> +        getWeekHeaderString: weekday => ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"][weekday],

Has a bug / bugs been filed to deal with all of the leftover TODOs for this and the timepicker?

::: toolkit/content/widgets/datepicker.js:63
(Diff revision 4)
> +          this.state.selectionValue = selectionValue;
> +          this.state.isYearSet = true;
> +          this.state.isMonthSet = true;
> +          this.state.isDateSet = true;

Are these supposed to have some default values before they're set here?

::: toolkit/content/widgets/datepicker.js:150
(Diff revision 4)
> +        this.context.monthYearView.classList.remove("hidden") :
> +        this.context.monthYearView.classList.add("hidden");
> +    },
> +
> +    /**
> +     * Dispatch CustomEvent to pass the state of picker to the panel.

Not a CustomEvent - you're using a message.

::: toolkit/content/widgets/datepicker.js:190
(Diff revision 4)
> +          if (event.target == this.context.buttonLeft) {
> +            this.state.dateKeeper.setMonthByOffset(-1);
> +            this._update();
> +          } else if (event.target == this.context.buttonRight) {
> +            this.state.dateKeeper.setMonthByOffset(1);
> +            this._update();
> +          }

Am I correct when I say this is going to work incorrectly in RTL locales?

::: toolkit/themes/shared/datetimepickers.css:1
(Diff revision 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/. */

FWIW, I think it might be worth doing an hg mv from toolkit/themes/shared/timepicker.css to toolkit/themes/shared/datetimepickers.css in order to make it easier to see what changed here.

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review95578

Yes I had looked into React and decided to adopt a pattern similar to it. If I had used React then I'd be able remove some boilerplate code, mostly around keeping track of state changes, and when to update DOM elements. However I don't think the benefit is that great to introduce a sizable dependency. Since the pickers are quite small and the DOM tree doesn't change that much (rarely do I need to add/remove nodes), I'm able to take a few shortcuts to model what React does without using it. Having said that, there are definitely room for improvement if I understand more about React.

I'm curious what you think of including dependencies? I know on the web people are more liberal about including external scripts, but I feel that we are a lot more conservative about it for Firefox front-end.

> Since these are all contained within the root, I wonder if it'd make more sense to call querySelector on the root itself instead of the document.

Yeah that makes sense.

> Should this debugging stuff still be in here?

No, I kept it there so that you could test it if you want. It's gone!

> Has a bug / bugs been filed to deal with all of the leftover TODOs for this and the timepicker?

Not yet for this one and a few other date picker features. I'll create them add them to the meta bug.

> Are these supposed to have some default values before they're set here?

Their default values are false. I've made that explicit.

> Not a CustomEvent - you're using a message.

Got it. Thanks!

> Am I correct when I say this is going to work incorrectly in RTL locales?

To be honest I haven't thought of that and never dealt with RTL before. Any suggestions?

> FWIW, I think it might be worth doing an hg mv from toolkit/themes/shared/timepicker.css to toolkit/themes/shared/datetimepickers.css in order to make it easier to see what changed here.

Oh! I didn't realize that. I thought that's just what happens when you move a file and change its content. It's strange because it does say I renamed file when I commit. Maybe it's because I'm using git cinnabar and something went wrong. Could be this bug: https://github.com/glandium/git-cinnabar/issues/10#issuecomment-262888840

Comment 10

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/4-5/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review95578

> To be honest I haven't thought of that and never dealt with RTL before. Any suggestions?

In general, it's often best to let Gecko handle the ordering of left-to-right items.

See https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/dir. Also see https://addons.mozilla.org/en-US/firefox/addon/force-rtl/ to put Gecko in RTL mode for testing.

It's not unheard of to do RTL work in follow-up bugs, but it's definitely something to keep in mind.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review95578

> Oh! I didn't realize that. I thought that's just what happens when you move a file and change its content. It's strange because it does say I renamed file when I commit. Maybe it's because I'm using git cinnabar and something went wrong. Could be this bug: https://github.com/glandium/git-cinnabar/issues/10#issuecomment-262888840

Okay. Would it be possible for you to manually post an interdiff between the original timepicker.css and datetimepickers.css so I can more easily review the changes?

Comment 13

[Scott Wu [:scottwu]]

Attachment: Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review commit: https://reviewboard.mozilla.org/r/96204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96204/

Comment 14

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/5-6/

Comment 15

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review95578

> In general, it's often best to let Gecko handle the ordering of left-to-right items.
> 
> See https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/dir. Also see https://addons.mozilla.org/en-US/firefox/addon/force-rtl/ to put Gecko in RTL mode for testing.
> 
> It's not unheard of to do RTL work in follow-up bugs, but it's definitely something to keep in mind.

Cool, thanks. I'll file a follow up bug for RTL support. I looked into how RTL works in front-end and I saw people use :-moz-locale-dir(rtl) to detect RTL locales. However I don't think I can do that because the date picker lives inside an iframe, so I'll probably need to pass in the locale direction when initializing the pop up.

> Okay. Would it be possible for you to manually post an interdiff between the original timepicker.css and datetimepickers.css so I can more easily review the changes?

I did a workaround by splitting the edit and rename into 2 different commits. Hope that's okay.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

https://reviewboard.mozilla.org/r/96204/#review96520

::: toolkit/themes/shared/jar.inc.mn:24
(Diff revision 1)
>    skin/classic/global/aboutReaderContent.css               (../../shared/aboutReaderContent.css)
>  * skin/classic/global/aboutReaderControls.css              (../../shared/aboutReaderControls.css)
>    skin/classic/global/aboutSupport.css                     (../../shared/aboutSupport.css)
>    skin/classic/global/appPicker.css                        (../../shared/appPicker.css)
>    skin/classic/global/config.css                           (../../shared/config.css)
> -  skin/classic/global/timepicker.css                       (../../shared/timepicker.css)
> +  skin/classic/global/datetimepickers.css                  (../../shared/datetimepickers.css)

There's also datetimepicker.css elsewhere in the tree - I guess there's a XUL-based date/time picker that XUL apps can use. I don't actually see it being used by Firefox anywhere (probably used by TB / SM), but it's an unfortunate naming collision.

Is there anything we can do here to make this distinct from the XUL datetimepicker.css file? Maybe these should be HTMLdatetimepicker.css? I don't have a great suggestion here. :/

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review96542

Thanks for your patience while I reviewed it!

::: toolkit/content/widgets/calendar.js:49
(Diff revision 6)
> +     *          {Array<Object>} days: Data for days
> +     *          {Array<Object>} weekHeaders: Data for weekHeaders

It might be a good idea to give some more detail on what these <Object> things contain.

::: toolkit/content/widgets/datekeeper.js:118
(Diff revision 6)
> +     */
> +    getMonths() {
> +      // TODO: add min/max and step support
> +      let months = [];
> +
> +      for (let i = 0; i < 12; i++) {

`const` for the magic 12 please.

::: toolkit/content/widgets/datekeeper.js:150
(Diff revision 6)
> +      // last item range. If not, return the cached result.
> +      if (!firstItem || !lastItem ||
> +          currentYear <= firstItem.value + YEAR_BUFFER_SIZE ||
> +          currentYear >= lastItem.value - YEAR_BUFFER_SIZE) {
> +        // The year is set in the middle with items on both directions
> +        for (let i = -(YEAR_VIEW_SIZE / 2); i < YEAR_VIEW_SIZE / 2; i++) {

I think this is the first time I've ever seen a for loop go from a negative value to a positive value like this in practice, and used with addition. Neat. :)

::: toolkit/content/widgets/datekeeper.js:232
(Diff revision 6)
> +      return this._newUTCDate(
> +        firstDayOfMonth.getUTCFullYear(),
> +        firstDayOfMonth.getUTCMonth(),
> +        // When first calendar date is the same as first day of the week, add
> +        // another row on top of it.
> +        firstDayOfWeek == dayOfWeek ? -6 : (firstDayOfWeek - dayOfWeek - 6) % DAYS_IN_A_WEEK);

I assume this 6 should always be DAYS_IN_A_WEEK - 1?

::: toolkit/themes/shared/timepicker.css:91
(Diff revision 6)
> +  opacity: 1;
> +  transition: opacity 0.15s, visibility 0.15s;
> +}
> +
> +.month-year-view.hidden {
> +  visibility: hidden;

I'm curious why you're transitioning visibility to hidden, while also having opacity at 0. Is there some advantage to having the visibility set to hidden as well that I'm not aware of?

Comment 18

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/6-7/

Comment 19

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96204/diff/1-2/

Comment 20

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

https://reviewboard.mozilla.org/r/92636/#review96542

> I assume this 6 should always be DAYS_IN_A_WEEK - 1?

I think it makes sense to name it daysOffset = -6, though I suspect many things will break if we don't have a 7 days week, but for now 7 days in a week seems like a very reasonable assumption.

> I'm curious why you're transitioning visibility to hidden, while also having opacity at 0. Is there some advantage to having the visibility set to hidden as well that I'm not aware of?

Visually we want the month-year picker to fade in, but it would shield the calendar from mouse events if visible (with 0 opacity).

I could rely on visibility for fading in, but a flash would happen to the calendar when I make the picker on top invisible. It may have something to do with how layout renders the layers.

Comment 21

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

https://reviewboard.mozilla.org/r/96204/#review96520

> There's also datetimepicker.css elsewhere in the tree - I guess there's a XUL-based date/time picker that XUL apps can use. I don't actually see it being used by Firefox anywhere (probably used by TB / SM), but it's an unfortunate naming collision.
> 
> Is there anything we can do here to make this distinct from the XUL datetimepicker.css file? Maybe these should be HTMLdatetimepicker.css? I don't have a great suggestion here. :/

Perhaps datetimeinputs.css or datetimeinputpickers.css

Comment 22

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96204/diff/2-3/

Comment 23

[Scott Wu [:scottwu]]

(In reply to Scott Wu [:scottwu] from comment #21)
> > Is there anything we can do here to make this distinct from the XUL datetimepicker.css file? Maybe these should be HTMLdatetimepicker.css? I don't have a great suggestion here. :/
> 
> Perhaps datetimeinputs.css or datetimeinputpickers.css

I think datetimeinputpickers.css should be clear enough. If this looks okay then I'll do checkin-needed. Thanks!

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to Scott Wu [:scottwu] from comment #23)
> (In reply to Scott Wu [:scottwu] from comment #21)
> > > Is there anything we can do here to make this distinct from the XUL datetimepicker.css file? Maybe these should be HTMLdatetimepicker.css? I don't have a great suggestion here. :/
> > 
> > Perhaps datetimeinputs.css or datetimeinputpickers.css
> 
> I think datetimeinputpickers.css should be clear enough. If this looks okay
> then I'll do checkin-needed. Thanks!

Yeah, let's do it! Thanks!

Comment 25

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/7-8/

Comment 26

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96204/diff/3-4/

Comment 27

[Scott Wu [:scottwu]]

Thanks Mike! Rebased to central.

Comment 28

[Carsten Book [:Tomcat]]

Scott, mike: mozreview still 2 open issues. can this be fixed so that we can use the autolander ? thanks!

Comment 29

[Carsten Book [:Tomcat]]

err i mean mozreview list 2 open issues :)

Comment 30

[Scott Wu [:scottwu]]

Oh yes I have addressed the issues but didn't close them. I closed them just now. Thanks!

Comment 31

[Carsten Book [:Tomcat]]

thanks scott! landed

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/effd2d3a31e0
(1/2) Implement date picker UI; r=mconley
https://hg.mozilla.org/integration/autoland/rev/315469b40e6d
(2/2) Rename timepicker.css to datetimeinputpickers.css; r=mconley

Comment 33

[Carsten Book [:Tomcat]]

had to back this out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7882571&repo=autoland

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/232292eb2009
Backed out changeset 315469b40e6d

Comment 35

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/8-9/

Comment 36

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96204/diff/4-5/

Comment 37

[Scott Wu [:scottwu]]

Sorry about that :Tomcat. I've fixed the eslint problem. Thanks!

Comment 38

[Scott Wu [:scottwu]]

Comment on attachment 8810283 [details]
Bug 1283385 - (1/2) Implement date picker UI;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92636/diff/9-10/

Comment 39

[Scott Wu [:scottwu]]

Comment on attachment 8815185 [details]
Bug 1283385 - (2/2) Rename timepicker.css to datetimeinputpickers.css;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96204/diff/5-6/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0f35ad8e7a4
(1/2) Implement date picker UI; r=mconley
https://hg.mozilla.org/integration/autoland/rev/35cd8ae08fd8
(2/2) Rename timepicker.css to datetimeinputpickers.css; r=mconley

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0f35ad8e7a4
https://hg.mozilla.org/mozilla-central/rev/35cd8ae08fd8

Comment 42

[Graham Armfield]

Has any testing of your date picker been done with a screen reader - like NVDA or Jaws?

Also, does this fix bug 1308952? https://bugzilla.mozilla.org/show_bug.cgi?id=1308952

Comment 43

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Graham Armfield from comment #42)
> Has any testing of your date picker been done with a screen reader - like
> NVDA or Jaws?
> 
> Also, does this fix bug 1308952?
> https://bugzilla.mozilla.org/show_bug.cgi?id=1308952

This date picker (on desktop) is still under construction.
The recent landed pieces and upcoming weeks of code will all be "pref-off", because we are incrementally delivering the feature and at this moment, it's still early stage.
 
Later on we will integrate localization, and then accessibility. 
We will definitely consider your input when planning on the accessibility testing. Thanks a lot for the suggestion.

For more details and future plan, feel free to refer to: https://wiki.mozilla.org/TPE_DOM/Date_time_input_types

Comment 44

[Alfred Kayser]

Issues in this implementation:
1. Selected date is not marked in daysView.
2. Scrolling in the spinner feel the wrong way around (for the non-mac users)
3. No today indication/jump to today button.
4. Hiding hover/selection during scrolling makes it feel slow as it takes ages to re-appear.
5. Hardcoded sizes in code.
6. Font of day is very small.