Bug 97299

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.