Closed Bug 1381421 Opened 7 years ago Closed 7 years ago

[DateTimePicker] Handle dates earlier than 0001-01-01 and later than 275760-09-13 correctly

Categories

(Core :: Layout: Form Controls, enhancement, P1)

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1381421 - (Part 1) Handle dates earlier than 0001-01-01 and later than 275760-09-13 correctly.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1381421 - (Part 2) Add browser chrome tests for the minimum and maximum dates.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
The HTML Spec[1] defines the min date to be 0001-01-01, and the largest date defined in ECMAScript[2] is 275760-09-13. Dates outside of this range should not only be unselectable, but also display properly. The spinners and arrow buttons on the calendar should also prevent user from moving beyond the boundaries.

[1] https://html.spec.whatwg.org/#valid-date-string
[2] http://ecma-international.org/ecma-262/5.1/#sec-15.9.1.1
This patch addresses the obvious issues when displaying 0001-01-01 and 275760-09-13. Anything smaller or larger are not displayed and unselectable. There are other things that are also changed in this patch:
- In calendar.js, renamed set() to setCalendar() for clarity, and the method takes only `year` and `month` because `day` isn't relevant for displaying the calendar month.
- For the Calendar object, functions are passed in on creation rather than as props, because they are not expected to change.
Comment on attachment 8895285 [details]
Bug 1381421 - (Part 2) Add browser chrome tests for the minimum and maximum dates.

https://reviewboard.mozilla.org/r/166474/#review175068

::: toolkit/content/tests/browser/browser_datetime_datepicker.js:253
(Diff revision 2)
> +  Assert.deepEqual(
> +    getCalendarText(),
> +    [
> +      "31", "1", "2", "3",  "4",  "5",  "6",
> +      "7",  "8", "9", "10", "11", "12", "13",
> +      "",   "",  "",  "",   "",   "",   "",

lol, I guess I shouldn't schedule anything here then. :)
Attachment #8895285 - Flags: review?(mconley) → review+
Comment on attachment 8888713 [details]
Bug 1381421 - (Part 1) Handle dates earlier than 0001-01-01 and later than 275760-09-13 correctly.

https://reviewboard.mozilla.org/r/159746/#review175324

Hey scottwu, this looks great! Thanks so much! Just two notes below, but nothing major.

::: toolkit/content/widgets/calendar.js:35
(Diff revision 3)
> -    weekHeaders: []
> +    weekHeaders: [],
> +    setSelection: options.setSelection,
> +    getDayString: options.getDayString,
> +    getWeekHeaderString: options.getWeekHeaderString
>    };
>    this.props = {};

I guess this is unused now?

::: toolkit/content/widgets/datekeeper.js:81
(Diff revision 3)
> -                           new Date(this.state.today);
> +        month: month === undefined ? today.getMonth() : month
> +      });
>      },
> +
>      /**
> -     * Set new date. The year is always treated as full year, so the short-form
> +     * Set new calendar date. The year is always treated as full year, so the

Hm, I don't think "calendar date" is the right term here.

See [this article on Calendar Date](https://en.wikipedia.org/wiki/Calendar_date) - specifically:

"A calendar date is a reference to a particular day represented within a calendar system."

So I think "calendar date" is kinda confusing, since we're choosing only the month and year, and not the day.

I don't actually know what the term is for just the month and year. Perhaps we should call this function:

`setCalendarMonth`

and change this comment to be "Set new calendar month."
Attachment #8888713 - Flags: review?(mconley) → review+
Comment on attachment 8888713 [details]
Bug 1381421 - (Part 1) Handle dates earlier than 0001-01-01 and later than 275760-09-13 correctly.

https://reviewboard.mozilla.org/r/159746/#review175324

> I guess this is unused now?

Yes, thanks for spotting it!

> Hm, I don't think "calendar date" is the right term here.
> 
> See [this article on Calendar Date](https://en.wikipedia.org/wiki/Calendar_date) - specifically:
> 
> "A calendar date is a reference to a particular day represented within a calendar system."
> 
> So I think "calendar date" is kinda confusing, since we're choosing only the month and year, and not the day.
> 
> I don't actually know what the term is for just the month and year. Perhaps we should call this function:
> 
> `setCalendarMonth`
> 
> and change this comment to be "Set new calendar month."

I agree. `setCalendarMonth` sounds good. I shall rename it that.
Comment on attachment 8895285 [details]
Bug 1381421 - (Part 2) Add browser chrome tests for the minimum and maximum dates.

https://reviewboard.mozilla.org/r/166474/#review175068

> lol, I guess I shouldn't schedule anything here then. :)

Yeah might disappoint some sci-fi fans but ¯\_(ツ)_/¯
Thanks again Mike! Checking it in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4ddc77083d20
(Part 1) Handle dates earlier than 0001-01-01 and later than 275760-09-13 correctly. r=mconley
https://hg.mozilla.org/integration/autoland/rev/48bb3e020058
(Part 2) Add browser chrome tests for the minimum and maximum dates. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ddc77083d20
https://hg.mozilla.org/mozilla-central/rev/48bb3e020058
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: