Closed Bug 1283385 Opened 5 years ago Closed 5 years ago

Implement UI for <input type="date">

Categories

(Toolkit :: XUL Widgets, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [milestone4])

Attachments

(2 files)

No description provided.
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
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.
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
Whiteboard: [milestone4]
Blocks: 1320647
Blocks: 1320649
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 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.
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 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 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
(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)
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 :)
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
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)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/232292eb2009
Backed out changeset 315469b40e6d
Sorry about that :Tomcat. I've fixed the eslint problem. Thanks!
Flags: needinfo?(scwwu)
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/d0f35ad8e7a4
https://hg.mozilla.org/mozilla-central/rev/35cd8ae08fd8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
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.
Depends on: 1346686
You need to log in before you can comment on or make changes to this bug.