Closed Bug 1028712 Opened 10 years ago Closed 9 years ago

Clamp value of number input between minimum and maximum when using spinner

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jacob, Assigned: agi)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

1. Open the attached test file containing three number input controls.
2. Click the up and down spinner buttons for each of the three controls two times.


Actual results:

a. For the first control with no value attribute and a min attribute of 2, clicking the down button doesn't do anything, while clicking the up button changes the value to 1 and marks the control as invalid. After that, clicking any of the buttons doesn't do anything.
b. For the second control with no value attribute and a max attribute of -2, clicking the up button doesn't do anything, while clicking the down button changes the value to -1 and marks the control as invalid. After that, clicking any of the buttons doesn't do anything.
c. For the third control with a value attribute of 1 and a min attribute of 3, clicking any of the buttons doesn't do anything. The control is not marked as invalid.


Expected results:

For all three input controls, clicking any of the spinner buttons should set the value to the nearest valid number. For a, this means setting the value to 2; for b, to -2; and for c, to 3.
For a demonstration of this functionality, view the test file in Chromium. Chromium has the intended functionality, except in case c, where clicking the down button doesn't change the value.
Flags: needinfo?(jwatt)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Clamp value of number input between minimum and maximun when using spinner → Clamp value of number input between minimum and maximum when using spinner
This patch does fix this bug, and also make the number control set the value to the {upper,lower} nearest value that doesn't suffer from step mismatch (it was a line change, we can remove this if you don't like it), which match with Chrome behavior. 

I had to change quite a few test cases because they rely on the fact that clicking on the spinner didn't change the value if the control is in an invalid state. 

Clicking on the spinner while value is not numeric doesn't do anything (this doesn't change), while on Chrome the value is set to the nearest valid value.

Boris do you want to review this? Or we can wait for jwatt to be back.
Attachment #8459949 - Flags: review?(bzbarsky)
Try (green for win32 & android): https://tbpl.mozilla.org/?tree=Try&rev=b9c8bda7b6f0
Comment on attachment 8459949 [details] [diff] [review]
Fix number behavior when value is invalid

If we're not super-rushed, I'd prefer jwatt deals with this.  That seems to be another week, right?
Attachment #8459949 - Flags: review?(bzbarsky) → review?(jwatt)
OS: Windows 8.1 → All
Hardware: x86_64 → All
Comment on attachment 8459949 [details] [diff] [review]
Fix number behavior when value is invalid

Review of attachment 8459949 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLInputElement.cpp
@@ +2158,5 @@
>    // We don't use ValidityState because we can be higher than the maximal
>    // allowed value and still not suffer from range overflow in the case of
>    // of the value specified in @max isn't in the step.
> +  if ((value == minimum && aStep < 0) ||
> +      (value == maximum && aStep > 0)) {

I don't think you need this change, and I don't think it's correct. This early return should only apply when stepping would cause the value to have even greater under/overflow.

@@ +2170,5 @@
>    // to check HasStepMismatch and pass true as its aUseZeroIfValueNaN argument
>    // since we need to treat the value "" as zero for stepping purposes even
>    // though we don't suffer from a step mismatch when our value is the empty
>    // string.)
> +  if (HasStepMismatch(true)) {

I can't remember offhand why I had these checks, but I don't think this change is necessary to fix this bug.

@@ +2199,5 @@
>    }
>  
> +  // When this function is called and the value is below minimum, we should clamp on
> +  // minimum unless doing so moves us higher than minimum.
> +  if (!minimum.isNaN() && value <= minimum) {

Seems like this would make a step down action initiated by the user increase the value. I'm not sure we want to do that, even in this case. (Checking Chrome, it does not.)

@@ +2204,3 @@
>      value = minimum;
> +  // Same goes for when the value is above maximum.
> +  } else if (!maximum.isNaN() && value >= maximum) {

Likewise.

@@ +3658,5 @@
>  {
> +  if (!IsValid() &&
> +      !GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) &&
> +      !GetValidityState(VALIDITY_STATE_RANGE_OVERFLOW) &&
> +      !GetValidityState(VALIDITY_STATE_STEP_MISMATCH)) {

I think we want to check |GetValidityState(VALIDITY_STATE_BAD_INPUT)| here rather than checking the overflow, underflow and step mismatch states.
Attachment #8459949 - Flags: review?(jwatt) → review-
The code as it currently is is already supposed to handle these cases.

The reason things go wrong for the first and second controls is because if the user hasn't previously taken an action to set/change the value of the input its validity state is not supposed to be set (per spec), but in GetValueIfStepped the GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) and GetValidityState(VALIDITY_STATE_RANGE_OVERFLOW) require the validity state to be set to work correctly. To fix this part we should only need to change these calls to IsRangeOverflow() and IsRangeUnderflow() calls  respectively (with a comment added to not why we're not using GetValidityState).

The third input doesn't work because StepNumberControlForUserEvent returns early if |!IsValid()|. What that should actually be checking is HasBadInput(), again with a comment noting why it can't use GetValidityState(VALIDITY_STATE_BAD_INPUT).

Giovanni, are there any other cases you were trying to handle in your patch?
Flags: needinfo?(jwatt)
Thanks for taking the time jwatt!

> Giovanni, are there any other cases you were trying to handle in your patch?

There was the step mismatch case, but it looks like the code is handling that as well.

> What that should actually be checking is HasBadInput()

But this would wipe any non-number user input, correct? it looks like this is something we want to avoid, given this comment in StepNumberControlForUserEvent

> // If the user has typed a value into the control and inadvertently made a
> // mistake (e.g. put a thousand separator at the wrong point) we do not
> // want to wipe out what they typed if they try to increment/decrement the
> // value. Better is to highlight the value as being invalid so that they
> // can correct what they typed.
> // We only do this if there actually is a value typed in by/displayed to
> // the user. (IsValid() can return false if the 'required' attribute is
> // set and the value is the empty string.)

I'll post a patch with your suggested changes this weekend if you don't have objections :-)
Flags: needinfo?(jwatt)
(In reply to Giovanni Sferro [:agi] from comment #9)
> > What that should actually be checking is HasBadInput()
> 
> But this would wipe any non-number user input, correct? it looks like this
> is something we want to avoid, given this comment in
> StepNumberControlForUserEvent

It shouldn't do. If we enter that block when HasBadInput() returns true (when the value is non-numeric) the block will highlight the control as invalid and return without touching its value.
Flags: needinfo?(jwatt)
Unfortunately we can't use IsRange{Under,Over}flow because those functions return early when value.IsNaN() which is the case when the control is empty.

Checking for value <= minimum seem to work just fine, reading from IsRangeUnderflow it looks like these two checks are equivalent.

One last thing that needs to be fixed is the case when the control is empty and the minimum is positive. In this case, stepDown would do nothing with the existing code. I think this should be changed (Chrome and Opera behaves this way) to take the minimum value (and the maximum value in the opposite case in which the maximum is negative)

I added some tests for those cases. Thanks!
Assignee: nobody → agi.novanta
Attachment #8459949 - Attachment is obsolete: true
Attachment #8477893 - Flags: review?(jwatt)
Depends on: 1071994
Comment on attachment 8477893 [details] [diff] [review]
Clamp value of number input between minimum and maximum when using spinner

Sorry, agi. I had multiple passes at this, but these changes are confusing (partly because even the current code is really hard to reason about) and don't seem correct in some scenarios. Eventually, after lots of experimenting and testing, I've come to the conclusion that this entire function should be rewritten to be clearer and more closely align with the current spec text, with a few necessary exceptions. It's not practical to describe those changes to you in prose, and anyway during my experiments I ended up writing the code. I've filled bug 1071994 to get that rewrite landed, and that work should also fix this bug. What I haven't done is added your new tests over there. Could you attach a new patch with just the tests, then once bug 1071994 is fixed we can get them landed?
Attachment #8477893 - Flags: review?(jwatt) → review-
Sure, no problem. I'm glad this will get fixed one way or the other :-). This patch has only the tests. Thanks jwatt!
Attachment #8477893 - Attachment is obsolete: true
Attachment #8494474 - Flags: review?(jwatt)
Comment on attachment 8494474 [details] [diff] [review]
Add tests for stepUp and stepDown when value is emptu

Thanks, agi, and thanks for you work on this!
Attachment #8494474 - Flags: review?(jwatt) → review+
Blocks: 1100682
Comment on attachment 8494474 [details] [diff] [review]
Add tests for stepUp and stepDown when value is emptu

https://hg.mozilla.org/integration/mozilla-inbound/rev/aad2c8f7ecba

Thanks, agi!
Attachment #8494474 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/aad2c8f7ecba
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1780302
You need to log in before you can comment on or make changes to this bug.