[webkit-reviews] review denied: [Bug 92678] Month-year selector on calendar picker should be touch friendly. : [Attachment 155362] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kevin Ellis <kevers at chromium.org>'s
request for review:
Bug 92678: Month-year selector on calendar picker should be touch friendly.
https://bugs.webkit.org/show_bug.cgi?id=92678

Attachment 155362: Patch
https://bugs.webkit.org/attachment.cgi?id=155362&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155362&action=review


> Source/WebCore/Resources/calendarPicker.js:578
> +	   var option = createElement("div",
ClassNames.MonthSelectorPopupEntry, formatYearMonth(Math.floor(m / 12), m %
12));
> +	   option.setAttribute('value', String(Math.floor(m / 12)) + "-" +
String(m % 12));
> +	   this._monthPopupContents.appendChild(option);
>	   if (m == current)
> -	       option.selected = true;
> +	       option.setAttribute('selected', true); 

<div> doesn't have 'value' and 'selected' attributes.  Using non-standard
attributes is not a good practice.
Please use data-* attributes and/or element.dataset.

> Source/WebCore/Resources/calendarPicker.js:-576
> -    this._monthPopup.size = Math.max(4, Math.min(10,
this._monthPopup.length));

Does the popup work fine with <input type=date max="2012-07-31">? I'm afraid
the popup has unnecessary whitespace at the bottom.

> Source/WebCore/Resources/calendarPicker.js:604
>  
> +YearMonthController.prototype._getSelection = function()

Please add a type annotation for the return value.

> Source/WebCore/Resources/calendarPicker.js:616
> +	   // move trigged during a scroll from resetting the selection. 
Automatically

We should add only one space after '.'

> Source/WebCore/Resources/calendarPicker.js:684
> +    if (!selection)
> +	 return;

wrong indentation