[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 bugzilla-daemon at webkit.org
Mon Jan 7 21:30:00 PST 2013


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





--- Comment #5 from Kunihiko Sakamoto <ksakamoto at chromium.org>  2013-01-07 21:31:54 PST ---
(From update of attachment 181487)
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.

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



More information about the webkit-unassigned mailing list