| 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: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 29359, 60868 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Joseph Pecoraro
2011-04-28 15:49:59 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
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. (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! 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." > > 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. You can put it as an HTML comment. 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.
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. > > 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?
Hopefully I didn't step on your toes with: <http://webkit.org/b/29359>, which I see you said you've started working on. (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. 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.
Created attachment 91633 [details]
[DIFF] Between Version 2 and 3
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. > 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.
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.
(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 :-) 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.
Created attachment 91788 [details]
[DIFF] Between Version 3 and 4
> > 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?
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.
> 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! Landed in <http://trac.webkit.org/changeset/85382>. GTK Build fix (missed them!) landed in <http://trac.webkit.org/changeset/85385>. Thanks! Landed Windows expected results in <http://trac.webkit.org/changeset/85387>. I guess windows falls back to mac, and so was reporting failures. (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? (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. (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! > 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.
Created attachment 92618 [details]
[UNREVIEWED] Pixel Test Result
Landed mac pixel results in <http://trac.webkit.org/changeset/85967>. |