Bug 101194 – Introduce Day class to calendar picker

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Introduce Day class to calendar picker

[Keishi Hattori]

Created attachment 172459 [details]
Patch

[Kent Tamura]

Comment on attachment 172459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172459&action=review

> Source/WebCore/Resources/pagepopups/calendarPicker.js:242
> +Day.createFromToday = function() {
> +    return Day.createFromDate(new Date());

I think this doesn't work as expected during 00:00 - 09:00 in Japan Standard Time.
For example, "new Date()" has 2012-11-06T07:00 in the local time, but getUTCDate() returns 5.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:268
> + * @return {!Day}

!Day -> !Date

> Source/WebCore/Resources/pagepopups/calendarPicker.js:507
> +    this._minimumValue = (typeof this._config.min !== "undefined") ? parseDateString(this._config.min).valueOf() : Day.Minimum;

Type mismatch. parseDateString().valueOf() is a number and Day.Minimum is a Date.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:620
> +CalendarPicker.prototype.shouldShowMonth = function(month) {

Please add jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:624
> +CalendarPicker.prototype.showMonth = function(month, animate, keepSelectionPosition) {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:636
> +CalendarPicker.prototype.currentMonth = function() {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:771
> -        this._left3.disabled = current - 13 < min;
> -    this._left2.disabled = current - 2 < min;
> -    this._left1.disabled = current - 1 < min;
> -    this._right1.disabled = current + 1 > max;
> -    this._right2.disabled = current + 2 > max;
> +        this._left3.disabled = !this.picker.shouldShowMonth(new Month(monthValue - 121));
> +    this._left2.disabled = !this.picker.shouldShowMonth(new Month(monthValue - 12));
> +    this._left1.disabled = !this.picker.shouldShowMonth(new Month(monthValue - 1));
> +    this._right1.disabled = !this.picker.shouldShowMonth(new Month(monthValue + 1));
> +    this._right2.disabled = !this.picker.shouldShowMonth(new Month(monthValue + 12));
>      if (this._right3)
> -        this._right3.disabled = current + 13 > max;
> -    this._month.innerText = this._currentMonth.toLocaleString();
> +        this._left3.disabled = !this.picker.shouldShowMonth(new Month(monthValue + 121));

This changes behavior.  Is it intentional?
The current code intention is that _left2 button moves to min(minimum-month, current-month - 12) and should be disabled if _left2 would have same behavior with _left1.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1047
>   * @param {!number} time date in millisecond.
>   * @return {!boolean}
>   */
> -CalendarPicker.prototype.isValidDate = function(time) {
> -    return !this.outOfRange(time) && !this.stepMismatch(time);
> +CalendarPicker.prototype.isValidDate = function(range) {
> +    var value = range.valueOf();

Please update the annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1056
>   * @param {!number} month
>   */
>  DaysTable.prototype._renderMonth = function(month) {
> -    this._currentMonth = new Month(month);
> -    var dayIterator = this._currentMonth.startDate();
> +    var dayIterator = month.startDate();

Ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1091
> +DaysTable.prototype.navigateToMonth = function(month, animate, keepSelectionPosition) {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1174
> -    this._navigateToMonthWithAnimation(currentMonth.previous());
> +    this.picker.showMonth(previousMonth);

The original code is "WithAnimation" but the new code has no "animation" flag.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1185
> -    this._navigateToMonthWithAnimation(currentMonth.next());
> +    this.picker.showMonth(nextMonth);

Ditto.

[Keishi Hattori]

Created attachment 172480 [details]
Patch

[Keishi Hattori]

Comment on attachment 172459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172459&action=review

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:242
>> +    return Day.createFromDate(new Date());
> 
> I think this doesn't work as expected during 00:00 - 09:00 in Japan Standard Time.
> For example, "new Date()" has 2012-11-06T07:00 in the local time, but getUTCDate() returns 5.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:268
>> + * @return {!Day}
> 
> !Day -> !Date

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:507
>> +    this._minimumValue = (typeof this._config.min !== "undefined") ? parseDateString(this._config.min).valueOf() : Day.Minimum;
> 
> Type mismatch. parseDateString().valueOf() is a number and Day.Minimum is a Date.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:620
>> +CalendarPicker.prototype.shouldShowMonth = function(month) {
> 
> Please add jsdoc annotation.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:624
>> +CalendarPicker.prototype.showMonth = function(month, animate, keepSelectionPosition) {
> 
> ditto.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:636
>> +CalendarPicker.prototype.currentMonth = function() {
> 
> ditto.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:771
>> +        this._left3.disabled = !this.picker.shouldShowMonth(new Month(monthValue + 121));
> 
> This changes behavior.  Is it intentional?
> The current code intention is that _left2 button moves to min(minimum-month, current-month - 12) and should be disabled if _left2 would have same behavior with _left1.

I didn't realize. Reverting the behavior.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1047
>> +    var value = range.valueOf();
> 
> Please update the annotation.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1056
>> +    var dayIterator = month.startDate();
> 
> Ditto.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1091
>> +DaysTable.prototype.navigateToMonth = function(month, animate, keepSelectionPosition) {
> 
> ditto.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1174
>> +    this.picker.showMonth(previousMonth);
> 
> The original code is "WithAnimation" but the new code has no "animation" flag.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1185
>> +    this.picker.showMonth(nextMonth);
> 
> Ditto.

Done.

[Kent Tamura]

Comment on attachment 172480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172480&action=review

> Source/WebCore/Resources/pagepopups/calendarPicker.js:632
> + * @param {!bool} animate
> + * @param {!bool} keepSelectionPosition

Some callsites omit these bool arguments.  They should be {bool=}.

[Keishi Hattori]

Created attachment 172482 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172482 [details]
Patch

Clearing flags on attachment: 172482

Committed r133565: <http://trac.webkit.org/changeset/133565>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.