Bug 110970 – Add calendar table view for the new calendar picker

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Add calendar table view for the new calendar picker

[Keishi Hattori]

Created attachment 190493 [details]
Patch

[Kent Tamura]

Comment on attachment 190493 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1901
> +DayCell.Height = 20;

Is it enough for touch input devices?  We use 28px in the pointer:coarse case for now.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:2069
> +        if (global.params.locale === "ja") {

global.params.locale should be getLanguage().
Or, this should check if the region part is "jp" because the rule of blue Saturdays and red Sundays is a common sense in Japan, not Japanese language. (But Google Chrome might provide no "jp" part.)

> Source/WebCore/Resources/pagepopups/calendarPicker.js:2204
> +    // Disable mouse wheel scrolling.

why?
We prefer "why" comments to "what" comments.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:2329
> + * @return {!Object}

should explain the content of Object.

[Keishi Hattori]

Comment on attachment 190493 [details]
Patch

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

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1901
>> +DayCell.Height = 20;
> 
> Is it enough for touch input devices?  We use 28px in the pointer:coarse case for now.

Can I add touch support in another patch? I'll have to prepare a patch so the ScrollView supports scrolling with touch events so you can scroll in the month popup anyway.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:2069
>> +        if (global.params.locale === "ja") {
> 
> global.params.locale should be getLanguage().
> Or, this should check if the region part is "jp" because the rule of blue Saturdays and red Sundays is a common sense in Japan, not Japanese language. (But Google Chrome might provide no "jp" part.)

I confirmed that Chrome on Mac only provides "ja" for Japanese. I'll use getLanguage.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:2204
>> +    // Disable mouse wheel scrolling.
> 
> why?
> We prefer "why" comments to "what" comments.

Done.

>> Source/WebCore/Resources/pagepopups/calendarPicker.js:2329
>> + * @return {!Object}
> 
> should explain the content of Object.

Done.

[Keishi Hattori]

Created attachment 190714 [details]
Patch

[Kent Tamura]

Comment on attachment 190493 [details]
Patch

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

>>> Source/WebCore/Resources/pagepopups/calendarPicker.js:1901
>>> +DayCell.Height = 20;
>> 
>> Is it enough for touch input devices?  We use 28px in the pointer:coarse case for now.
> 
> Can I add touch support in another patch? I'll have to prepare a patch so the ScrollView supports scrolling with touch events so you can scroll in the month popup anyway.

Please add a FIXEM comment.

[Kent Tamura]

Comment on attachment 190714 [details]
Patch

ok

[Keishi Hattori]

Created attachment 190891 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190891 [details]
Patch

Clearing flags on attachment: 190891

Committed r144423: <http://trac.webkit.org/changeset/144423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.