Implement UI for <input type="date">

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
XUL Widgets
P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [milestone4])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

8 months ago
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
Assignee: nobody → scwwu
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

8 months ago
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!
Flags: needinfo?(mconley)
Thanks scottwu, I'll have a review for you this week as soon as I can (probably tomorrow?).
Flags: needinfo?(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.
(Assignee)

Comment 9

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Whiteboard: [milestone4]
(Assignee)

Updated

8 months ago
Blocks: 1320647
(Assignee)

Updated

8 months ago
Blocks: 1320649
(Assignee)

Updated

8 months ago
Blocks: 1320650
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 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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

8 months ago
mozreview-review-reply
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.
(Assignee)

Updated

8 months ago
Blocks: 1320880
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. :/
Attachment #8815185 - Flags: review?(mconley) → review+
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?
Attachment #8810283 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

8 months ago
mozreview-review-reply
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.
(Assignee)

Comment 21

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 23

8 months ago
(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!
Flags: needinfo?(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!
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

7 months ago
Thanks Mike! Rebased to central.
Keywords: checkin-needed
Scott, mike: mozreview still 2 open issues. can this be fixed so that we can use the autolander ? thanks!
Flags: needinfo?(scwwu)
err i mean mozreview list 2 open issues :)

Updated

7 months ago
Keywords: checkin-needed
(Assignee)

Comment 30

7 months ago
Oh yes I have addressed the issues but didn't close them. I closed them just now. Thanks!
Flags: needinfo?(scwwu)
Keywords: checkin-needed
thanks scott! landed

Comment 32

7 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
had to back this out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7882571&repo=autoland
Flags: needinfo?(scwwu)

Comment 34

7 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/232292eb2009
Backed out changeset 315469b40e6d
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

7 months ago
Sorry about that :Tomcat. I've fixed the eslint problem. Thanks!
Flags: needinfo?(scwwu)
Keywords: checkin-needed
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 40

7 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Priority: -- → P1
Blocks: 1323674
No longer blocks: 1323674

Comment 41

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d0f35ad8e7a4
https://hg.mozilla.org/mozilla-central/rev/35cd8ae08fd8
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
See Also: → bug 1286182

Comment 42

7 months ago
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
(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
Depends on: 1331608

Comment 44

6 months ago
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.

Updated

5 months ago
Depends on: 1346686
You need to log in before you can comment on or make changes to this bug.