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.
Created attachment 181487 [details] Patch
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
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.
Created attachment 181631 [details] Patch 2
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.
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.
Created attachment 181644 [details] Patch 3
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.
Comment on attachment 181644 [details] Patch 3 ok
Comment on attachment 181644 [details] Patch 3 Clearing flags on attachment: 181644 Committed r139036: <http://trac.webkit.org/changeset/139036>
All reviewed patches have been landed. Closing bug.