Bug 104628 – Milliseconds field of date/time input UI should respect step attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kunihiko Sakamoto]

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.

[Kunihiko Sakamoto]

Created attachment 178709 [details]
WIP Patch

[Kunihiko Sakamoto]

Kent-san, could you look at this patch to see if the approach is right?

[Kent Tamura]

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.

[Kunihiko Sakamoto]

Created attachment 178961 [details]
Patch

[Kunihiko Sakamoto]

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.

[Kent Tamura]

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.

[Kunihiko Sakamoto]

Created attachment 179172 [details]
Patch 2

[Kunihiko Sakamoto]

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.

[Kent Tamura]

Comment on attachment 179172 [details]
Patch 2

ok, looks good.

[WebKit Review Bot]

Comment on attachment 179172 [details]
Patch 2

Clearing flags on attachment: 179172

Committed r137561: <http://trac.webkit.org/changeset/137561>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.