[Webkit-unassigned] [Bug 106212] INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of month/day field should respect min/max attributes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106212


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #181487|review?, commit-queue-      |review-, commit-queue?
               Flag|                            |




--- Comment #3 from Kent Tamura <tkent at chromium.org>  2013-01-07 02:36:12 PST ---
(From update of attachment 181487)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.