Bug 92678 – Month-year selector on calendar picker should be touch friendly.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kevin Ellis]

Month-year selector on calendar picker should be touch friendly.

[Kevin Ellis]

Created attachment 155362 [details]
Patch

[Kent Tamura]

Comment on attachment 155362 [details]
Patch

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

[Kevin Ellis]

Created attachment 155527 [details]
Patch

[Kevin Ellis]

Comment on attachment 155362 [details]
Patch

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

>> Source/WebCore/Resources/calendarPicker.js:578
>> +            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.

Switched to dataset.value and a class name to mark the selected item.

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

The CSS rules use max-height to avoid unnecessary whitespace; however, there was a problem with scroll positioning and extra white-space at the side when a scroll-bar is not required.

>> Source/WebCore/Resources/calendarPicker.js:604
>> +YearMonthController.prototype._getSelection = function()
> 
> Please add a type annotation for the return value.

Done.

>> Source/WebCore/Resources/calendarPicker.js:616
>> +        // move trigged during a scroll from resetting the selection.  Automatically
> 
> We should add only one space after '.'

Done.

>> Source/WebCore/Resources/calendarPicker.js:684
>> +      return;
> 
> wrong indentation

Fixed.

[Kent Tamura]

Comment on attachment 155527 [details]
Patch

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

Almost ok.

> Source/WebCore/ChangeLog:31
> +        * Resources/calendarPicker.css:
> +        (.month-selector-popup):
> +        (.month-selector-popup-contents):
> +        (.month-selector-popup-entry):
> +        (.month-selector-popup-entry[selected=true]):
> +        (@media (pointer:coarse)):
> +        * Resources/calendarPicker.js:
> +        (YearMonthController.prototype.attachTo):
> +        (YearMonthController.prototype._redraw):
> +        (YearMonthController.prototype._showPopup):
> +        (YearMonthController.prototype._closePopup):
> +        (YearMonthController.prototype._getSelection):
> +        (YearMonthController.prototype._handleMouseMove):
> +        (YearMonthController.prototype._handleMonthPopupKey):
> +        (YearMonthController.prototype._handleYearMonthChange):

Please update the list, and add comments for each of entries.

> Source/WebCore/Resources/calendarPicker.js:443
> +    this._monthPopup.setAttribute('tabindex', 0);

nit: this._monthPopup.tabIndex = 0 is simpler and consistent with other part.

> Source/WebCore/Resources/calendarPicker.js:601
> +        this._monthPopup.style.setProperty('-webkit-padding-end', '15px');

nit: ...style.webkitPaddingEnd = '15px' is simpler and consistent with others.

[Kevin Ellis]

Created attachment 155809 [details]
Patch

[Kevin Ellis]

Comment on attachment 155527 [details]
Patch

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

>> Source/WebCore/ChangeLog:31
>> +        (YearMonthController.prototype._handleYearMonthChange):
> 
> Please update the list, and add comments for each of entries.

Done.

>> Source/WebCore/Resources/calendarPicker.js:443
>> +    this._monthPopup.setAttribute('tabindex', 0);
> 
> nit: this._monthPopup.tabIndex = 0 is simpler and consistent with other part.

Done.

>> Source/WebCore/Resources/calendarPicker.js:601
>> +        this._monthPopup.style.setProperty('-webkit-padding-end', '15px');
> 
> nit: ...style.webkitPaddingEnd = '15px' is simpler and consistent with others.

Done.

[WebKit Review Bot]

Comment on attachment 155527 [details]
Patch

Cleared review? from obsolete attachment 155527 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Kent Tamura]

Comment on attachment 155809 [details]
Patch

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

> Source/WebCore/ChangeLog:30
> +        Summary of changes:
> +          - Changed selector popup from a list-box to an absolute positined div
> +            to enable better CSS control over appearance.
> +          - Added styles for menu content wrapper and month-year entries
> +            to layout as a scrollable table within the popup window. 
> +          - Add style to highlight selected month-year.
> +          - Added rules to enlarge entries in the popup menu on devices that
> +            support touch.
> +          - Update attachTo to create popup menu and content wrapper.
> +          - Update redraw to populate table in popup rather than listbox.
> +          - Update showPopup to set the scroll position and add padding to 
> +            accomodate a scrollbar as required.
> +          - Update closePopup to restore focus to the calendar.
> +          - Add support for keyboard navigation in handleMonthPopupKey.
> +          - Added handleMouseMove to update the selected month-year on hover.
> +          - Update handleYearMonthChange to retrieve the value from the selected
> +            month-year.

Such comments should be put to the file/function list below.

[Kevin Ellis]

Created attachment 156096 [details]
Patch

[Kevin Ellis]

Comment on attachment 155809 [details]
Patch

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

>> Source/WebCore/ChangeLog:30
>> +            month-year.
> 
> Such comments should be put to the file/function list below.

Done.

[WebKit Review Bot]

Comment on attachment 156096 [details]
Patch

Clearing flags on attachment: 156096

Committed r124473: <http://trac.webkit.org/changeset/124473>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.