HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the min and max.
Created attachment 93614 [details] Patch
Comment on attachment 93614 [details] Patch Please review my patch. The original didn't have tests http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998 So I added. If we don't need it, please let me know. Thanks,
(In reply to comment #2) > (From update of attachment 93614 [details]) > Please review my patch. > > The original didn't have tests > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998 > > So I added. No, we had. http://trac.webkit.org/changeset/72884 Please add new test cases to LayoutTest/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js.
Oops, I should I ask first;-) If you have any comment in C++ code, please give me too. Thanks, (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 93614 [details] [details]) > > Please review my patch. > > > > The original didn't have tests > > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998 > > > > So I added. > > No, we had. > http://trac.webkit.org/changeset/72884 > > Please add new test cases to LayoutTest/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js.
Comment on attachment 93614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93614&action=review > Source/WebCore/html/HTMLInputElement.cpp:339 > -void HTMLInputElement::applyStep(double count, ExceptionCode& ec) > +void HTMLInputElement::applyStep(double count, bool clearedByDefault, ExceptionCode& ec) I don't think we need to handle such cases in applyStep(). We can handle such cases only in stepUpFromRenderer(). Also, a bool function parameter is not recommended.
Created attachment 93620 [details] Patch
Comment on attachment 93620 [details] Patch Just I changed C++ part. Please review C++ part only. I'll change the test tomorrow...
Comment on attachment 93620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93620&action=review > LayoutTests/fast/forms/numeric-input-spin.html:48 > +// - The value should be the minimum value for both keyDown and keyUp. Don't call them keydown / keyup. Keydown and keyup are the names of DOM events. > Source/WebCore/html/HTMLInputElement.cpp:1466 > + int nextDiff = step * n; > + if (current < m_inputType->minimum() - nextDiff) > + current = m_inputType->minimum() - nextDiff; This looks tricky. But I understand you want to use the following step-rounding and the event dispatching. So I think this is acceptable. > Source/WebCore/html/HTMLInputElement.cpp:1475 > - if (stepMismatch(currentStringValue)) { > + if (stepMismatch(value())) { Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber(). Anyway, this also fixes another bug http://crbug.com/76046 , right?
(In reply to comment #8) > (From update of attachment 93620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93620&action=review > > > LayoutTests/fast/forms/numeric-input-spin.html:48 > > +// - The value should be the minimum value for both keyDown and keyUp. > > Don't call them keydown / keyup. Keydown and keyup are the names of DOM events. Ok, anyway the test is consolidated to the test you suggested;-) > > > Source/WebCore/html/HTMLInputElement.cpp:1466 > > + int nextDiff = step * n; > > + if (current < m_inputType->minimum() - nextDiff) > > + current = m_inputType->minimum() - nextDiff; > > This looks tricky. But I understand you want to use the following step-rounding and the event dispatching. So I think this is acceptable. Thanks, > > > Source/WebCore/html/HTMLInputElement.cpp:1475 > > - if (stepMismatch(currentStringValue)) { > > + if (stepMismatch(value())) { > > Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber(). > Anyway, this also fixes another bug http://crbug.com/76046 , right? I didn't know the bug. But fortunately, the bug is gone after the change. My original intention is to fix this one // * If 0 is in-range, but not matched to step value // - The value should be the larger matched value nearest to 0 if n > 0 // e.g. <input type=number min=-100 step=3> -> 2 Should I separate the bug entry? Also I suppose > > + if (stepMismatch(value())) { Is better than currentStringValue=value(). Because after this step, the following steps are executed, if (currentStringValue != value()) { if (m_inputType->isRangeControl()) dispatchFormControlChangeEvent(); else dispatchFormControlInputEvent(); } So shouldn't we change currentStringValue, right? What do you think?
(In reply to comment #9) > > Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber(). > > Anyway, this also fixes another bug http://crbug.com/76046 , right? > I didn't know the bug. But fortunately, the bug is gone after the change. > > My original intention is to fix this one > // * If 0 is in-range, but not matched to step value > // - The value should be the larger matched value nearest to 0 if n > 0 // e.g. <input type=number min=-100 step=3> -> 2 > > Should I separate the bug entry? I don't think so. This change is needed for this issue. Let's go ahead with the single patch. > Also I suppose > > > + if (stepMismatch(value())) { > Is better than currentStringValue=value(). > > Because after this step, the following steps are executed, > if (currentStringValue != value()) { > if (m_inputType->isRangeControl()) > dispatchFormControlChangeEvent(); > else > dispatchFormControlInputEvent(); > } > > So shouldn't we change currentStringValue, right? Ah, you're right.
Created attachment 93724 [details] Patch
Comment on attachment 93724 [details] Patch Please review again.
Comment on attachment 93724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93724&action=review > LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:245 > +shouldBe('stepUpExplicitBounds(-4, 4, 1, "")', '"1"'); > +shouldBe('stepDownExplicitBounds(-4, 4, 1, "")', '"-1"'); > +shouldBe('stepDownExplicitBounds(0, 4, 1, "")', '"0"'); > +shouldBe('stepUpExplicitBounds(-4, 0, 1, "")', '"0"'); > +shouldBe('stepDownExplicitBounds(1, 4, 1, "")', '"1"'); > +shouldBe('stepUpExplicitBounds(1, 4, 1, "")', '"1"'); > +shouldBe('stepDownExplicitBounds(-4, -1, 1, "")', '"-1"'); > +shouldBe('stepUpExplicitBounds(-4, -1, 1, "")', '"-1"'); > +shouldBe('stepUpExplicitBounds(-100, null, 3, "")', '"2"'); > +shouldBe('stepDownExplicitBounds(-100, null, 3, "")', '"-1"'); > +shouldBe('stepUpExplicitBounds(1, 4, 1, 0)', '"1"'); > +shouldBe('stepDownExplicitBounds(1, 4, 1, 0)', '"0"'); > +shouldBe('stepDownExplicitBounds(-4, -1, 1, 0)', '"-1"'); > +shouldBe('stepUpExplicitBounds(-4, -1, 1, 0)', '"0"'); > +shouldBe('stepUpExplicitBounds(-100, null, 3, 3)', '"5"'); > +shouldBe('stepDownExplicitBounds(-100, null, 3, 3)', '"2"'); nit: The function arguments should be strings unless null. Specifying numbers causes extra conversion.
Comment on attachment 93724 [details] Patch Clearing flags on attachment: 93724 Committed r86650: <http://trac.webkit.org/changeset/86650>
All reviewed patches have been landed. Closing bug.