Bug 92678 - Month-year selector on calendar picker should be touch friendly.
Summary: Month-year selector on calendar picker should be touch friendly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kevin Ellis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-30 14:17 PDT by Kevin Ellis
Modified: 2012-08-02 10:02 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.73 KB, patch)
2012-07-30 14:22 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2012-07-31 07:23 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2012-08-01 07:47 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (11.76 KB, patch)
2012-08-02 08:37 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ellis 2012-07-30 14:17:09 PDT
Month-year selector on calendar picker should be touch friendly.
Comment 1 Kevin Ellis 2012-07-30 14:22:38 PDT
Created attachment 155362 [details]
Patch
Comment 2 Kent Tamura 2012-07-30 18:51:51 PDT
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
Comment 3 Kevin Ellis 2012-07-31 07:23:09 PDT
Created attachment 155527 [details]
Patch
Comment 4 Kevin Ellis 2012-07-31 07:27:30 PDT
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.
Comment 5 Kent Tamura 2012-07-31 20:21:45 PDT
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.
Comment 6 Kevin Ellis 2012-08-01 07:47:50 PDT
Created attachment 155809 [details]
Patch
Comment 7 Kevin Ellis 2012-08-01 07:49:13 PDT
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.
Comment 8 WebKit Review Bot 2012-08-01 11:21:39 PDT
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).
Comment 9 Kent Tamura 2012-08-01 16:06:10 PDT
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.
Comment 10 Kevin Ellis 2012-08-02 08:37:26 PDT
Created attachment 156096 [details]
Patch
Comment 11 Kevin Ellis 2012-08-02 08:38:31 PDT
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.
Comment 12 WebKit Review Bot 2012-08-02 10:02:16 PDT
Comment on attachment 156096 [details]
Patch

Clearing flags on attachment: 156096

Committed r124473: <http://trac.webkit.org/changeset/124473>
Comment 13 WebKit Review Bot 2012-08-02 10:02:20 PDT
All reviewed patches have been landed.  Closing bug.