Bug 97299 – [Forms] Multiple fields month input UI

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

Chromium port wants to have multiple fields month input UI like multiple fields time input UI.

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 165963 [details]
Patch 2

Attachment 165963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14027863

[Kent Tamura]

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.

[Kent Tamura]

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

[Kent Tamura]

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.

[Kent Tamura]

Comment on attachment 166181 [details]
Patch 6

ok.
Please land this after confirming green EWS, or land this with commit-queue.

[WebKit Review Bot]

Comment on attachment 166181 [details]
Patch 6

Clearing flags on attachment: 166181

Committed r129867: <http://trac.webkit.org/changeset/129867>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.