Bug 110970

Summary: Add calendar table view for the new calendar picker
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109439    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Keishi Hattori 2013-02-27 04:28:17 PST
Add calendar table view for the new calendar picker
Comment 1 Keishi Hattori 2013-02-27 05:00:57 PST
Created attachment 190493 [details]
Patch
Comment 2 Kent Tamura 2013-02-27 06:45:29 PST
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.
Comment 3 Keishi Hattori 2013-02-28 06:39:39 PST
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.
Comment 4 Keishi Hattori 2013-02-28 06:41:44 PST
Created attachment 190714 [details]
Patch
Comment 5 Kent Tamura 2013-02-28 19:41:08 PST
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.
Comment 6 Kent Tamura 2013-02-28 19:41:24 PST
Comment on attachment 190714 [details]
Patch

ok
Comment 7 Keishi Hattori 2013-02-28 22:54:19 PST
Created attachment 190891 [details]
Patch
Comment 8 WebKit Review Bot 2013-02-28 23:50:32 PST
Comment on attachment 190891 [details]
Patch

Clearing flags on attachment: 190891

Committed r144423: <http://trac.webkit.org/changeset/144423>
Comment 9 WebKit Review Bot 2013-02-28 23:50:36 PST
All reviewed patches have been landed.  Closing bug.