Implement month picking to calendar picker

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Implement month picking to calendar picker

[Keishi Hattori]

Created attachment 172557 [details]
Patch

[Kent Tamura]

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]

Created attachment 172692 [details]
Patch

[Keishi Hattori]

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 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]

Created attachment 172714 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172714 [details]
Patch

Clearing flags on attachment: 172714

Committed r133722: <http://trac.webkit.org/changeset/133722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.