Bug 83011 – Add JavaScript and CSS code for the calendar picker implementation

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

[Kent Tamura]

Add JavaScript and CSS code for the calendar picker implementation

[Kent Tamura]

Created attachment 135302 [details]
Patch

[Kent Tamura]

Created attachment 135486 [details]
Patch 2

Code cleanup

[Kent Tamura]

Hattori-san, would you do informal review for the patch please?

[Kent Tamura]

Comment on attachment 135486 [details]
Patch 2

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

> Source/WebCore/ChangeLog:8
> +        Add calendarPicker.js and calendarPicker.css, and a buld rule to

oops,
and a buld rule -> and add a build rule

> Source/WebCore/ChangeLog:9
> +        generte a C++ file. This change doesn't make any behavior change

generte -> generate

[Keishi Hattori]

Comment on attachment 135486 [details]
Patch 2

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

> Source/WebCore/Resources/calendarPicker.css:97
> +}

Is this used?

> Source/WebCore/Resources/calendarPicker.js:36
> +var Class = {

Inspector code would use something plural like ClassNames.

> Source/WebCore/Resources/calendarPicker.js:37
> +    Available: 'available',

Inspector code uses double quotes for all strings.

> Source/WebCore/Resources/calendarPicker.js:656
> + * @param {boolean} right false:left, true:right

this param doesn't exist anymore.

> Source/WebCore/Resources/calendarPicker.js:693
> +    if (this._lastY > 0 && this._x > 0)

this._lastY should be this._y
The next line too.

> Source/WebCore/Resources/calendarPicker.js:775
> +    node.classList.add(Class.Selected);

I found it strange that the selected date of the DaysTable doesn't match the node with the day-selected class.
I also think that it would be useful to show the current value inside the calendar.

Maybe there should be a selected date that is the value
and a separate highlighted date for mouse hover.

> Source/WebCore/Resources/calendarPicker.js:863
> +        } else if (dayNode.classList.contains(Class.UnavailableUp)) {

Class.UnavailableUp and Class.UnavailableDown are never added to any node.

> Source/WebCore/Resources/calendarPicker.js:930
> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_MONTH : YearMonthController.NEXT_MONTH);

Wrong names.
_moveRelatively
YearMonthController._PreviousMonth
YearMonthController._NextMonth

> Source/WebCore/Resources/calendarPicker.js:932
> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_YEAR : YearMonthController.NEXT_YEAR);

_moveRelatively
YearMonthController._NextYear
YearMonthController._PreviousYear

> Source/WebCore/Resources/calendarPicker.js:934
> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_10YEARS : YearMonthController.NEXT_10YEARS);

_moveRelatively
YearMonthController._PreviousTenYears
YearMonthController._NextTenYears

> ManualTests/forms/calendar-picker.html:53
> +var japaneseArguments = {

Is this used?

[Kent Tamura]

Comment on attachment 135486 [details]
Patch 2

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

Thank you for reviewing the long code.

>> Source/WebCore/Resources/calendarPicker.css:97
>> +}
> 
> Is this used?

Deleted.

>> Source/WebCore/Resources/calendarPicker.js:36
>> +var Class = {
> 
> Inspector code would use something plural like ClassNames.

Renamed to ClassNames.

>> Source/WebCore/Resources/calendarPicker.js:37
>> +    Available: 'available',
> 
> Inspector code uses double quotes for all strings.

Replaced ' with ".

>> Source/WebCore/Resources/calendarPicker.js:656
>> + * @param {boolean} right false:left, true:right
> 
> this param doesn't exist anymore.

Right.  Deleted.

>> Source/WebCore/Resources/calendarPicker.js:693
>> +    if (this._lastY > 0 && this._x > 0)
> 
> this._lastY should be this._y
> The next line too.

Fixed.

>> Source/WebCore/Resources/calendarPicker.js:775
>> +    node.classList.add(Class.Selected);
> 
> I found it strange that the selected date of the DaysTable doesn't match the node with the day-selected class.
> I also think that it would be useful to show the current value inside the calendar.
> 
> Maybe there should be a selected date that is the value
> and a separate highlighted date for mouse hover.

I don't like to change the UI behavior at this moment because Chrome UX/UI team agreed about the current behavior.
Let's change the behavior if we have a lot of complain.

>> Source/WebCore/Resources/calendarPicker.js:863
>> +        } else if (dayNode.classList.contains(Class.UnavailableUp)) {
> 
> Class.UnavailableUp and Class.UnavailableDown are never added to any node.

Removed related code.

>> Source/WebCore/Resources/calendarPicker.js:930
>> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_MONTH : YearMonthController.NEXT_MONTH);
> 
> Wrong names.
> _moveRelatively
> YearMonthController._PreviousMonth
> YearMonthController._NextMonth

Fixed. They should be public.

>> Source/WebCore/Resources/calendarPicker.js:932
>> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_YEAR : YearMonthController.NEXT_YEAR);
> 
> _moveRelatively
> YearMonthController._NextYear
> YearMonthController._PreviousYear

ditto.

>> Source/WebCore/Resources/calendarPicker.js:934
>> +        global.yearMonthController.moveRelatively(event.shiftKey ? YearMonthController.PREVIOUS_10YEARS : YearMonthController.NEXT_10YEARS);
> 
> _moveRelatively
> YearMonthController._PreviousTenYears
> YearMonthController._NextTenYears

ditto.

>> ManualTests/forms/calendar-picker.html:53
>> +var japaneseArguments = {
> 
> Is this used?

This is not used, but it's useful for l10n test. I'd like to keep this.

[Kent Tamura]

Created attachment 135750 [details]
Patch 3

Addressed the comments.

[Keishi Hattori]

Comment on attachment 135750 [details]
Patch 3

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

Also I noticed that you can't use up/down arrow keys inside of the month popup.

> Source/WebCore/Resources/calendarPicker.js:206
> +    return yearString + "-" + ("0" + (month + 1)).substr(-2, 2) + "-" + ("0" + day).substr(-2, 2)

Missing semicolon.

> Source/WebCore/Resources/calendarPicker.js:390
> +    this._wall.addEventListener("click", bind(this._closePopup, this), false);

Maybe you can use blur event to remove this wall element?
this._monthPopup.addEventListener("blur", bind(this._closePopup, this), false);

[Kent Tamura]

Created attachment 135762 [details]
Patch 4

Keyboard operation for the month popup

[Kent Tamura]

(In reply to comment #8)
> (From update of attachment 135750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135750&action=review
> 
> Also I noticed that you can't use up/down arrow keys inside of the month popup.

Fixed.

> > Source/WebCore/Resources/calendarPicker.js:206
> > +    return yearString + "-" + ("0" + (month + 1)).substr(-2, 2) + "-" + ("0" + day).substr(-2, 2)
> 
> Missing semicolon.

Fixed.

> > Source/WebCore/Resources/calendarPicker.js:390
> > +    this._wall.addEventListener("click", bind(this._closePopup, this), false);
> 
> Maybe you can use blur event to remove this wall element?
> this._monthPopup.addEventListener("blur", bind(this._closePopup, this), false);

It's not sufficient.  I'd like to disallow DaysArea hover and click actions during opening the month popup.

[Keishi Hattori]

I think its ok now.

[Hajime Morrita]

Comment on attachment 135762 [details]
Patch 4

kinda stamping. Let us land this and see how it works.

[Kent Tamura]

Thank you!!

[Kent Tamura]

Committed r113298: <http://trac.webkit.org/changeset/113298>