Bug 106212 – INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of month/day field should respect min/max attributes

• Component: Forms
• Status: RESOLVED FIXED

[Kunihiko Sakamoto]

For example, consider this input:
<input type=date min=2012-10-20 max=2012-10-30>
We can make the month field read-only with value=10, and limit the day field range to 20-30.

Currently only year-field limits its range by min/max. We would like to implement this for month and day fields too.

[Kunihiko Sakamoto]

Created attachment 181487 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181487 [details]
Patch

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

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Kent Tamura]

Comment on attachment 181487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181487&action=review

The code change looks good.  Some nits.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:221
> +    case DateTimeFormat::FieldTypeMonthStandAlone: {
> +        int minMonth = 0, maxMonth = 11;
> +        if (m_parameters.minimum.type() != DateComponents::Invalid
> +            && m_parameters.maximum.type() != DateComponents::Invalid
> +            && m_parameters.minimum.fullYear() == m_parameters.maximum.fullYear()
> +            && m_parameters.minimum.month() <= m_parameters.maximum.month()) {
> +            minMonth = m_parameters.minimum.month();
> +            maxMonth = m_parameters.maximum.month();
> +        }
> +        RefPtr<DateTimeFieldElement> field;

There are a lot of duplicated code in the FieldTypeMonth case and the FieldTypeMonthStandAlone case.  We had better merge them.
e.g.  ...::create(document, m_editElement, fieldType == DateTimeFormat::FieldTypeMonth ? m_parameters.locale.shortMonthLabels() : m_parameters.locale.shortStandAloneMonthLabels(), minMonth, maxMonth);

> Source/WebCore/html/shadow/DateTimeFieldElements.h:64
> +    // DateTimeNumericFieldElement functions.

functions -> function

> Source/WebCore/html/shadow/DateTimeFieldElements.h:137
> +    // DateTimeNumericFieldElement functions.

Ditto.

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:77
> +    const int m_minIndex;
> +    const int m_maxIndex;

should not be abbreviations.
  m_minIndex -> m_minimumIndex
  m_maxIndex -> m_maximumIndex

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfield.html:49
> +shouldBeTrue('isReadOnlyField(createDateInput("2012-12-01", "2012-12-31", ""), pseudoMonth)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-11-01", "2013-12-31", ""), pseudoMonth)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-01", "2013-12-31", ""), pseudoMonth)');

should add out-of-range value case such as createDateInput("2012-12-01", "2012-12-31", "2012-11-30").

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfield.html:59
> +shouldBeTrue('isReadOnlyField(createDateInput("2012-12-17", "2012-12-17", ""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-17", "2013-12-18", ""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-11-17", "2012-12-17", ""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-17", "2013-12-17", ""), pseudoDay)');

Ditto.

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:11
> +if (window.internals)
> +    internals.settings.setLangAttributeAwareFormControlUIEnabled(true);

Looks unnecessary.

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:20
> +function keyDown(key, modifiers)
> +{

Style is inconsistent.  You put "{" on the same line as "function" in other functions.

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:22
> +    if (!window.eventSender)
> +        return;

You had better warn the lack of eventSender at the beginning of the test rather than ignoring eventSender.keyDown.
We sometimes load a layout test with a real browser, and if the test doesn't work on the browser, we need to investigate the reason.

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:27
> +    input.value = input.min = input.max = null;

Is this needed?

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:58
> +shouldBeEqualToString('stepUp("2000-05-01", null, null)', '2000-06-01');
> +shouldBeEqualToString('stepDown("2000-05-01", null, null)', '2000-04-01');
> +shouldBeEqualToString('stepUp("2000-12-01", null, null)', '2000-01-01');
> +shouldBeEqualToString('stepDown("2000-01-01", null, null)', '2000-12-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["upArrow", "upArrow", "upArrow"])', '2000-08-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["downArrow", "downArrow", "downArrow"])', '2000-02-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["delete", "upArrow"])', '2000-01-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["delete", "downArrow"])', '2000-12-01');

They look duplications of date-multiple-fields-keyboard-events.html.
We should remove them from this test, or merge everything to date-multiple-fields-keyboard-events.html.

> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:116
> +document.body.removeChild(input);

input.remove() is enough.

> LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-readonly-subfield-expected.txt:24
> +PASS isReadOnlyField(createMonthInput("2012-12", "2012-12", ""), pseudoMonth) is true
> +PASS isReadOnlyField(createMonthInput("2012-11", "2013-12", ""), pseudoMonth) is false
> +PASS isReadOnlyField(createMonthInput("2012-12", "2013-12", ""), pseudoMonth) is false

should add out-of-range month value cases.

[Kunihiko Sakamoto]

Created attachment 181631 [details]
Patch 2

[Kunihiko Sakamoto]

Comment on attachment 181487 [details]
Patch

Thanks for the review.
All done, please take another look.


View in context: https://bugs.webkit.org/attachment.cgi?id=181487&action=review

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:221
>> +        RefPtr<DateTimeFieldElement> field;
> 
> There are a lot of duplicated code in the FieldTypeMonth case and the FieldTypeMonthStandAlone case.  We had better merge them.
> e.g.  ...::create(document, m_editElement, fieldType == DateTimeFormat::FieldTypeMonth ? m_parameters.locale.shortMonthLabels() : m_parameters.locale.shortStandAloneMonthLabels(), minMonth, maxMonth);

Done.

>> Source/WebCore/html/shadow/DateTimeFieldElements.h:64
>> +    // DateTimeNumericFieldElement functions.
> 
> functions -> function

Done.

>> Source/WebCore/html/shadow/DateTimeFieldElements.h:137
>> +    // DateTimeNumericFieldElement functions.
> 
> Ditto.

Done.

>> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:77
>> +    const int m_maxIndex;
> 
> should not be abbreviations.
>   m_minIndex -> m_minimumIndex
>   m_maxIndex -> m_maximumIndex

Done.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfield.html:49
>> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-01", "2013-12-31", ""), pseudoMonth)');
> 
> should add out-of-range value case such as createDateInput("2012-12-01", "2012-12-31", "2012-11-30").

Done.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfield.html:59
>> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-17", "2013-12-17", ""), pseudoDay)');
> 
> Ditto.

Done.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:11
>> +    internals.settings.setLangAttributeAwareFormControlUIEnabled(true);
> 
> Looks unnecessary.

Removed.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:20
>> +{
> 
> Style is inconsistent.  You put "{" on the same line as "function" in other functions.

Done.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:22
>> +        return;
> 
> You had better warn the lack of eventSender at the beginning of the test rather than ignoring eventSender.keyDown.
> We sometimes load a layout test with a real browser, and if the test doesn't work on the browser, we need to investigate the reason.

Done.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:27
>> +    input.value = input.min = input.max = null;
> 
> Is this needed?

Removed.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:58
>> +shouldBeEqualToString('test("2000-05-01", null, null, ["delete", "downArrow"])', '2000-12-01');
> 
> They look duplications of date-multiple-fields-keyboard-events.html.
> We should remove them from this test, or merge everything to date-multiple-fields-keyboard-events.html.

Ah, I see.
Removed the test cases with min=max=null from this file.

>> LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdown-from-renderer.html:116
>> +document.body.removeChild(input);
> 
> input.remove() is enough.

Done.

>> LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-readonly-subfield-expected.txt:24
>> +PASS isReadOnlyField(createMonthInput("2012-12", "2013-12", ""), pseudoMonth) is false
> 
> should add out-of-range month value cases.

Done.

[Kent Tamura]

Comment on attachment 181631 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=181631&action=review

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:200
> +            const Vector<String>& labels = fieldType == DateTimeFormat::FieldTypeMonth
> +                ? (count == countForFullMonth
> +                    ? m_parameters.locale.monthLabels()
> +                    : m_parameters.locale.shortMonthLabels())
> +                : (count == countForFullMonth
> +                    ? m_parameters.locale.standAloneMonthLabels()
> +                    : m_parameters.locale.shortStandAloneMonthLabels());

Nested  conditional operators are not good for code readability.
I recommend not merge count==countForFullMonth case and count==countForNarrow/AbbreviatedMonth case.

[Kunihiko Sakamoto]

Created attachment 181644 [details]
Patch 3

[Kunihiko Sakamoto]

Comment on attachment 181631 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=181631&action=review

>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:200
>> +                    : m_parameters.locale.shortStandAloneMonthLabels());
> 
> Nested  conditional operators are not good for code readability.
> I recommend not merge count==countForFullMonth case and count==countForNarrow/AbbreviatedMonth case.

Done.

[Kent Tamura]

Comment on attachment 181644 [details]
Patch 3

ok

[WebKit Review Bot]

Comment on attachment 181644 [details]
Patch 3

Clearing flags on attachment: 181644

Committed r139036: <http://trac.webkit.org/changeset/139036>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.