636627 - Implement stepDown() and stepUp() for <input type='number'>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Use double instead of float.

Comment 3

[Alex Vincent [:WeirdAl]]

Peanut gallery question:  Do we want to let users say input.stepDown(-2)?  input.stepUp(0)?  input.stepUp(-0)?

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> input.stepDown(-2)? 

Why not. That should be like stepUp(2).

> input.stepUp(0)?
> input.stepUp(-0)?

Both should be a no-op.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 515639 [details] [diff] [review]
Patch v1

>+nsresult
>+nsHTMLInputElement::ApplyStep(PRInt32 aStep)

>+  // TODO: refactorize with GetMin()/GetMax(), see bug 636634.
>+  nsAutoString minStr;
>+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::min, minStr)) {
>+    double min;
>+    PRInt32 ec;
>+    min = minStr.ToDouble(&ec);
>+    if (!NS_FAILED(ec) && min > value) {
>+      return NS_ERROR_DOM_INVALID_STATE_ERR;
>+    }
>+  }
>+
>+  nsAutoString maxStr;
>+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::max, maxStr)) {
>+    double max;
>+    PRInt32 ec;
>+    max = maxStr.ToDouble(&ec);
>+    if (!NS_FAILED(ec) && max < value) {
>+      return NS_ERROR_DOM_INVALID_STATE_ERR;
>+    }
>+  }

Huh? Are these functions supposed to throw rather than clamp? That seems like a terrible idea. Do you know if there's a reason why? I would imagine the most common usecase for these functions are to back buttons in the UI, but this will currently result in a bunch of errors being logged if the user presses "too much".

Would you mind bringing this up on the list?

>+nsHTMLInputElement::StepDown(PRInt32 n)
>+{
>+  nsAXPCNativeCallContext *ncc = nsnull;
>+  nsresult rv = nsContentUtils::XPConnect()->GetCurrentNativeCallContext(&ncc);

Use [optional_argc] instead. See for example:

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIXMLHttpRequest.idl#221


>diff --git a/content/html/content/src/nsHTMLInputElement.h b/content/html/content/src/nsHTMLInputElement.h
>--- a/content/html/content/src/nsHTMLInputElement.h
>+++ b/content/html/content/src/nsHTMLInputElement.h
>@@ -532,16 +532,21 @@ protected:
>   bool DoesMinMaxApply() const;
> 
>   /**
>    * Returns if the step attribute apply for the current type.
>    */
>   bool DoesStepApply() const { return DoesMinMaxApply(); }
> 
>   /**
>+   * Returns if stepDown and stepUp methods apply for the current type.
>+   */
>+  bool DoStepDownStepUpApply() const { return DoesStepApply(); }

Why this separate function? Please just add it later if it's needed at that time. For now simply calling DoesStepApply seems quite enough.

r=me with those things fixed (I think we should make those functions not throw for now)

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Review comments

Use optionar_argc and do not throw.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Comment on attachment 515639 [details] [diff] [review]
> Patch v1
> 
> >+nsresult
> >+nsHTMLInputElement::ApplyStep(PRInt32 aStep)
> 
> >+  // TODO: refactorize with GetMin()/GetMax(), see bug 636634.
> >+  nsAutoString minStr;
> >+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::min, minStr)) {
> >+    double min;
> >+    PRInt32 ec;
> >+    min = minStr.ToDouble(&ec);
> >+    if (!NS_FAILED(ec) && min > value) {
> >+      return NS_ERROR_DOM_INVALID_STATE_ERR;
> >+    }
> >+  }
> >+
> >+  nsAutoString maxStr;
> >+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::max, maxStr)) {
> >+    double max;
> >+    PRInt32 ec;
> >+    max = maxStr.ToDouble(&ec);
> >+    if (!NS_FAILED(ec) && max < value) {
> >+      return NS_ERROR_DOM_INVALID_STATE_ERR;
> >+    }
> >+  }
> 
> Huh? Are these functions supposed to throw rather than clamp? That seems like a
> terrible idea. Do you know if there's a reason why? I would imagine the most
> common usecase for these functions are to back buttons in the UI, but this will
> currently result in a bunch of errors being logged if the user presses "too
> much".
> 
> Would you mind bringing this up on the list?

I'm ashamed that I wrote that :( This is indeed insane to throw.
I'm going to open a bug against the specs (will CC you).

> >+nsHTMLInputElement::StepDown(PRInt32 n)
> >+{
> >+  nsAXPCNativeCallContext *ncc = nsnull;
> >+  nsresult rv = nsContentUtils::XPConnect()->GetCurrentNativeCallContext(&ncc);
> 
> Use [optional_argc] instead. See for example:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIXMLHttpRequest.idl#221

Eh, that is much more easier :)

> >diff --git a/content/html/content/src/nsHTMLInputElement.h b/content/html/content/src/nsHTMLInputElement.h
> >--- a/content/html/content/src/nsHTMLInputElement.h
> >+++ b/content/html/content/src/nsHTMLInputElement.h
> >@@ -532,16 +532,21 @@ protected:
> >   bool DoesMinMaxApply() const;
> > 
> >   /**
> >    * Returns if the step attribute apply for the current type.
> >    */
> >   bool DoesStepApply() const { return DoesMinMaxApply(); }
> > 
> >   /**
> >+   * Returns if stepDown and stepUp methods apply for the current type.
> >+   */
> >+  bool DoStepDownStepUpApply() const { return DoesStepApply(); }
> 
> Why this separate function? Please just add it later if it's needed at that
> time. For now simply calling DoesStepApply seems quite enough.

I have a patch removing this code later in my queue. The patch has been reviewed so I don't think it's wroth changing that here.

Comment 8

[Mounir Lamouri (:mounir)]

Actually, I don't really like my change. I think we should at least throw if value < min or value > max when stepDown or stepUp is called because it makes no sens to call those methods in that situation and the clamping would be weird.

In addition, I wonder what we should do when the element suffer from a step mismatch. I think we should throw too. One of the reason is that it will make this situation horrible:
<input type=number min=0 step=2 max=9>
If when we go up, we clamp to 9, we will clamp to an invalid value. We very likely don't want to do that. If we clamp to 8, we have to add some checks and those checks would have no sense if value=7 when calling stepUp(). That means if value=1 and we call stepUp() a few times, we will keep invalid values (3, 5 and 7) then clamp to a valid one. It seems inconsistent. Checking if the value follows the step before clamping to not clamp and put value=9 seems not really saner.
I see three options when stepDown() and stepUp() are called with a step mismatch:
- always clamp to the closest valid value (like the higher one if stepUp() was called and the lower one otherwise);
- don't care about clamping to a value that matches step (so if value=7 max=9 and step=2, stepUp() will make value=9);
- throw when step doesn't match.

I would prefer the last one. The first could make sens but is somewhat complex. The second is weird IMO.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Going further with the change

This patch makes us throwing when the element suffers from a range underflow, a range overflow or a step mismatch. I prefer to ask a review for this change ;)

Comment 10

[Mounir Lamouri (:mounir)]

Jonas, have a look to the last comments.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How and where are you expecting these functions to be used? My understanding is that they are expected to be used in situations like this:

<form>
<input type=number min=1 max=10 step=2 id=n>
<button type=button onclick="form.n.stepUp()">more</button>
<button type=button onclick="form.n.stepDown()">less</button>
</form>

In this scenario, you I'd say that you want the following behavior:

* Neither stepUp() nor stepDown() throws no matter what the current value
  of the input is.
* stepUp() sets the value to the next valid value higher than the current
  value. If no such value exists (i.e. if the number is 9 or higher in the
  example above) the value remains unchanged.
* stepDown() sets the value to the next valid value lower than the current
  value. If no such value exists (i.e. if the number is 1 or less in the
  example above) the value remains unchanged.

Do you have other usage patterns in mind?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 527529 [details] [diff] [review]
Going further with the change

Removing from my review queue for now

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> How and where are you expecting these functions to be used? My understanding
> is that they are expected to be used in situations like this:
> 
> <form>
> <input type=number min=1 max=10 step=2 id=n>
> <button type=button onclick="form.n.stepUp()">more</button>
> <button type=button onclick="form.n.stepDown()">less</button>
> </form>
> 
> In this scenario, you I'd say that you want the following behavior:
> 
> * Neither stepUp() nor stepDown() throws no matter what the current value
>   of the input is.
> * stepUp() sets the value to the next valid value higher than the current
>   value. If no such value exists (i.e. if the number is 9 or higher in the
>   example above) the value remains unchanged.
> * stepDown() sets the value to the next valid value lower than the current
>   value. If no such value exists (i.e. if the number is 1 or less in the
>   example above) the value remains unchanged.
> 
> Do you have other usage patterns in mind?

The example you gave should not exist given that <input type='number'> should let authors use the spin buttons. Though, I can already imagine authors preferring to deal with their own buttons and hiding ours.

Anyway, after thinking about it, I agree that going up and down should always clamp to the nearest valid value in the same direction and I agree that nothing should happen if stepUp() is called and there is no higher valid value or stepDown() is called with no lower valid value.
However, I'm not sure that throwing here is a bad idea. IMO, given that we agreed to not change the value, throwing will help the authors to catch this situation which should actually not happen. In addition, in the use case you gave, throwing wouldn't be hurtful.
Why are you against throwing in that situation?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

First off, in the example above, calling stepUp or stepDown when there isn't a higher/lower valid value is not a bug. It's simply the user pressing the button when no such value is available.

If we do want to say "you should always check that there is a valid value to go to" then I agree that the above would contain a bug. But then we're also requiring them to write a whole lot more code (probably in the order of three times what's above).

What is the benefit of adding such a requirement? At that point there isn't much point in having these function at all. I.e. It would be just as easy to set .value directly.

I think the main use case for these functions is creating custom buttons like the example above, so that's what we should optimize for. What other use cases do you have in mind where throwing would be better?

The harm in throwing in the example above is that it'll put crap in the error console, as well as fire onerror which hopefully is hooked up to do error logging. Hence we're putting crap in those logs as well.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #14)
> First off, in the example above, calling stepUp or stepDown when there isn't
> a higher/lower valid value is not a bug. It's simply the user pressing the
> button when no such value is available.

Oups, I wasn't clear: I think we should throw if the value is out of range and we try to stepUp() or stepDown() in a direction where there is no valid value. If the value equals the max (or min) possible value, stepUp() (or stepDown()) should do nothing withouth throwing. IOW, when you are out of range, the only allowed action is to go back in range.
The only way to go into this situation is to manually set .value to an out of range value and continue in the wrong direction. Using the native UI, I would expect that setting a value to a wrong one will not be possible (but that's another discussion). So, this situation is very limited and I don't see any valable use case to have it happen. In that situation, I do not think throwing is completely out of consideration.

Though, we've already talked a bit too long so if you still disagree, I will assume that the oldest^Wmost experienced knows better and will propose this behavior without throwing ;)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #15)
> (In reply to comment #14)
> > First off, in the example above, calling stepUp or stepDown when there isn't
> > a higher/lower valid value is not a bug. It's simply the user pressing the
> > button when no such value is available.
> 
> Oups, I wasn't clear: I think we should throw if the value is out of range
> and we try to stepUp() or stepDown() in a direction where there is no valid
> value. If the value equals the max (or min) possible value, stepUp() (or
> stepDown()) should do nothing withouth throwing. IOW, when you are out of
> range, the only allowed action is to go back in range.
> The only way to go into this situation is to manually set .value to an out
> of range value and continue in the wrong direction.

Again, I don't think the bug appears when the stepUp/Down functions are called since that's likely in response to a user action.

In the scenario you're describing, the bug in the page would seem to be when setting .value to a out-of-range value. Making that throw seems like it would make a lot more sense to me.

Comment 17

[Mounir Lamouri (:mounir)]

(In reply to comment #16)
> In the scenario you're describing, the bug in the page would seem to be when
> setting .value to a out-of-range value. Making that throw seems like it
> would make a lot more sense to me.

But throwing when setting .value seems wrong: the element becomes invalid, that should be allowed. Maybe I should stop wasting our time and just file a bug against the specs and ask for the change...

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would prefer that yes :)

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, don't mean to be short. I agree that throwing when .value is set might not be the right thing to do, but it is where the bug is. Throwing after, when .stepUp() or .stepDown() is called is throwing at the wrong point in time IMO, i.e. it's throwing when correct code is executing.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I completely forgot about that patch. There is a new version throwing only on those situations:
- there is no step defined;
- the value is NaN.

It is not changing the value in those situations:
- stepUp() is called and the value is already higher than the maximum valid one;
- stepDown() is called and the value is already lower than the minimum.

If stepUp() or stepDown() is called and the value isn't matching the step, it will be moved to the first valid value in the correct direction.

It is not possible to go outside of [min, max] by calling stepDown() or stepUp().

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 636087 [details] [diff] [review]
Patch v2

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

r=me though I think we shouldn't throw when stepUp/Down is called an .value is empty. I think it should simply be a no-op.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also change the code which detects NaN by doing |value != value| to using the mfbt functions for NaN detection instead.

Comment 23

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/7d3e502fb58c

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #21)
> Comment on attachment 636087 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 636087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me though I think we shouldn't throw when stepUp/Down is called an .value
> is empty. I think it should simply be a no-op.

Updated.

(In reply to Jonas Sicking (:sicking) from comment #22)
> Also change the code which detects NaN by doing |value != value| to using
> the mfbt functions for NaN detection instead.

Opened bug 771182.

Comment 25

[Mounir Lamouri (:mounir)]

FWIW, the spirit of the changes we asked for as been included in the WHATWG HTML specification:
http://html5.org/tools/web-apps-tracker?from=7517&to=7518

There is a small difference between what we asked and what has been specified. I will create some bugs if they happen to be kept.

Comment 26

[Florian Scholz (Open Web Docs)]

See bug 866457 for documentation.