765772 - input type=number should not allow its value to be a non valid floating point number

Status: RESOLVED FIXED

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

Description

[Raphael Catolino (:raphc)]

As specified in http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#number-state-%28type=number%29

Comment 1

[Raphael Catolino (:raphc)]

Attachment: patch wip

Adding a call to SanitizeValue on submission would prevent value from being submitted while invalid, yet someone might still access .value while the input element still has focus and get an invalid value. Sanitizing value on GetValue() might be a better idea.

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 634073 [details] [diff] [review]
patch wip

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

Look for this:
"For some input types, if the user hits enter, the form is submitted."

You should sanitize the value here if the element is of type 'number'.

Tests are welcome :)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2205,5 @@
>            }
>            break;
>          }
>  
> +        case NS_BLUR_CONTENT:

You should actually do that in ::PreHandleEvent before FireChangeEventIfNeeded() so a change event will not be fired if the value was empty, the user types 'foo' and then TAB to another element.
You should do that only if this is an element of type 'number'. You should explain why in a comment.

::: modules/libpref/src/init/all.js
@@ +633,5 @@
>  pref("dom.new_bindings", true);
>  pref("dom.experimental_bindings", true);
>  
> +//Don't use new input types
> +pref("dom.experimental_forms", false);

Does that really belong to this patch?

Comment 3

[Raphael Catolino (:raphc)]

Attachment: sanitize value on blur/submit/GetValue()

Moved sanitize to PreHandle on blur event
Added sanitize on submit
Sanitize value in GetValue();

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 634522 [details] [diff] [review]
sanitize value on blur/submit/GetValue()

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

The test you have added seems like this one:
content/html/content/test/test_bug549475.html
But I see you haven't set the pref in it. Is it passing?

You are not testing the case of implicit submission and blur.
You will need that for them: testing/mochitest/tests/SimpleTest/EventUtils.js

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +914,5 @@
>  {
> +  nsresult rv = GetValueInternal(aValue);
> +  //Don't allow invalid value for number inputs:
> +  //bug 765772
> +  if (mType==NS_FORM_INPUT_NUMBER) {

nit: space after "//".
nit: don't mention the bug number unless it is a TODO.
nit: spaces between "==".

@@ +2025,5 @@
>    // Fire onchange (if necessary), before we do the blur, bug 357684.
>    if (aVisitor.mEvent->message == NS_BLUR_CONTENT) {
> +    //In number inputs we can't allow the user to set an invalid value.
> +    //See bug 765772
> +    if (mType==NS_FORM_INPUT_NUMBER) {

nits: see above.

@@ +2318,5 @@
>                 (IsSingleLineTextControl(false, mType) ||
>                  mType == NS_FORM_INPUT_NUMBER)) {
> +            //Sanitize the value if type = number, so that we don't submit an invalid value
> +            //Bug 765772
> +            if (mType==NS_FORM_INPUT_NUMBER) {

nits: see above.

Comment 5

[Raphael Catolino (:raphc)]

Attachment: patch

Add a test cases for submission/blur

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 635230 [details] [diff] [review]
patch

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

Invalid and un-sanitized are not the same thing. We should not make that more confusing for the readers. Might be nice to update the patch in that sense.

Generally, the patch looks good but the tests still need some love.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +912,5 @@
>  NS_IMETHODIMP
>  nsHTMLInputElement::GetValue(nsAString& aValue)
>  {
> +  nsresult rv = GetValueInternal(aValue);
> +  //Don't allow invalid value for number inputs:

nit:
// Don't allow un-sanitized value for input of type 'number'.

@@ +2022,5 @@
>    aVisitor.mItemFlags |= mType;
>  
>    // Fire onchange (if necessary), before we do the blur, bug 357684.
>    if (aVisitor.mEvent->message == NS_BLUR_CONTENT) {
> +    //In number inputs we can't allow the user to set an invalid value.

nit:
// Don't allow the user to set an un-sanitized value for input of type 'number'.

@@ +2314,5 @@
>                (keyEvent->keyCode == NS_VK_RETURN ||
>                 keyEvent->keyCode == NS_VK_ENTER) &&
>                 (IsSingleLineTextControl(false, mType) ||
>                  mType == NS_FORM_INPUT_NUMBER)) {
> +            //Sanitize the value if type = number, so that we don't submit an invalid value

nit:
// Make sure we don't submit an un-sanitized value.

@@ +2318,5 @@
> +            //Sanitize the value if type = number, so that we don't submit an invalid value
> +            if (mType == NS_FORM_INPUT_NUMBER) {
> +              nsAutoString aValue;
> +              GetValueInternal(aValue);
> +              SetValueInternal(aValue, false, false);

I think you could even remove that chunk. Make sure the tests pass though. The value put in the submission data is retrieved with ::GetValue() which sanitizes the value.
By putting this code we would actually update the visible value of the field when pressing enter but that doesn't seem very important _for the moment_.

::: content/html/content/test/forms/test_input_number_value.html
@@ +15,5 @@
> +<script type="application/javascript">
> +function onFrameLoaded(){};
> +</script>
> +<p id="display"></p>
> +<iframe name="submit_frame" onload="onFrameLoaded()" style="visibility: hidden;"></iframe>

Setting onload here is a bad practice because it might be triggered by the initial frame load (about:blank) if the test is not run before. That would cause a random orange.
Instead, you should not set onload here and start the test after the page's load event (use: addLoadEvent(function() {})) and before starting the tests, you can put onFrameLoaded as the load event handler for the frame.

@@ +25,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +var input = document.getElementById('i');
> +var form = document.getElementById('f');

FTR, you can also do: document.forms[0];

@@ +52,5 @@
> +{
> +  is(frames['submit_frame'].location.href,
> +  'http://mochi.test:8888/tests/content/html/content/test/forms/foo?i='
> +  +validData.shift(),
> +  "The submitted value should not have been sanitized");

nits:
- indentation: everything should be aligned with the fist charecter inside |is(|.
- coding style: spaces around '+'

@@ +53,5 @@
> +  is(frames['submit_frame'].location.href,
> +  'http://mochi.test:8888/tests/content/html/content/test/forms/foo?i='
> +  +validData.shift(),
> +  "The submitted value should not have been sanitized");
> +  input.value="";

nit: |input.value = "";|

@@ +68,5 @@
> +  isnot(frames['submit_frame'].location.href,
> +  'http://mochi.test:8888/tests/content/html/content/test/forms/foo?i='
> +  +invalidData.shift(),
> +   "The submitted value should have been sanitized");
> +  input.value="";

ditto

@@ +75,5 @@
> +
> +function submitNextValue() {
> +  SpecialPowers.focus(input);
> +  sendString(testData[0]);
> +  form.submit();

We should test with form.submit() and implicit submission (done when RETURN is pressed).
Could you check both?

@@ +84,5 @@
> +  input.type = "number";
> +
> +  for each (data in validData) {
> +    input.value = data;
> +    is(input.value, data, "valid number should not be sanitized");

This is already tested somewhere else, no need to re-test.

@@ +89,5 @@
> +  }
> +
> +  for each (data in invalidData) {
> +    input.value = data;
> +    isnot(input.value, data, "invalid number should be sanitized");

ditto

Comment 7

[Raphael Catolino (:raphc)]

Attachment: patch

Removed sanitize on submit
Changed test according to comment 6

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 635691 [details] [diff] [review]
patch

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

r=me with the changes listed below.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +912,5 @@
>  NS_IMETHODIMP
>  nsHTMLInputElement::GetValue(nsAString& aValue)
>  {
> +  nsresult rv = GetValueInternal(aValue);
> +  // Don't allow invalid value for number inputs:

nit: s/:/./

::: content/html/content/test/forms/test_input_number_value.html
@@ +45,5 @@
> +  "foo", 
> +  "42,13", // comma can't be used as a decimal separator
> +];
> +var valueIndex = 0;
> +var submitMethod = submitForm;

Declare this *after* the functions below ;)

@@ +61,5 @@
> +  is(frames['submit_frame'].location.href,
> +     'http://mochi.test:8888/tests/content/html/content/test/forms/foo?i='
> +     + validData[valueIndex++],
> +     "The submitted value should not have been sanitized");
> +  input.value = "";

nit: leave a blank line before input.value = "";

@@ +77,5 @@
> +  isnot(frames['submit_frame'].location.href,
> +        'http://mochi.test:8888/tests/content/html/content/test/forms/foo?i='
> +        + invalidData[valueIndex++],
> +        "The submitted value should have been sanitized");
> +  input.value = "";

nit: leave a blank line before input.value = "";

@@ +116,5 @@
> +      input.value = "";
> +      SpecialPowers.focus(input);
> +      sendString(data);
> +      input.blur();
> +      isnot(input.value, data, "invalid user input should be sanitized");

is(input.value, "", "...");

Always do is() instead of isnot() unless you really don't know what the expected value is.
isnot() is kind of a "loose test".

Comment 9

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 10

[Raphael Catolino (:raphc)]

Mounir I guess you'll want to push this along with the <input type=number> patch stack, once bug 636627 is patched.

Comment 11

[Mounir Lamouri (:mounir)]

I forgot to push this patch with the other ones.

Just sent it to try:
http://tbpl.mozilla.org/?tree=Try&rev=523f77848da0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7ecc9ec354d