Bug 59752

Summary:

Allow Localized Date Strings for Date Input Fields

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ddkilzer, joepeck, tkent, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

29359, 60868

Attachments:

Description	Flags
[PATCH] Add LocalizedDate and Default Implementation	none
[PATCH] Add LocalizedDate Allow Formatting + Parsing	tkent: review+
[PATCH] Add LocalizedDate Allow Formatting + Parsing	tkent: review-
[DIFF] Between Version 2 and 3	none
[PATCH] Add LocalizedDate Allow Formatting + Parsing	tkent: review+, tkent: commit-queue-
[DIFF] Between Version 3 and 4	none
[UNREVIEWED] Pixel Test Result	none

Joseph Pecoraro

Reported 2011-04-28 15:49:59 PDT

Allow localized date strings as the visible value for date input fields. This would be similar to the localized numbers for <input type="number">.

Attachments
[PATCH] Add LocalizedDate and Default Implementation (27.83 KB, patch) 2011-04-28 16:06 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[PATCH] Add LocalizedDate Allow Formatting + Parsing (37.18 KB, patch) 2011-04-28 18:49 PDT, Joseph Pecoraro	tkent: review+	Details Formatted Diff Diff
[PATCH] Add LocalizedDate Allow Formatting + Parsing (38.64 KB, patch) 2011-04-28 21:17 PDT, Joseph Pecoraro	tkent: review-	Details Formatted Diff Diff
[DIFF] Between Version 2 and 3 (3.48 KB, patch) 2011-04-28 21:18 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[PATCH] Add LocalizedDate Allow Formatting + Parsing (43.94 KB, patch) 2011-04-29 18:34 PDT, Joseph Pecoraro	tkent: review+ tkent: commit-queue-	Details Formatted Diff Diff
[DIFF] Between Version 3 and 4 (3.37 KB, patch) 2011-04-29 18:35 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[UNREVIEWED] Pixel Test Result (17.95 KB, patch) 2011-05-06 12:03 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2011-04-28 16:06:26 PDT

Created attachment 91579 [details] [PATCH] Add LocalizedDate and Default Implementation This patch: • moves DateComponents into WebCore/platform. Doesn't seem like it has any dependencies outside of platform. And its very useful. • adds LocalizedDate.h and LocalizedDateNone.cpp much like number. • adds the files to the build • calls out to the localization as appropriate

Kent Tamura

Comment 2 2011-04-28 16:42:06 PDT

Comment on attachment 91579 [details] [PATCH] Add LocalizedDate and Default Implementation View in context: https://bugs.webkit.org/attachment.cgi?id=91579&action=review > LayoutTests/fast/forms/date-input-visible-strings.html:1 > +<!DOCTYPE html> Please add sentences describing the expected result. e.g. "the user-visible values of the input fields should be localized if the platform has a LocalizedDate implementation. Otherwise, they should be in the HTML5 formats." > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22989 > F3810C1E1365A4D400ED6E33 /* WorkerContextInspectorProxy.h in Headers */, > + A5732B0B136A161D005C8D7C /* DateComponents.h in Headers */, > + A5732B0D136A16C4005C8D7C /* LocalizedDate.h in Headers */, nit: They should be inserted to sorted positions. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25688 > 781755861365A1B00093BE2E /* DataTransferItems.cpp in Sources */, > + A5732B0A136A161D005C8D7C /* DateComponents.cpp in Sources */, > + A5732B0F136A1715005C8D7C /* LocalizedDateNone.cpp in Sources */, ditto. > Source/WebCore/platform/text/LocalizedDate.h:37 > +String formatLocalizedDate(const DateComponents& dateComponents); This only has a format function. What about parsing? Showing a localized string but accepting no localized string is not good.

Joseph Pecoraro

Comment 3 2011-04-28 17:14:01 PDT

(In reply to comment #2) > (From update of attachment 91579 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91579&action=review > > > LayoutTests/fast/forms/date-input-visible-strings.html:1 > > +<!DOCTYPE html> > > Please add sentences describing the expected result. e.g. "the user-visible values of the input fields should be localized if the platform has a LocalizedDate implementation. Otherwise, they should be in the HTML5 formats." That sounds fine. I hate putting text in non-text dump render tree tests, but this would make looking at the test in your browser much clearer. > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22989 > > F3810C1E1365A4D400ED6E33 /* WorkerContextInspectorProxy.h in Headers */, > > + A5732B0B136A161D005C8D7C /* DateComponents.h in Headers */, > > + A5732B0D136A16C4005C8D7C /* LocalizedDate.h in Headers */, > > nit: They should be inserted to sorted positions. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25688 > > 781755861365A1B00093BE2E /* DataTransferItems.cpp in Sources */, > > + A5732B0A136A161D005C8D7C /* DateComponents.cpp in Sources */, > > + A5732B0F136A1715005C8D7C /* LocalizedDateNone.cpp in Sources */, > > ditto. I made sure the files are sorted in the actual Xcode groups. The Xcode project file is weird. > > Source/WebCore/platform/text/LocalizedDate.h:37 > > +String formatLocalizedDate(const DateComponents& dateComponents); > > This only has a format function. What about parsing? > Showing a localized string but accepting no localized string is not good. I see what you mean. I suppose I'll add a function for that as well. Thanks!

Kent Tamura

Comment 4 2011-04-28 17:15:28 PDT

Joseph Pecoraro

Comment 5 2011-04-28 18:49:18 PDT

Created attachment 91616 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing Take 2. Allow for formatting localized dates and parsing incoming localized dates. I pass the date type in for parsing because an implementation may want to parse the input differently depending on if this is for a "time" or a "date" type. Or, that can be ignored if a port wants.

Kent Tamura

Comment 6 2011-04-28 18:56:01 PDT

Comment on attachment 91616 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing View in context: https://bugs.webkit.org/attachment.cgi?id=91616&action=review > Source/WebCore/platform/text/LocalizedDate.h:38 > +// Parses a string representation of a date string localized > +// for the browser's current locale for a particular date type. > +// If the input string is not valid or an implementation doesn't > +// support localized dates, this function returns an empty string. > +String parseLocalizedDate(const String&, DateComponents::Type); Should add a comment about what should be returned for valid inputs.

Joseph Pecoraro

Comment 7 2011-04-28 19:08:28 PDT

> > Source/WebCore/platform/text/LocalizedDate.h:38 > > +// Parses a string representation of a date string localized > > +// for the browser's current locale for a particular date type. > > +// If the input string is not valid or an implementation doesn't > > +// support localized dates, this function returns an empty string. > > +String parseLocalizedDate(const String&, DateComponents::Type); > > Should add a comment about what should be returned for valid inputs. Arg. This is a good point. And raises an issue. Currently I added the following: // If the input string is valid this function returns a valid // HTML5 date string for the specified type. HTML5 date strings // are a subset of ISO-8601. However, I could instead have the function return a DateComponents object, That would match the format function. Or a double value, representing the milliseconds since the 1970 epoch (which can then be serialized pretty easily by BaseDateTimeInputType. Do you have a preference for once of the other types?

Joseph Pecoraro

Comment 8 2011-04-28 19:10:14 PDT

Hopefully I didn't step on your toes with: <http://webkit.org/b/29359>, which I see you said you've started working on.

Kent Tamura

Comment 9 2011-04-28 19:19:02 PDT

(In reply to comment #7) > Arg. This is a good point. And raises an issue. Currently I added the following: > > // If the input string is valid this function returns a valid > // HTML5 date string for the specified type. HTML5 date strings > // are a subset of ISO-8601. > > However, I could instead have the function return a DateComponents object, > That would match the format function. Or a double value, representing > the milliseconds since the 1970 epoch (which can then be serialized > pretty easily by BaseDateTimeInputType. Do you have a preference for > once of the other types? Probably the double value is simpler. (In reply to comment #8) > Hopefully I didn't step on your toes with: <http://webkit.org/b/29359>, > which I see you said you've started working on. I'm working on webkit.org/b/53961 and it won't cover the text field localization. This patch is very helpful. Thank you.

Joseph Pecoraro

Comment 10 2011-04-28 21:17:29 PDT

Created attachment 91632 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing This switches to return a double from the parseLocalizedDate. Generalize serialization a bit so that we can serialize from a DateComponent that we make (always from msec) instead of using serialize(double) which interprets its argument differently for <input type="month">. I'll follow up and attach just a patch of the diff between this patch and the last, because this is getting large but there were relatively few changes.

Joseph Pecoraro

Comment 11 2011-04-28 21:18:15 PDT

Created attachment 91633 [details] [DIFF] Between Version 2 and 3

Kent Tamura

Comment 12 2011-04-29 15:58:53 PDT

Comment on attachment 91632 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing View in context: https://bugs.webkit.org/attachment.cgi?id=91632&action=review > Source/WebCore/ChangeLog:27 > + (WebCore::BaseDateAndTimeInputType::serialize): > + (WebCore::BaseDateAndTimeInputType::serializeWithComponents): > + serializing for a double value may be different for different > + types. The month type assumes it is monthes instead of msecs > + like the others. So provide a more general serialization > + function that will always serialize a string correctly for > + the current type but from a DateComponents object. Useful > + when we create our own DateComponents object from the msec > + value returned from a LocalizedDate implementation. I don't think this change is needed. BaseDateAndTimeInputType::serialize(double) already handle the month type correctly. > Source/WebCore/html/BaseDateAndTimeInputType.cpp:208 > + if (!date.setMillisecondsSinceEpochForDateTime(parsedValue)) This is wrong. It sets DateTime type to the DateComponents object. I think serialize(parsedValue) is enough.

Joseph Pecoraro

Comment 13 2011-04-29 16:28:36 PDT

> This is wrong. It sets DateTime type to the DateComponents object. > I think serialize(parsedValue) is enough. Hmm, I can try this again. But I thought that serialize(parsedValue) for month, falls back to: DateComponents::setMonthsSinceEpoch(double months) Which is months, not msec, despite its name. Maybe the implementation of setMonthsSinceEpoch should actually take msec. However this was yesterday. Let me reverify this today.

Joseph Pecoraro

Comment 14 2011-04-29 16:33:49 PDT

Hmm, this: bool MonthInputType::setMillisecondToDateComponents(double value, DateComponents* date) const { ASSERT(date); return date->setMonthsSinceEpoch(value); } Should probably be using: return date->setMillisecondsSinceEpochForMonth(value) If that is the case I'll file a separate bug, fix that first, and come back to this with the direct serialize approach.

Kent Tamura

Comment 15 2011-04-29 16:55:14 PDT

(In reply to comment #13) > Which is months, not msec, despite its name. Maybe the > implementation of setMonthsSinceEpoch should actually > take msec. Ah, I understand your intention. Yes, Month of DateComponents is an exception for a double value, and LocalizedDate always uses milliseconds. What we need is similar to setValueAsDate(parsedValue, ec). The behavior of MonthInputType::setMillisecondToDateComponents() should be correct because HTMLInputElement and MonthInputType treats double values as the number of months though the name is wrong :-)

Joseph Pecoraro

Comment 16 2011-04-29 18:34:58 PDT

Created attachment 91787 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing This keeps serializeWithComponents and adds serializeWithMilliseconds, which does the correct DateComponents construction. Diff between this and the last to follow.

Joseph Pecoraro

Comment 17 2011-04-29 18:35:21 PDT

Created attachment 91788 [details] [DIFF] Between Version 3 and 4

Joseph Pecoraro

Comment 18 2011-04-29 18:36:22 PDT

> > Source/WebCore/html/BaseDateAndTimeInputType.cpp:208 > > + if (!date.setMillisecondsSinceEpochForDateTime(parsedValue)) > > This is wrong. It sets DateTime type to the DateComponents object. > I think serialize(parsedValue) is enough. Yah, I was taking a shortcut I clearly should not have. How does this new patch look?

Kent Tamura

Comment 19 2011-04-29 18:43:56 PDT

Comment on attachment 91787 [details] [PATCH] Add LocalizedDate Allow Formatting + Parsing ok. We can use serializeWithMilliseconds() in BaseDateAndTimeInputType::setValueAsDate() and can remove MonthInputType::setValueAsDate(). Please rebase and run sort-Xcode-project-file before landing.

Joseph Pecoraro

Comment 20 2011-04-29 19:12:27 PDT

> We can use serializeWithMilliseconds() in BaseDateAndTimeInputType::setValueAsDate() > and can remove MonthInputType::setValueAsDate(). Good point! Changed base and removed month's (header + implementation). > Please rebase and run sort-Xcode-project-file before landing. Ahh, I see what you were saying before about the sorting. I did not know about this script. In the past when I moved items around I though that Xcode just reordered them back anyways. Done. Thanks!

Joseph Pecoraro

Comment 21 2011-04-29 20:29:23 PDT

Landed in <http://trac.webkit.org/changeset/85382>. GTK Build fix (missed them!) landed in <http://trac.webkit.org/changeset/85385>. Thanks!

Joseph Pecoraro

Comment 22 2011-04-29 21:19:19 PDT

Landed Windows expected results in <http://trac.webkit.org/changeset/85387>. I guess windows falls back to mac, and so was reporting failures.

Tony Chang

Comment 23 2011-05-06 10:26:46 PDT

(In reply to comment #22) > Landed Windows expected results in <http://trac.webkit.org/changeset/85387>. > I guess windows falls back to mac, and so was reporting failures. fast/forms/date-input-visible-strings.html is missing pixel results. Can someone add them for mac?

Joseph Pecoraro

Comment 24 2011-05-06 10:32:22 PDT

(In reply to comment #23) > (In reply to comment #22) > > Landed Windows expected results in <http://trac.webkit.org/changeset/85387>. > > I guess windows falls back to mac, and so was reporting failures. > > fast/forms/date-input-visible-strings.html is missing pixel results. Can someone add them for mac? I'm sorry. Are pixel results necessary for every non-dump-as-text test? Here it was simply sufficient to see the text in the DRT dump. I suppose it can't hurt to also include pixel results though. I'll make mac pixel results and upload a patch here.

Tony Chang

Comment 25 2011-05-06 11:05:14 PDT

(In reply to comment #24) > (In reply to comment #23) > > fast/forms/date-input-visible-strings.html is missing pixel results. Can someone add them for mac? > > I'm sorry. Are pixel results necessary for every non-dump-as-text test? Yes. > Here it was simply sufficient to see the text in the DRT dump. I suppose > it can't hurt to also include pixel results though. I'll make mac pixel > results and upload a patch here. If you only care about the text, you should add a call to layoutTestController.dumpAsText(). Thanks!

Joseph Pecoraro

Comment 26 2011-05-06 12:02:42 PDT

> If you only care about the text, you should add a call to layoutTestController.dumpAsText(). Well, the issue there is I want the value dumped by the renderer, which would be unavailable from JavaScript unless I added plumbing to DRT / LayoutTestController etc. I'll attach a patch I will land unreviewed once the rebase is done.

Joseph Pecoraro

Comment 27 2011-05-06 12:03:16 PDT

Created attachment 92618 [details] [UNREVIEWED] Pixel Test Result

Joseph Pecoraro

Comment 28 2011-05-06 12:12:58 PDT

Landed mac pixel results in <http://trac.webkit.org/changeset/85967>.

Note You need to log in before you can comment on or make changes to this bug.