WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45491
when empty, clicking "down" on outer-spin-button returns "max value"
https://bugs.webkit.org/show_bug.cgi?id=45491
Summary
when empty, clicking "down" on outer-spin-button returns "max value"
greg+webkit-bugzilla
Reported
2010-09-09 14:04:20 PDT
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?
Attachments
Patch
(55.21 KB, patch)
2010-11-24 21:54 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(57.71 KB, patch)
2010-11-25 00:15 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(57.71 KB, patch)
2010-11-25 01:04 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(56.37 KB, patch)
2010-11-28 22:00 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(57.17 KB, patch)
2010-11-28 22:53 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(57.30 KB, patch)
2010-11-29 02:00 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(59.88 KB, patch)
2010-11-29 02:17 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Patch
(60.15 KB, patch)
2010-11-29 18:54 PST
,
Dai Mikurube
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Kent Tamura
Comment 1
2010-10-25 22:39:03 PDT
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
Comment 2
2010-10-25 23:44:17 PDT
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
greg+webkit-bugzilla
Comment 3
2010-10-26 08:27:19 PDT
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
Comment 4
2010-10-27 01:45:29 PDT
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?
greg+webkit-bugzilla
Comment 5
2010-10-27 07:34:14 PDT
Yes, that all makes a lot of sense. I'm excited about the upcoming fix. Thx again! =)
Kent Tamura
Comment 6
2010-10-30 09:38:05 PDT
(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
Comment 7
2010-11-03 23:47:47 PDT
(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
Comment 8
2010-11-19 03:25:51 PST
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
Comment 9
2010-11-19 04:03:23 PST
(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
Comment 10
2010-11-19 08:14:29 PST
(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
Comment 11
2010-11-24 00:40:18 PST
I found this bug depends on another
bug 50007
.
>
https://bugs.webkit.org/show_bug.cgi?id=50007
Dai Mikurube
Comment 12
2010-11-24 21:50:04 PST
(In reply to
comment #11
) Finally, found the 50007 is not required.
Dai Mikurube
Comment 13
2010-11-24 21:54:51 PST
Created
attachment 74830
[details]
Patch
Kent Tamura
Comment 14
2010-11-24 22:25:23 PST
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 15
2010-11-25 00:07:22 PST
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
Comment 16
2010-11-25 00:15:15 PST
Created
attachment 74835
[details]
Patch
Build Bot
Comment 17
2010-11-25 00:27:41 PST
Attachment 74830
[details]
did not build on win: Build output:
http://queues.webkit.org/results/6425019
Dai Mikurube
Comment 18
2010-11-25 01:04:10 PST
Created
attachment 74839
[details]
Patch
Dai Mikurube
Comment 19
2010-11-25 01:04:36 PST
(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 20
2010-11-25 01:26:30 PST
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
Comment 21
2010-11-25 04:35:48 PST
Attachment 74839
[details]
did not build on win: Build output:
http://queues.webkit.org/results/6304055
Kent Tamura
Comment 22
2010-11-25 17:00:23 PST
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 23
2010-11-25 17:46:17 PST
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 24
2010-11-25 18:27:10 PST
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
Comment 25
2010-11-28 13:14:07 PST
Attachment 74839
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6297080
Dai Mikurube
Comment 26
2010-11-28 21:26:42 PST
(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
Comment 27
2010-11-28 22:00:04 PST
Created
attachment 74984
[details]
Patch
Dai Mikurube
Comment 28
2010-11-28 22:02:04 PST
(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
Comment 29
2010-11-28 22:09:24 PST
Attachment 74984
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6380081
Dai Mikurube
Comment 30
2010-11-28 22:18:23 PST
(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
Comment 31
2010-11-28 22:24:37 PST
(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
Comment 32
2010-11-28 22:53:33 PST
Created
attachment 74988
[details]
Patch
Dai Mikurube
Comment 33
2010-11-28 22:54:27 PST
(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 34
2010-11-28 23:01:29 PST
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 35
2010-11-28 23:04:33 PST
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
Comment 36
2010-11-28 23:17:59 PST
Attachment 74988
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6423076
Dai Mikurube
Comment 37
2010-11-29 00:23:07 PST
(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
Comment 38
2010-11-29 01:07:10 PST
(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
Comment 39
2010-11-29 02:00:09 PST
Created
attachment 75003
[details]
Patch
Dai Mikurube
Comment 40
2010-11-29 02:04:07 PST
(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
Comment 41
2010-11-29 02:07:52 PST
(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
Comment 42
2010-11-29 02:17:32 PST
Created
attachment 75004
[details]
Patch
Kent Tamura
Comment 43
2010-11-29 17:19:54 PST
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
Comment 44
2010-11-29 18:54:03 PST
Created
attachment 75099
[details]
Patch
Dai Mikurube
Comment 45
2010-11-29 18:56:24 PST
(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 46
2010-11-29 18:59:55 PST
Comment on
attachment 75099
[details]
Patch ok. Thank you for woking on this!
WebKit Commit Bot
Comment 47
2010-11-29 22:11:25 PST
Comment on
attachment 75099
[details]
Patch Clearing flags on attachment: 75099 Committed
r72884
: <
http://trac.webkit.org/changeset/72884
>
WebKit Commit Bot
Comment 48
2010-11-29 22:11:34 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug