Bug 83011

Created attachment 135302 [details] Patch

Created attachment 135486 [details] Patch 2 Code cleanup

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

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

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?

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.

Created attachment 135750 [details] Patch 3 Addressed the comments.

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);

Created attachment 135762 [details] Patch 4 Keyboard operation for the month popup

(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.

I think its ok now.

Comment on attachment 135762 [details] Patch 4 kinda stamping. Let us land this and see how it works.

Thank you!!

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