Bug 45491

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?

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

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! =)

(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

(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

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.

(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/.

(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. :)

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

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

Created attachment 74830 [details] Patch

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?

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

Created attachment 74835 [details] Patch

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

Created attachment 74839 [details] Patch

(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().

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.

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

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.

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.

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?

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

(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.

Created attachment 74984 [details] Patch

(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")

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

(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.

(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.

Created attachment 74988 [details] Patch

(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.

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.

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.

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

(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 ?

(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".

Created attachment 75003 [details] Patch

(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.

(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 -->

Created attachment 75004 [details] Patch

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.

Created attachment 75099 [details] Patch

(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.

Comment on attachment 75099 [details] Patch ok. Thank you for woking on this!

Comment on attachment 75099 [details] Patch Clearing flags on attachment: 75099 Committed r72884: <http://trac.webkit.org/changeset/72884>

All reviewed patches have been landed. Closing bug.