Bug 83872 – Show the format indicator in a date field

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

[Kent Tamura]

Show the format indicator in a date field

[Kent Tamura]

Created attachment 137066 [details]
WIP

[Kent Tamura]

I'll split the patch into multiple.

[Kent Tamura]

Created attachment 137851 [details]
Patch

[Hajime Morrita]

Comment on attachment 137851 [details]
Patch

I hope this to be tested in non-pixel way somehow.
For example, we can access the dumped render treee using Internals.elementRenderTreeAsText(),
which allows us to inspect the render tree programatically.
Or maybe we can simply use add a reftest.

[Ryosuke Niwa]

Comment on attachment 137851 [details]
Patch

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

> Source/WebCore/platform/text/LocalizedDate.h:49
> +// Localized date format string such as "month/day/year".
> +String dateFormatText();

Instead of adding a comment, can we simply name this function localizedDateFormatString?

> Source/WebCore/platform/text/LocalizedDateICU.cpp:138
> +            if (lastChar == 'y' || lastChar == 'Y') {

Can we add a comment saying that these Y, M, d, etc... follow ICU convention?
Or maybe we can define inline helper functions like isICUYearSymbol (We probably prefer the latter approach).

> Source/WebCore/platform/text/LocalizedDateICU.cpp:140
> +                builder.append(text.isEmpty() ? "year" : text);

Maybe we should capitalize these?

> LayoutTests/fast/forms/date/date-appearance.html:21
> +<div><input type="date" value=""></div>

Can we use a ref-test here?

[Kent Tamura]

Created attachment 138315 [details]
Patch 2

[Kent Tamura]

Comment on attachment 137851 [details]
Patch

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

>> Source/WebCore/platform/text/LocalizedDate.h:49
>> +String dateFormatText();
> 
> Instead of adding a comment, can we simply name this function localizedDateFormatString?

ok, I renamed it.

>> Source/WebCore/platform/text/LocalizedDateICU.cpp:138
>> +            if (lastChar == 'y' || lastChar == 'Y') {
> 
> Can we add a comment saying that these Y, M, d, etc... follow ICU convention?
> Or maybe we can define inline helper functions like isICUYearSymbol (We probably prefer the latter approach).

I inroduced isICUFooSymbol functions.

>> Source/WebCore/platform/text/LocalizedDateICU.cpp:140
>> +                builder.append(text.isEmpty() ? "year" : text);
> 
> Maybe we should capitalize these?

Done.

>> LayoutTests/fast/forms/date/date-appearance.html:21
>> +<div><input type="date" value=""></div>
> 
> Can we use a ref-test here?

I made a text test using window.internals.visiblePlaceholder().

[Hajime Morrita]

Comment on attachment 138315 [details]
Patch 2

Looks better now.

[WebKit Review Bot]

Comment on attachment 138315 [details]
Patch 2

Rejecting attachment 138315 [details] from commit-queue.

New failing tests:
fast/forms/date/date-appearance.html
Full output: http://queues.webkit.org/results/12528070

[WebKit Review Bot]

Created attachment 138455 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Created attachment 138491 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138491 [details]
Patch for landing

Clearing flags on attachment: 138491

Committed r114999: <http://trac.webkit.org/changeset/114999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.