Bug 101333

Summary:

Implement month picking to calendar picker

Product:

WebKit

Reporter:

Keishi Hattori <keishi>

Component:

Forms

Assignee:

Keishi Hattori <keishi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

100938

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Keishi Hattori

Reported 2012-11-06 04:20:20 PST

Implement month picking to calendar picker

Attachments
Patch (16.83 KB, patch) 2012-11-06 05:18 PST, Keishi Hattori	no flags	Details Formatted Diff Diff
Patch (17.80 KB, patch) 2012-11-06 18:41 PST, Keishi Hattori	no flags	Details Formatted Diff Diff
Patch (17.77 KB, patch) 2012-11-06 21:32 PST, Keishi Hattori	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Keishi Hattori

Comment 1 2012-11-06 05:18:27 PST

Created attachment 172557 [details] Patch

Kent Tamura

Comment 2 2012-11-06 18:09:32 PST

Comment on attachment 172557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172557&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:241 > -}; > +} nit: ";" should not be removed because this is a sentence. > Source/WebCore/Resources/pagepopups/calendarPicker.js:529 > + // We assume this._config.min/max are valid dates. dates -> dates or months > Source/WebCore/Resources/pagepopups/calendarPicker.js:1203 > + return Day.parse(this._days[5][6].dataset.submitValue).endDate(); please write as _days[DaysTable._Weeks - 1][7 - 1] for readability. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1385 > + * @param {!boolean} showEntireRange > + */ > +MonthPickerDaysTable.prototype.selectRange = function(month, showEntireRange) { Boolean argument is not good. Why don't you introduce another function like selectRangeAndShowEntireRange? We can remove ugly comment /*, showEntireRange*/ of DaysTable.prototype.selectRange. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1424 > + var firstNodeInSelectedRange = this._firstNodeInSelectedRange(); > + if (!firstNodeInSelectedRange > + && (key == "Right" || key == "Left" || key == "Up" || key == "Down" || key == "PageUp" || key == "PageDown")) { > + this.picker.showMonth(currentMonth); In what case does this happen? this.picker.showMonth(currentMonth) looks no-op. Why is this necessary?

Keishi Hattori

Comment 3 2012-11-06 18:41:01 PST

Created attachment 172692 [details] Patch

Keishi Hattori

Comment 4 2012-11-06 19:49:08 PST

Comment on attachment 172557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172557&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.js:241 >> +} > > nit: ";" should not be removed because this is a sentence. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:529 >> + // We assume this._config.min/max are valid dates. > > dates -> dates or months Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1203 >> + return Day.parse(this._days[5][6].dataset.submitValue).endDate(); > > please write as _days[DaysTable._Weeks - 1][7 - 1] for readability. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1385 >> +MonthPickerDaysTable.prototype.selectRange = function(month, showEntireRange) { > > Boolean argument is not good. > Why don't you introduce another function like selectRangeAndShowEntireRange? We can remove ugly comment /*, showEntireRange*/ of DaysTable.prototype.selectRange. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1424 >> + this.picker.showMonth(currentMonth); > > In what case does this happen? > this.picker.showMonth(currentMonth) looks no-op. Why is this necessary? This was a mistake. We should be selecting the current month when there is no selection and an arrow key is pressed.

Kent Tamura

Comment 5 2012-11-06 19:55:52 PST

Comment on attachment 172692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172692&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:1422 > +MonthPickerDaysTable.prototype.selectRangeAndShowEntireRange = function(month, showEntireRange) { showEntireRange argument is unnecessary.

Keishi Hattori

Comment 6 2012-11-06 21:32:51 PST

Created attachment 172714 [details] Patch

WebKit Review Bot

Comment 7 2012-11-06 22:30:11 PST

Comment on attachment 172714 [details] Patch Clearing flags on attachment: 172714 Committed r133722: <http://trac.webkit.org/changeset/133722>

WebKit Review Bot

Comment 8 2012-11-06 22:30:15 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.