Summary: | [Forms] Multiple fields month input UI | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, gyuyoung.kim, macpherson, menard, mifenton, rakuco, tkent, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 97300, 97303, 97327, 97521, 97633, 97640, 97757, 97863, 97864, 98227, 99704, 102046 | ||||||||||||||||||
Bug Blocks: | 29359, 97888 | ||||||||||||||||||
Attachments: |
|
Description
yosin
2012-09-20 23:50:01 PDT
Created attachment 165731 [details]
Proof of Concept
Created attachment 165960 [details]
Patch 1
Created attachment 165963 [details]
Patch 2
Comment on attachment 165963 [details]
Patch 2
Could you review this patch?
Thanks in advance.
Attachment 165963 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-android/TestExpectations:1: No port uses path LayoutTests/platform/chromium-android/TestExpectations for test_expectations [test/expectations] [5]
Total errors found: 1 in 29 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 165963 [details] Patch 2 Attachment 165963 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14027863 Comment on attachment 165963 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=165963&action=review Let's split. 1. Adding placeholder feature to DateTimeNumericElement, and update its existing subclasses. 2. Adding DateTimeMonthFieldElement and DateTimeYearFieldElement 3. Remaining C++/CSS changes 4. Tests > Source/WebCore/html/shadow/DateTimeEditElement.cpp:71 > + const String& m_placeholderForMonth; > + const String& m_placeholderForYear; Let's use "const String" instead of "const String&". I understand "const String&" works in this case, but it is not robust for the future changes. Copying String objects is not expensive. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:127 > + case DateTimeFormat::FieldTypeMonth: > + m_editElement.addField(DateTimeMonthFieldElement::create(document, m_editElement, m_placeholderForMonth)); This code seems to support only M and MM. Don't you support MMM, MMMM, MMMMM ? > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:351 > +static const int minimumYear = 1; > +// Date in ECMAScript can't represent dates later than 275760-09-13T00:00Z. > +// So, we have the same upper limit in HTML5 dates. > +static const int maximumYear = 275760; > + > +DateTimeYearFieldElement::DateTimeYearFieldElement(Document* document, FieldOwner& fieldOwner, const String& placeholder) > + : DateTimeNumericFieldElement(document, fieldOwner, minimumYear, maximumYear, placeholder) We had better apply actual min/max years of the host <input>. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:194 > String DateTimeNumericFieldElement::visibleValue() const > { > - if (m_hasValue) > - return value(); > - > - StringBuilder builder; > - for (int numberOfDashs = std::max(displaySizeOfNumber(m_range.maximum), displaySizeOfNumber(m_range.minimum)); numberOfDashs; --numberOfDashs) > - builder.append('-'); > - return builder.toString(); > + return m_hasValue ? value() : m_placeholder; You assume the length of m_placeholder is longer than or equal to the length of the maximum value? (In reply to comment #7) > (From update of attachment 165963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165963&action=review > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:127 > > + case DateTimeFormat::FieldTypeMonth: > > + m_editElement.addField(DateTimeMonthFieldElement::create(document, m_editElement, m_placeholderForMonth)); > > This code seems to support only M and MM. Don't you support MMM, MMMM, MMMMM ? > No, we don't support short month name and full month name in month field. Month is always displayed as zero-padded two digits. When using month name we need to have padding space for shorter month name. > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:194 > > String DateTimeNumericFieldElement::visibleValue() const > > { > > - if (m_hasValue) > > - return value(); > > - > > - StringBuilder builder; > > - for (int numberOfDashs = std::max(displaySizeOfNumber(m_range.maximum), displaySizeOfNumber(m_range.minimum)); numberOfDashs; --numberOfDashs) > > - builder.append('-'); > > - return builder.toString(); > > + return m_hasValue ? value() : m_placeholder; > > You assume the length of m_placeholder is longer than or equal to the length of the maximum value? Yes, we'll ask localizer to choose text longer than four dashes, "----", in browser. Created attachment 166167 [details]
Patch 3
Comment on attachment 166167 [details]
Patch 3
Could you review this patch?
Thanks in advance.
P.S.
Supporting min/max attributes for year field will be in another patch with a test.
Comment on attachment 166167 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=166167&action=review > Source/WebCore/ChangeLog:11 > + Note: This patch affects ports which enable both ENABLE_INPUT_TYPE_TIME Looks wrong. ENABLE_INPUT_TYPE_TIME is unrelated. > Source/WebCore/html/MonthInputType.cpp:152 > + return String::format("%u-%02u", dateTimeFieldsState.year(), dateTimeFieldsState.month()); Don't you need %04u-%02u? > Source/WebCore/html/MonthInputType.h:39 > +#if ENABLE(INPUT_MULTIPLE_FIELDS_UI) > +#include "BaseMultipleFieldsDateAndTimeInputType.h" > +#else > +#include "BaseDateAndTimeInputType.h" > +#endif You may just include BaseMultipleFieldsDateAndTimeInputType.h. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:71 > + const String& m_placeholderForMonth; > + const String& m_placeholderForYear; See Comment #7: > Let's use "const String" instead of "const String&". I understand "const String&" works in this case, but it is not robust for the future changes. Copying String objects is not expensive. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:127 > + case DateTimeFormat::FieldTypeMonth: > + m_editElement.addField(DateTimeMonthFieldElement::create(document, m_editElement, m_placeholderForMonth)); Need a comment about MMM, MMMM, MMMMM. Created attachment 166173 [details]
Patch 4
Created attachment 166174 [details]
Patch 5
Comment on attachment 166174 [details]
Patch 5
Could you review this patch?
Thanks in advance.
= Changes since the last review =
* Not to use reference in DateTimeEditBuilder member variables.
* Use "%04u-%02u" for MonthInputType::formatDateTimeFieldsState
* Added comment for month specifier in DateTimeEditBuilder::visitField
Comment on attachment 166174 [details]
Patch 5
I think we need to add LayoutTests/platform/chromium/fast/forms/month/month-input-visible-string-expected.txt and LayoutTests/platform/chromium/fast/forms/month/month-stepup-stepdown-from-renderer-expected.txt to this patch.
Created attachment 166181 [details]
Patch 6
Comment on attachment 166181 [details]
Patch 6
Could you review this patch?
Thanks in advance.
= Changes since the last review =
* Add test expectations for knowing failure tests.
Comment on attachment 166181 [details]
Patch 6
ok.
Please land this after confirming green EWS, or land this with commit-queue.
Comment on attachment 166181 [details] Patch 6 Clearing flags on attachment: 166181 Committed r129867: <http://trac.webkit.org/changeset/129867> All reviewed patches have been landed. Closing bug. |