when empty, clicking "down" on outer-spin-button returns "max value"

• Component: Forms
• Status: RESOLVED FIXED

Honestly not sure about the webkit version (sorry), but this was tested on an up-to-date Mac with OSX 10.5 with up-to-date stable Chrome and Safari. 

Minimum value was set to 0 (as we only want non-negative possible values).  When the attached field is empty, clicking "down" on an outer-spin-button for a number type field returns max value (-1.7976931348623157e+308 before I set 0 as the min; now, I get the positive version of that number). Additional clicks do not seem to affect the value when in this state, although I can edit the field, so it is not entirely "dead".

I have been searching high and low and have not been able to find an adequate solution to this problem, and every website form that I have found using the same component has the same issue.  Any ideas?

[Kent Tamura]

It's an intentional behavior.  1.7976931348623157e+308 is the hard limit of the internal number representation.

I think we can improve it.  What should happen when a user presses "down" / "up" button for an empty number field?

[Johan Douma]

Hi all, 

Related: http://code.google.com/p/chromium/issues/detail?id=50628&can=5

Even though it's a hard limit (or "intentional behavior"), most webkit (and chrome/safari) won't see it that way. 

I think most people would expect it to start from 0 when clicking on either Up or down while the field is empty.

When empty:
Pressing up: changes value from empty to 1
Pressing down: changes value from empty to -1

I don't really see any issues with that, but I might be missing something?

Cheers, 
Johan

Thanks for the comments guys.  Agreed that an empty field intuitively starts from 0.  

Where we diverge is that hitting down from an empty field where minimum is set to 0 does not intuitively lead you to a max value.  Is that the typical behavior (to loop back around)?  I would expect that hitting "down" from a minimum value would maintain the minimum value (in this case, propagate a 0 into the field without looping around to a max value.)

Also, once the max value was there, I could no longer use the button to change the value.  I had the max value in the field, and clicking up/down no longer did anything (no looping back or value change at all).  Thanks for the help!!!

-g

[Kent Tamura]

Thanks for the comments.  I agree that assuming the empty as 0 is better.

* If 0 is in-range, and matches to step value
  "down" -> -step
  "up" -> +step
  If -step or +step is out of range, new value should be 0.

* If 0 is smaller than the minimum value
 "down" -> the minimum value
 "up" -> the minimum value

* If 0 is larger than the maximum value
 "down" -> the maximum value
 "up" -> the maximum value

* If 0 is in-range, but not matched to step value
 "down" -> smaler matched value nearest to 0.
   e.g. <input type=number min=-100 step=3> -> -99
 "up" -> larger matched value nearest to 0.
   e.g. <input type=number min=-100 step=3> -> 2

As for date/datetime/datetime-local/month/time/week types, we should assume "0" is "current date/time".

Does this make sense?

Yes, that all makes a lot of sense.  I'm excited about the upcoming fix. Thx again! =)

[Kent Tamura]

(In reply to comment #4)
> * If 0 is in-range, but not matched to step value
>  "down" -> smaler matched value nearest to 0.
>    e.g. <input type=number min=-100 step=3> -> -99

Correction:
  <input type=number min=-100 step=3> -> -1

[Dai Mikurube]

(In reply to comment #6)
The case if 0 is in-range, but not matched to step value is related to the bug 48976.
https://bugs.webkit.org/show_bug.cgi?id=48976

[Dai Mikurube]

Would someone tell me some nice ways to test "current date/time" for date/datetime/datetime-local/month/time/week types? For now, I've started to work on this bug and it looks like almost working.


IFMO, options are :

1-A. Add a function like setCurrentTimeAndStop() into layoutTestController. And add a hook into JavaScriptCore/wtf/CurrentTime.c:currentTime().

1-B. Add setCurrentTimeAndStop(). In contrast to 1-A, add another function like currentTimeWithTest() into CurrentTime.c. The if-statement is in the new function. No influence on the original currentTime().

2. "new Date()" in the test (like fast/forms/input-stepup-stepdown-from-renderer.html), and compare the results of stepUp()/stepDown() with the constructed Date.

I think 1-B is the best in them. But the number of functions increases if we applied the same approach for other tests in future.

I'm wondering if you could tell me which is better, or another nice idea.

[Kent Tamura]

(In reply to comment #8)
> Would someone tell me some nice ways to test "current date/time" for date/datetime/datetime-local/month/time/week types? For now, I've started to work on this bug and it looks like almost working.
> 
> 
> IFMO, options are :

The question is almost equivalent to "How to test 'new Date()'?".
As far as I know, we don't have tests of 'new Date()' behavior.

WTF::currentTIme() must be a hot-spot and we'd like to avoid to change it :-)
So, the easiest way is to make a manual test.  See WebCore/manual-tests/.

[Dai Mikurube]

(In reply to comment #9)
> The question is almost equivalent to "How to test 'new Date()'?".
> As far as I know, we don't have tests of 'new Date()' behavior.
Yes. Got it.

> WTF::currentTIme() must be a hot-spot and we'd like to avoid to change it :-)
> So, the easiest way is to make a manual test.  See WebCore/manual-tests/.
Thank you. Manual-tests looks like reasonable. 
1-B doesn't affect other currentTime() calls, but we cannot apply the same approaches to other calls. :)

[Dai Mikurube]

I found this bug depends on another bug 50007.
> https://bugs.webkit.org/show_bug.cgi?id=50007

[Dai Mikurube]

(In reply to comment #11)
Finally, found the 50007 is not required.

[Dai Mikurube]

Created attachment 74830 [details]
Patch

[Kent Tamura]

Comment on attachment 74830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74830&action=review

quick review.

> JavaScriptCore/ChangeLog:7
> +        when empty, clicking "down" on outer-spin-button returns "max value"
> +        https://bugs.webkit.org/show_bug.cgi?id=45491
> +

Write why we need this wtf/ change please.

> JavaScriptCore/wtf/DateMath.h:89
> +// Returns the offsets for UTC and DST.
> +int32_t calculateUTCOffset();
> +double calculateDSTOffset(double ms, double utcOffset);

The return values are unclear.  Milliseconds, seconds, minutes, or hours?

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:28
> +    if (typeof optionalStepCount != "undefined")
> +        if (optionalStepCount < 0)
> +            for (var i = 0; i < -optionalStepCount; i++)
> +                sendKey('Down');
> +        else
> +            for (var i = 0; i < optionalStepCount; i++)
> +                sendKey('Up');
> +    else

If you follow WebKit C++ style in JavaScript code, we should add {}  for multiple-line if-clause / else-clause.

> WebCore/ChangeLog:7
> +        when empty, clicking "down" on outer-spin-button returns "max value"
> +        https://bugs.webkit.org/show_bug.cgi?id=45491
> +

Please write what behavior change we have.

> WebCore/html/HTMLInputElement.h:270
> +    virtual bool isRangeControl() const { return deprecatedInputType() == RANGE; }

Do not use deprecatedInputType() in new code.

> WebCore/html/InputType.h:92
> +    virtual double defaultValue() const;

The name "defaultValue" is not good.  The functions of InputType are usually synchronized with HTMLInputElement, and HTMLInputElement already has defaultValue().

> WebCore/html/RangeInputType.cpp:165
> -        String lastStringValue = element()->value();
> -        element()->stepUp(stepMagnification, ec);
> -        if (lastStringValue != element()->value())
> -            element()->dispatchFormControlChangeEvent();
> +        // Using stepUpFromRenderer() since it is a kind of stepUp() operation
> +        element()->stepUpFromRenderer(stepMagnification);

Is this change related to this bug?

[Dai Mikurube]

Comment on attachment 74830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74830&action=review

>> JavaScriptCore/ChangeLog:7
>> +
> 
> Write why we need this wtf/ change please.

Added a description.

>> JavaScriptCore/wtf/DateMath.h:89
>> +double calculateDSTOffset(double ms, double utcOffset);
> 
> The return values are unclear.  Milliseconds, seconds, minutes, or hours?

Milliseconds. Added.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:28
>> +    else
> 
> If you follow WebKit C++ style in JavaScript code, we should add {}  for multiple-line if-clause / else-clause.

Done. Thanks!

>> WebCore/ChangeLog:7
>> +
> 
> Please write what behavior change we have.

Described.

>> WebCore/html/HTMLInputElement.h:270
>> +    virtual bool isRangeControl() const { return deprecatedInputType() == RANGE; }
> 
> Do not use deprecatedInputType() in new code.

Replaced with formControlType() == InputTypeNames::range().
Is it ok?

>> WebCore/html/InputType.h:92
>> +    virtual double defaultValue() const;
> 
> The name "defaultValue" is not good.  The functions of InputType are usually synchronized with HTMLInputElement, and HTMLInputElement already has defaultValue().

Replaced with startingValue().

>> WebCore/html/RangeInputType.cpp:165
>> +        element()->stepUpFromRenderer(stepMagnification);
> 
> Is this change related to this bug?

Yes, keydowns from renderer for range-type inputs are handled with stepUp(), not stepUpFromRenderer() without this change. It results in no clamping steps for range-type inputs.

If clamping steps (stepwise adjustment) is not related in this bug, this change should be done in another bug. We discussed it at https://bugs.webkit.org/show_bug.cgi?id=48976#c23

[Dai Mikurube]

Created attachment 74835 [details]
Patch

[Build Bot]

Attachment 74830 [details] did not build on win:
Build output: http://queues.webkit.org/results/6425019

[Dai Mikurube]

Created attachment 74839 [details]
Patch

[Dai Mikurube]

(In reply to comment #17)
> Attachment 74830 [details] did not build on win:
> Build output: http://queues.webkit.org/results/6425019

I wrongly exchanged calculateUTCOffset() and calculateDSTOffset().

[Kent Tamura]

Comment on attachment 74830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74830&action=review

>>> WebCore/html/HTMLInputElement.h:270
>>> +    virtual bool isRangeControl() const { return deprecatedInputType() == RANGE; }
>> 
>> Do not use deprecatedInputType() in new code.
> 
> Replaced with formControlType() == InputTypeNames::range().
> Is it ok?

It's acceptable.  But the best way is to add isRangeControl() to InputType and RangeInputType.
isRangeControl() is not called by other classes.  We don't need to add it to the public section of HTMLInputElement.

>>> WebCore/html/InputType.h:92
>>> +    virtual double defaultValue() const;
>> 
>> The name "defaultValue" is not good.  The functions of InputType are usually synchronized with HTMLInputElement, and HTMLInputElement already has defaultValue().
> 
> Replaced with startingValue().

It is not self-descriptible.  How about defaultValueForStepUp() or something?

>>> WebCore/html/RangeInputType.cpp:165
>>> +        element()->stepUpFromRenderer(stepMagnification);
>> 
>> Is this change related to this bug?
> 
> Yes, keydowns from renderer for range-type inputs are handled with stepUp(), not stepUpFromRenderer() without this change. It results in no clamping steps for range-type inputs.
> 
> If clamping steps (stepwise adjustment) is not related in this bug, this change should be done in another bug. We discussed it at https://bugs.webkit.org/show_bug.cgi?id=48976#c23

Ok, I understand.  stepUp() is insufficient for "natural" behavior and we had better centralize the code.

[Build Bot]

Attachment 74839 [details] did not build on win:
Build output: http://queues.webkit.org/results/6304055

[Kent Tamura]

Comment on attachment 74839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74839&action=review

I stopped reviewing halfway.  The test quality is bad.  Please be careful.

> JavaScriptCore/ChangeLog:20
> +        Another option was to provide another currentTime() to return the current local milliseconds.
> +        Surveying the codes, however, all of existing "milliseconds" in double variables are
> +        consistently in UTC. I guess we should not emit "milliseconds" in non-UTC in the code.
> +        Finally I locked "milliseconds" in non-UTC in calculating default values.

This paragraph looks unnecessary.
Also, I feel it's strange to have the word "I" in ChangeLog.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:1
> +description('Check stepUp() and stepDown() bahevior from renderer for type=date, datetime, datetime-local, month, time, week.');

The test contains "number" and "range" types.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:88
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10", null, null, "0")', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, "0")', '"2010-02-10"');
> +shouldBe('stepUp("2010-02-10", null, null, "foo")', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, "foo")', '"2010-02-10"');
> +shouldBe('stepUp("2010-02-10", null, null, null)', '"2010-02-10"');
> +shouldBe('stepDown("2010-02-10", null, null, null)', '"2010-02-10"');

The 4th parameter is recognized in JavaScript code and is not passed to C++ code.
We don't need to test invalid values.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:101
> +shouldBe('stepUp("2010-02-10", "3.40282346e+38", null)','"1970-01-01"');
> +shouldBe('stepDown("2010-02-10", "3.40282346e+38", null)', '"275760-09-13"');

These results look very strange.  Are they expected?

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:114
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, "0")', '"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, "0")', '"2010-02-10T20:13Z"');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, "foo")', '"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, "foo")', '"2010-02-10T20:13Z"');
> +shouldBe('stepUp("2010-02-10T20:13Z", null, null, null)', '"2010-02-10T20:13Z"');
> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, null)', '"2010-02-10T20:13Z"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:140
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, "0")', '"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, "0")', '"2010-02-10T20:13"');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, "foo")', '"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, "foo")', '"2010-02-10T20:13"');
> +shouldBe('stepUp("2010-02-10T20:13", null, null, null)', '"2010-02-10T20:13"');
> +shouldBe('stepDown("2010-02-10T20:13", null, null, null)', '"2010-02-10T20:13"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:166
> +debug('Non-number arguments');
> +shouldBe('stepUp("2010-02", null, null, "0")', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, "0")', '"2010-02"');
> +shouldBe('stepUp("2010-02", null, null, "foo")', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, "foo")', '"2010-02"');
> +shouldBe('stepUp("2010-02", null, null, null)', '"2010-02"');
> +shouldBe('stepDown("2010-02", null, null, null)', '"2010-02"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:195
> +debug('Non-number arguments');
> +shouldBe('stepUp("0", null, null, "0")', '"0"');
> +shouldBe('stepDown("0", null, null, "0")', '"0"');
> +shouldBe('stepUp("0", null, null, "foo")', '"0"');
> +shouldBe('stepDown("0", null, null, "foo")', '"0"');
> +shouldBe('stepUp("0", null, null, null)', '"0"');
> +shouldBe('stepDown("0", null, null, null)', '"0"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:205
> +debug('Extra arguments');
> +shouldBe('input.value = "0"; input.min = null; input.step = null; input.stepUp(1, 2); input.value', '"1"');
> +shouldBe('input.value = "1"; input.stepDown(1, 3); input.value', '"0"');

These are not related to the code change.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:212
> +debug('Invalid step value');
> +shouldBe('stepUp("0", "foo", null)', '"1"');
> +shouldBe('stepUp("1", "0", null)', '"2"');
> +shouldBe('stepUp("2", "-1", null)', '"3"');
> +debug('Step=any');
> +shouldBe('stepUp("0", "any", null)', '"0"');
> +shouldBe('stepDown("0", "any", null)', '"0"');

We should add test cases with empty/invalid values and invalid/any step.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:217
> +shouldBe('input.value', '"0"');

This doesn't test anything.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:222
> +shouldBe('input.value', '"0"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:224
> +debug('stepDown()/stepUp() for stepMismatch values');

Other types should have similar test cases.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:226
> +shouldBe('input.stepDown(); input.value', '"0"');

This is not related to the code change.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:227
> +shouldBe('input.min = "0"; stepUp("9", "10", "", 9)', '"90"');

This looks equivalent to
shouldBe('input.min = "0"; stepUp("9", "10", "")', '"10"');
The 4th parameter is not needed.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:236
> +shouldBe('input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.stepUp(); input.value', '"3"');

This is not related to the code change.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:238
> +shouldBe('for (var i = 0; i < 255; i++) { input.stepDown(); }; input.value', '"0"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:250
> +debug('function arguments are (min, max, step, value, [stepCount])');

Nice to have explanation of stepUp() and stepDown() too.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:260
> +debug('Non-number arguments (stepCount)');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", "0")', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", "0")', '"0"');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", "foo")', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", "foo")', '"0"');
> +shouldBe('stepUpExplicitBounds(null, null, null, "0", null)', '"0"');
> +shouldBe('stepDownExplicitBounds(null, null, null, "0", null)', '"0"');

It makes no sense to test invalid 5th parameter.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:270
> +debug('Extra arguments');
> +shouldBe('setInputAttributes(null, null, null, "0"); input.stepUp(1,2); input.value', '"1"');
> +shouldBe('setInputAttributes(null, null, null, "1"); input.stepDown(1,3); input.value', '"0"');

These are not related to the code change.

[Dai Mikurube]

Comment on attachment 74839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74839&action=review

Thank you for the comments.
It's just a reply. No new patch is attached yet. I'll attach it after adding some test cases.

>> JavaScriptCore/ChangeLog:20
>> +        Finally I locked "milliseconds" in non-UTC in calculating default values.
> 
> This paragraph looks unnecessary.
> Also, I feel it's strange to have the word "I" in ChangeLog.

Removed.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:1
>> +description('Check stepUp() and stepDown() bahevior from renderer for type=date, datetime, datetime-local, month, time, week.');
> 
> The test contains "number" and "range" types.

Done.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:88
>> +shouldBe('stepDown("2010-02-10", null, null, null)', '"2010-02-10"');
> 
> The 4th parameter is recognized in JavaScript code and is not passed to C++ code.
> We don't need to test invalid values.

Exactly. Removed.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:101
>> +shouldBe('stepDown("2010-02-10", "3.40282346e+38", null)', '"275760-09-13"');
> 
> These results look very strange.  Are they expected?

No "max" is given, so it results in the maximum number (date) for type=date under 3.40282346e+38.

The maximum number is defined in WebCore/html/DateComponent.h :
WebCore/html/DateComponents.h:    static inline double maximumDate() { return 8640000000000000.0; } // 275760-09-13T00:00Z
WebCore/html/DateComponents.h:    static inline double maximumMonth() { return (275760 - 1970) * 12.0 + 9 - 1; } // 275760-09
WebCore/html/DateComponents.h:    static inline double maximumWeek() { return 8639999568000000.0; } // 275760-09-08, the Monday of the week including 275760-09-13.

1970-01-01 comes from the internal representation. The date is represented by 0. In this tag, the step-base is 0. So it resulted in 1970-01-01. It may be unnatural.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:114
>> +shouldBe('stepDown("2010-02-10T20:13Z", null, null, null)', '"2010-02-10T20:13Z"');
> 
> ditto.

Removed. (The same for dittos.)

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:205
>> +shouldBe('input.value = "1"; input.stepDown(1, 3); input.value', '"0"');
> 
> These are not related to the code change.

Right, it doesn't use stepUpFromRenderer(). Removed cases with "input.stepUp/Down".

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:212
>> +shouldBe('stepDown("0", "any", null)', '"0"');
> 
> We should add test cases with empty/invalid values and invalid/any step.

Ok, adding.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:217
>> +shouldBe('input.value', '"0"');
> 
> This doesn't test anything.

Right. Removed.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:224
>> +debug('stepDown()/stepUp() for stepMismatch values');
> 
> Other types should have similar test cases.

Thanks. Adding.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:227
>> +shouldBe('input.min = "0"; stepUp("9", "10", "", 9)', '"90"');
> 
> This looks equivalent to
> shouldBe('input.min = "0"; stepUp("9", "10", "")', '"10"');
> The 4th parameter is not needed.

Replaced.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:250
>> +debug('function arguments are (min, max, step, value, [stepCount])');
> 
> Nice to have explanation of stepUp() and stepDown() too.

Added explanations.

>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:260
>> +shouldBe('stepDownExplicitBounds(null, null, null, "0", null)', '"0"');
> 
> It makes no sense to test invalid 5th parameter.

Removed.

[Kent Tamura]

Comment on attachment 74839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74839&action=review

>>> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:101
>>> +shouldBe('stepDown("2010-02-10", "3.40282346e+38", null)', '"275760-09-13"');
>> 
>> These results look very strange.  Are they expected?
> 
> No "max" is given, so it results in the maximum number (date) for type=date under 3.40282346e+38.
> 
> The maximum number is defined in WebCore/html/DateComponent.h :
> WebCore/html/DateComponents.h:    static inline double maximumDate() { return 8640000000000000.0; } // 275760-09-13T00:00Z
> WebCore/html/DateComponents.h:    static inline double maximumMonth() { return (275760 - 1970) * 12.0 + 9 - 1; } // 275760-09
> WebCore/html/DateComponents.h:    static inline double maximumWeek() { return 8639999568000000.0; } // 275760-09-08, the Monday of the week including 275760-09-13.
> 
> 1970-01-01 comes from the internal representation. The date is represented by 0. In this tag, the step-base is 0. So it resulted in 1970-01-01. It may be unnatural.

Why does 2010-02-10 + huge step become 1970-01-01, which is less than the initial value?
Why does 2010-02-10 - huge step become 275769-09-13, which is larger than the initial value?

[WebKit Review Bot]

Attachment 74839 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6297080

[Dai Mikurube]

(In reply to comment #24)
> Why does 2010-02-10 + huge step become 1970-01-01, which is less than the initial value?
> Why does 2010-02-10 - huge step become 275769-09-13, which is larger than the initial value?
Thank you for This was a mistake. I fixed it.


BTW, what do you think which is preferable stepUp/Down for <input value=(empty/invalid) step="any" /> ?
- Not any changes after stepUp/Down.
  (e.g. step-up from renderer for <input type="number" value="foo" step="any" /> => "foo")
- Assuming value="0" or the current time, and no stepping due to step="any".
  (e.g. step-up from renderer for <input type="number" value="foo" step="any" /> => "0")

I think the latter is better to avoid invalid inputs.
In addition, the former looks complicating because value is sanitized out of stepUpFromRenderer().

FYI: The stepUp/Down DOM APIs for invalid values cause DOM Exceptions.

[Dai Mikurube]

Created attachment 74984 [details]
Patch

[Dai Mikurube]

(In reply to comment #27)
> Created an attachment (id=74984) [details]
> Patch

This is a patch for the case of :
> - Assuming value="0" or the current time, and no stepping due to step="any".
>   (e.g. step-up from renderer for <input type="number" value="foo" step="any" /> => "0")

[WebKit Review Bot]

Attachment 74984 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6380081

[Dai Mikurube]

(In reply to comment #29)
> Attachment 74984 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/6380081

Looks like missing include files... Submit an added patch later.

[Kent Tamura]

(In reply to comment #26)
> BTW, what do you think which is preferable stepUp/Down for <input value=(empty/invalid) step="any" /> ?
> - Not any changes after stepUp/Down.
>   (e.g. step-up from renderer for <input type="number" value="foo" step="any" /> => "foo")
> - Assuming value="0" or the current time, and no stepping due to step="any".
>   (e.g. step-up from renderer for <input type="number" value="foo" step="any" /> => "0")
> 
> I think the latter is better to avoid invalid inputs.
> In addition, the former looks complicating because value is sanitized out of stepUpFromRenderer().

I have no preference on it.
We had better add a FIXME comment that explains another choice.

[Dai Mikurube]

Created attachment 74988 [details]
Patch

[Dai Mikurube]

(In reply to comment #31)

> I have no preference on it.
> We had better add a FIXME comment that explains another choice.

Thanks. Revised for include files and added FIXME.

[Kent Tamura]

Comment on attachment 74984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74984&action=review

> JavaScriptCore/ChangeLog:12
> +        Calculating milliseconds from a struct tm is not clear. timegm() cannot be used in all

I don't catch what "clear" here means.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:1
> +description('Check stepUp() and stepDown() for <input> from renderer. No cases of empty initial values for type=date, datetime, datetime-local, month, time, week.');

This is not a test for stepUp() and stepDown(), which are standard DOM API.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:89
> +debug('function arguments are (value, step, {min or max}, [stepCount])');

A sentence should start with a capital letter and end with a period.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:275
> +shouldBe('stepUpExplicitBounds(null, "100", "1", "100")', '"100"');
> +shouldBe('input.value', '"100"');

Checking input.value again makes no sense.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:276
> +shouldBe('stepUpExplicitBounds(null, "100", "1", "99", "2")', '"100"');

The 5th parameter doesn't need to be a string.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:277
> +shouldBe('input.value', '"100"');

Checking input.value again makes no sense.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:280
> +shouldBe('input.value', '"0"');

ditto.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:281
> +shouldBe('stepDownExplicitBounds("0", null, "1", "1", "2")', '"0"');

The 5th parameter doesn't need to be a string.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:282
> +shouldBe('input.value', '"0"');

Checking input.value again makes no sense.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:283
> +shouldBe('stepDownExplicitBounds(null, null, "3.40282346e+38", "1", "2")', '"0"');

The 5th parameter doesn't need to be a string.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:287
> +shouldBe('input.value', '"0"');

Checking input.value again makes no sense.

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:288
> +shouldBe('stepUpExplicitBounds(null, null, "3.40282346e+38", "1", "2")', '"0"');

The 5th parameter doesn't need to be a string.

> WebCore/ChangeLog:42
> +        (WebCore::BaseDateAndTimeInputType::startingValue): Added startingValue() which returns the current local time

Please update the ChangeLog.

> WebCore/ChangeLog:62
> +        * manual-tests/input-type-datetime-default-value.html: Added manual tests for default values of date/time inputs

Please add a reason why we need a manual test.

> WebCore/html/HTMLInputElement.cpp:1901
> +    int sign = n * (step < 0 ? (-1) : (1));

Inner parentheses are redundant.

> WebCore/html/HTMLInputElement.cpp:1916
> +        if (stepMismatch(currentStringValue)) {
> +            double newValue;

We had better have ASSERT(step).

> WebCore/html/RangeInputType.cpp:152
> +        // Not using stepUpFromRenderer() since it is not any kind of stepUp()
> +        // No stepUp() for step="any"

These words are unclear though I understand what you'd like to say.

[Kent Tamura]

Comment on attachment 74988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74988&action=review

> WebCore/html/HTMLInputElement.cpp:1896
> +    // (e.g. Stepping-up for <input type="number" value="foo" step="any" /> => "foo")

The example is wrong.  The value for <input type=number value=foo step=any> is "" because of sanitization.

[WebKit Review Bot]

Attachment 74988 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6423076

[Dai Mikurube]

(In reply to comment #35)
> (From update of attachment 74988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74988&action=review
> 
> > WebCore/html/HTMLInputElement.cpp:1896
> > +    // (e.g. Stepping-up for <input type="number" value="foo" step="any" /> => "foo")
> 
> The example is wrong.  The value for <input type=number value=foo step=any> is "" because of sanitization.

Ok, thanks. Then, what do you mean by "invalid values" :
> We should add test cases with empty/invalid values and invalid/any step.
in #22 ?

[Kent Tamura]

(In reply to comment #37)
> > > +    // (e.g. Stepping-up for <input type="number" value="foo" step="any" /> => "foo")
> > 
> > The example is wrong.  The value for <input type=number value=foo step=any> is "" because of sanitization.
> 
> Ok, thanks. Then, what do you mean by "invalid values" :
> > We should add test cases with empty/invalid values and invalid/any step.
> in #22 ?

Probably yes.  I meant "non-number".

[Dai Mikurube]

Created attachment 75003 [details]
Patch

[Dai Mikurube]

(In reply to comment #38)
> (In reply to comment #37)
> > > > +    // (e.g. Stepping-up for <input type="number" value="foo" step="any" /> => "foo")
> > > 
> > > The example is wrong.  The value for <input type=number value=foo step=any> is "" because of sanitization.
> > 
> > Ok, thanks. Then, what do you mean by "invalid values" :
> > > We should add test cases with empty/invalid values and invalid/any step.
> > in #22 ?
> 
> Probably yes.  I meant "non-number".

Ok, I kept it and just changed the comment as follows :
    // FIXME: Not any changes after stepping, even if it is an invalid value, may be better.

[Dai Mikurube]

(In reply to comment #39)
> Created an attachment (id=75003) [details]
> Patch

Sorry, canceled it as no manual tests are added.

> 8 <!--
> 9 ****
> 10  We should add test cases with empty/invalid values and invalid/any step.
> 11 ***
> 12 -->

[Dai Mikurube]

Created attachment 75004 [details]
Patch

[Kent Tamura]

Comment on attachment 75004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75004&action=review

> WebCore/html/BaseDateAndTimeInputType.cpp:103
> +double BaseDateAndTimeInputType::defaultValueForStepUp() const
> +{
> +    double ms = currentTimeMS();
> +    double utcOffset = calculateUTCOffset();
> +    double dstOffset = calculateDSTOffset(ms, utcOffset);
> +    int offset = static_cast<int>((utcOffset + dstOffset) / msPerMinute);
> +    return ms + (offset * msPerMinute);
> +}

If the current date-time is 2010-11-30T23:00Z and the user's timezone is UTC+9, the default value would be
date: 2010-12-01
datetime: 2010-11-30T23:00Z
datetime-local: 2010-12-01T08:00
month: 2010-12
time: 08:00
week: 2010-Wxx (the week of 2010-12-01)

Right?

> WebCore/manual-tests/input-type-datetime-default-value.html:66
> +<p>The input fields should show the current date/time (with + or - a unit date/time described below except for step=any).</p>

So, we should explain the expectations are current *local* date/time except type=datetime.

[Dai Mikurube]

Created attachment 75099 [details]
Patch

[Dai Mikurube]

(In reply to comment #43)
> (From update of attachment 75004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75004&action=review
> 
> > WebCore/html/BaseDateAndTimeInputType.cpp:103
> > +double BaseDateAndTimeInputType::defaultValueForStepUp() const
> > +{
> > +    double ms = currentTimeMS();
> > +    double utcOffset = calculateUTCOffset();
> > +    double dstOffset = calculateDSTOffset(ms, utcOffset);
> > +    int offset = static_cast<int>((utcOffset + dstOffset) / msPerMinute);
> > +    return ms + (offset * msPerMinute);
> > +}
> 
> If the current date-time is 2010-11-30T23:00Z and the user's timezone is UTC+9, the default value would be
> date: 2010-12-01
> datetime: 2010-11-30T23:00Z
> datetime-local: 2010-12-01T08:00
> month: 2010-12
> time: 08:00
> week: 2010-Wxx (the week of 2010-12-01)
> 
> Right?
> 
> > WebCore/manual-tests/input-type-datetime-default-value.html:66
> > +<p>The input fields should show the current date/time (with + or - a unit date/time described below except for step=any).</p>
> 
> So, we should explain the expectations are current *local* date/time except type=datetime.

Thanks. Added some comments about local/UTC on source code and the description here.
Because not only local, I wrote local/UTC here. Added descriptions for each of types.

[Kent Tamura]

Comment on attachment 75099 [details]
Patch

ok.  Thank you for woking on this!

[WebKit Commit Bot]

Comment on attachment 75099 [details]
Patch

Clearing flags on attachment: 75099

Committed r72884: <http://trac.webkit.org/changeset/72884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.