This is the first step toward supporting min/max/step attributes in multiple fields UI. For example, if step attribute is 0.25 (250ms), the milliseconds field should be selectable only from 000, 250, 500, 750.
Created attachment 178709 [details] WIP Patch
Kent-san, could you look at this patch to see if the approach is right?
Comment on attachment 178709 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178709&action=review The approach looks good. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:220 > + ASSERT(!m_parameters.stepRange.step().isZero()); > + int step = 1; > + if (Decimal(static_cast<int>(msPerSecond)).remainder(m_parameters.stepRange.step()).isZero()) > + step = static_cast<int>(m_parameters.stepRange.step().toDouble()); > + RefPtr<DateTimeNumericFieldElement> field = DateTimeMillisecondFieldElement::create(document, m_editElement, step); We should take account of non-zero step-base value. e.g. <input type=time step=0.100 min=00:00:00.050> We need to pass step-base value (the milliseconds part of the min), or stop step-aware behavior if step-base is not zero. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:71 > + , m_step(step) > { > // We show a direction-neutral string such as "--" as a placeholder. It We should have ASSERT(m_step). > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:199 > setValueAsInteger(defaultValueForStepDown(), DispatchEvent); defaultValueForStepDown() needs to be aligned to m_step. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:212 > setValueAsInteger(defaultValueForStepUp(), DispatchEvent); defaultValueForStepUp() needs to be aligned to m_step.
Created attachment 178961 [details] Patch
Comment on attachment 178709 [details] WIP Patch Thanks for the review. Updated patch with tests. Please take another look. View in context: https://bugs.webkit.org/attachment.cgi?id=178709&action=review >> Source/WebCore/html/shadow/DateTimeEditElement.cpp:220 >> + RefPtr<DateTimeNumericFieldElement> field = DateTimeMillisecondFieldElement::create(document, m_editElement, step); > > We should take account of non-zero step-base value. e.g. <input type=time step=0.100 min=00:00:00.050> > We need to pass step-base value (the milliseconds part of the min), or stop step-aware behavior if step-base is not zero. Changed to pass step-base value. >> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:71 >> // We show a direction-neutral string such as "--" as a placeholder. It > > We should have ASSERT(m_step). Done. >> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:199 >> setValueAsInteger(defaultValueForStepDown(), DispatchEvent); > > defaultValueForStepDown() needs to be aligned to m_step. Done. >> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:212 >> setValueAsInteger(defaultValueForStepUp(), DispatchEvent); > > defaultValueForStepUp() needs to be aligned to m_step. Done.
Comment on attachment 178961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178961&action=review > Source/WebCore/ChangeLog:11 > + Test: fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html You need to add tests for step=0 step=0.00001 > Source/WebCore/html/shadow/DateTimeEditElement.cpp:221 > + if (decimalMsPerSecond.remainder(m_parameters.stepRange.step()).isZero()) { You need to check m_parameters.stepRange.step().remainder(Decimal(1)).isZero() too. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:223 > + stepBase = static_cast<int>(m_parameters.stepRange.stepBase().remainder(decimalMsPerSecond).toDouble()); Why don't you subtract msPerSecond from the remainder result and assume stepBase<=0 ? It would make roundUp/roundDown simpler. > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:82 > +shouldBeEqualToString('stepDown("07:13:00.500", 0.100, "00:00:00.050", null)', '07:13:00.450'); > + We should test rounding of defaultValeuForStepDown/Up.
Created attachment 179172 [details] Patch 2
Comment on attachment 178961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178961&action=review >> Source/WebCore/ChangeLog:11 >> + Test: fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html > > You need to add tests for > step=0 > step=0.00001 Done. >> Source/WebCore/html/shadow/DateTimeEditElement.cpp:221 >> + if (decimalMsPerSecond.remainder(m_parameters.stepRange.step()).isZero()) { > > You need to check m_parameters.stepRange.step().remainder(Decimal(1)).isZero() too. Done. >> Source/WebCore/html/shadow/DateTimeEditElement.cpp:223 >> + stepBase = static_cast<int>(m_parameters.stepRange.stepBase().remainder(decimalMsPerSecond).toDouble()); > > Why don't you subtract msPerSecond from the remainder result and assume stepBase<=0 ? It would make roundUp/roundDown simpler. But roundDown still has to handle n=-1 when stepped down from zero. I'd prefer to keep roundUp/roundDown capable of handling general cases. >> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdown-from-renderer.html:82 >> + > > We should test rounding of defaultValeuForStepDown/Up. Done.
Comment on attachment 179172 [details] Patch 2 ok, looks good.
Comment on attachment 179172 [details] Patch 2 Clearing flags on attachment: 179172 Committed r137561: <http://trac.webkit.org/changeset/137561>
All reviewed patches have been landed. Closing bug.