Bug 110970 - Add calendar table view for the new calendar picker
Summary: Add calendar table view for the new calendar picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 109439
  Show dependency tree / graph
 
Reported: 2013-02-27 04:28 PST by Keishi Hattori
Modified: 2013-02-28 23:50 PST (History)
2 users (show)

See Also:

Attachments
Patch (20.71 KB, patch)
2013-02-27 05:00 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (20.75 KB, patch)
2013-02-28 06:41 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (20.87 KB, patch)
2013-02-28 22:54 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.