Bug 83011 - Add JavaScript and CSS code for the calendar picker implementation
: Add JavaScript and CSS code for the calendar picker implementation
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 83001
: 53961
  Show dependency treegraph
 
Reported: 2012-04-03 02:33 PST by
Modified: 2012-04-05 00:31 PST (History)


Attachments
Patch (43.47 KB, patch)
2012-04-03 03:53 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2 (44.15 KB, patch)
2012-04-03 19:09 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3 (44.93 KB, patch)
2012-04-04 20:27 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch 4 (45.49 KB, patch)
2012-04-04 23:22 PST, Kent Tamura
morrita: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-04-03 02:33:07 PST
Add JavaScript and CSS code for the calendar picker implementation
------- Comment #1 From 2012-04-03 03:53:40 PST -------
Created an attachment (id=135302) [details]
Patch
------- Comment #2 From 2012-04-03 19:09:46 PST -------
Created an attachment (id=135486) [details]
Patch 2

Code cleanup
------- Comment #3 From 2012-04-03 19:16:16 PST -------
Hattori-san, would you do informal review for the patch please?
------- Comment #4 From 2012-04-03 19:19:15 PST -------
(From update of attachment 135486 [details])
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
------- Comment #5 From 2012-04-04 03:49:42 PST -------
(From update of attachment 135486 [details])
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?
------- Comment #6 From 2012-04-04 20:21:46 PST -------
(From update of attachment 135486 [details])
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.
------- Comment #7 From 2012-04-04 20:27:21 PST -------
Created an attachment (id=135750) [details]
Patch 3

Addressed the comments.
------- Comment #8 From 2012-04-04 21:37:15 PST -------
(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.

> 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);
------- Comment #9 From 2012-04-04 23:22:54 PST -------
Created an attachment (id=135762) [details]
Patch 4

Keyboard operation for the month popup
------- Comment #10 From 2012-04-04 23:24:28 PST -------
(In reply to comment #8)
> (From update of attachment 135750 [details] [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.
------- Comment #11 From 2012-04-05 00:01:11 PST -------
I think its ok now.
------- Comment #12 From 2012-04-05 00:26:58 PST -------
(From update of attachment 135762 [details])
kinda stamping. Let us land this and see how it works.
------- Comment #13 From 2012-04-05 00:27:56 PST -------
Thank you!!
------- Comment #14 From 2012-04-05 00:31:06 PST -------
Committed r113298: <http://trac.webkit.org/changeset/113298>