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)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: scottwu, Assigned: scottwu)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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 ¯\_(ツ)_/¯
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
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.
Description
•