836323 - Mochitest additions and fixes to provide coverage of <input type=range>

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Spinning this out from bug 344618 since this can be landed as soon as it has reviews.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Mounir Lamouri (:mounir)]

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

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

You didn't write any test for 'range' in:
- content/html/content/test/forms/test_step_attribute.html
- content/html/content/test/forms/test_stepup_stepdown.html
- content/html/content/test/forms/test_max_attribute.html
- content/html/content/test/forms/test_min_attribute.html

I would add tests in content/html/content/test/forms/test_input_sanitization.html or in another file to make sure that we correctly clamp to a valid value if the element's value is set to an invalid value.

In addition, shouldn't you modify:
- content/html/content/test/test_bug590363.html
- content/html/content/test/test_bug598643.html

::: content/html/content/test/forms/test_validation.html
@@ +286,5 @@
>  }
>  
>  SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", true],
> +                                   ["dom.experimental_forms_range", true]]}, function() {

I do not think you need that here.
I'm not sure why "dom.experimental_forms" was required either actually.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch + review comments

(In reply to Mounir Lamouri (:mounir) from comment #2)
> I would add tests in
> content/html/content/test/forms/test_input_sanitization.html or in another
> file to make sure that we correctly clamp to a valid value if the element's
> value is set to an invalid value.

That's pretty thoroughly tested in test_stepup_stepdown.html, believe me (tons of the tests in there initially failed).

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 713469 [details] [diff] [review]
patch + review comments

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

r=me with the comments applied.

However, I think we should create a test dedicated to the step/min/max rounding crazyness. For example, I wonder how we would handle that while parsing:
  <input type='range' max='foo' min='bar' value='foobar' step='tulip'>
Would .value have a different value depending on the order of those attributes? It should not IMO.

For the moment, I'm fine with landing this as-is (and likely the related code) and have a follow-up that takes care of testing this and fixing if needed.

::: content/html/content/test/forms/test_max_attribute.html
@@ +98,5 @@
>      todo_is(input.type, test.type, test.type + " isn't implemented yet");
>      continue;
>    }
>  
> +  checkValidity(input, true, apply, test.type == 'range'); // range has default minimum/maximum

I would prefer:
// 'range' has a default minimum and maximum so it has a min and max by default.
if (input.type == 'range') {
  checkValidity(input, true, apply, true);
} else {
  checkValidity(input, true, apply, false);
}

@@ +268,5 @@
> +      input.value = 'foo';
> +      checkValidity(input, true, apply, apply);
> +
> +      input.value = '3';
> +      checkValidity(input, true, apply, apply);

Add a test that does:
is(input.value, input.max, "the value should have been set to max");

@@ +274,5 @@
> +      input.max = '5';
> +      checkValidity(input, true, apply, apply);
> +
> +      input.value = '42';
> +      checkValidity(input, true, apply, apply);

Same here.

@@ +284,5 @@
> +      checkValidity(input, true, apply, apply);
> +
> +      // We don't check the conversion of input.max to a double in
> +      // validationMessage for 'range' since range sanitizes, so there is no
> +      // validationMessage.

This is not really true, you can test the following:
input.min = 5;
input.max = 0.66666666666666666666666666666666666
input.step = 'any';
input.value = 1;

Then, the value is suffering from an overflow, isn't it?

Could you add that test?

::: content/html/content/test/forms/test_min_attribute.html
@@ +99,5 @@
>      continue;
>    }
>  
>    // The element should be valid. Range should not apply.
> +  checkValidity(input, true, apply, test.type == 'range'); // range has default minimum/maximum

See the comment for the _max test.

@@ +131,5 @@
> +      // it's default maximum is 100, its value would be 999, and it would
> +      // suffer from overflow (note, _not_ underflow as tested by
> +      // checkValidity, but still invalid). 
> +      // Now make it something that won't cause an error below:
> +      input.min = '0';

You could also just not set anything. this would be perfectly fine regarding 'range' always has a Range.

@@ +257,5 @@
>        break;
> +    case 'range':
> +      input.min = '0';
> +      input.value = '1';
> +      checkValidity(input, true, apply, apply);

Add the following test:
is(input.value, input.min, "the value should have been set to min");

@@ +266,5 @@
> +      input.value = 'foo';
> +      checkValidity(input, true, apply, apply);
> +
> +      input.value = '-1';
> +      checkValidity(input, true, apply, apply);

ditto

@@ +272,5 @@
> +      input.min = '-1';
> +      checkValidity(input, true, apply, apply);
> +
> +      input.value = '-42';
> +      checkValidity(input, true, apply, apply);

ditto

@@ +282,5 @@
> +      checkValidity(input, true, apply, true);
> +
> +      // We don't check the conversion of input.min to a double in
> +      // validationMessage for 'range' since range sanitizes, so there is no
> +      // validationMessage.

Same comment as for _max test.

::: content/html/content/test/forms/test_step_attribute.html
@@ +417,5 @@
>                                    "The nearest valid value is 9.",
>           "The validation message should not include the higher value.");
>        break;
> +    case 'range':
> +      // Range is special in that it sanitizes values, so it is much rarer for

s/sanitizes/clamps to valid/

@@ +423,5 @@
> +
> +      // When step=0, the allowed step is 1.
> +      input.step = '0';
> +      input.value = '1.2';
> +      checkValidity(input, true, apply);

Add the following test:
is(input.value, 1.0, "the value should have been set to the nearest valid value");

@@ +437,5 @@
> +      input.value = '1';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '1.5';
> +      checkValidity(input, true, apply, { low: 1, high: 2 });

You don't need that Object with low and high.

Also, add the following test:
is(input.value, 1.0, "the value should have been set to the nearest valid value");

@@ +441,5 @@
> +      checkValidity(input, true, apply, { low: 1, high: 2 });
> +
> +      // When step is negative, the allowed step value is 1.
> +      input.step = '-0.1';
> +      checkValidity(input, true, apply, { low: 1, high: 2 });

ditto: remove the Object and add the test.

@@ +449,5 @@
> +
> +      // When step is missing, the allowed step value is 1.
> +      input.removeAttribute('step');
> +      input.value = '1.5';
> +      checkValidity(input, true, apply, { low: 1, high: 2 });

ditto: object and test.

@@ +477,5 @@
> +      input.value = '3';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '2';
> +      checkValidity(input, true, apply, { low: 1, high: 3 });

ditto: object and test

@@ +485,5 @@
> +      input.value = '5.5';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '1';
> +      checkValidity(input, true, apply, { low: 0.5, high: 1.5 });

ditto: Object and test

@@ +493,5 @@
> +      input.value = '0.9';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '0.1';
> +      checkValidity(input, true, apply, { low: -0.1, high: 0.9 });

ditto: object and test

@@ +502,5 @@
> +      input.value = '1';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '0.5';
> +      checkValidity(input, true, apply, { low: 0, high: 1 });

ditto

@@ +509,5 @@
> +      input.value = '1';
> +      checkValidity(input, true, apply);
> +
> +      input.value = '0.5';
> +      checkValidity(input, true, apply, { low: 0, high: 1 });

ditto

@@ +515,5 @@
> +      input.removeAttribute('min');
> +
> +      // If value isn't a number, the element isn't invalid.
> +      input.value = '';
> +      checkValidity(input, true, apply);

Add:
is(input.value, 50, ...);

@@ +520,5 @@
> +
> +      // Regular situations.
> +      input.step = '2';
> +      input.value = '1.5';
> +      checkValidity(input, true, apply, { low: 0, high: 2 });

object and test

@@ +533,5 @@
> +      input.step = '2';
> +      input.removeAttribute('min');
> +      input.max = '10';
> +      input.value = '-9';
> +      checkValidity(input, true, apply, {low: -10, high: -8});

Add the test...
and at that point, just add a comment at the beginning of the black (case 'range') saying that you pass low/high when the value is clamped for information...

@@ +535,5 @@
> +      input.max = '10';
> +      input.value = '-9';
> +      checkValidity(input, true, apply, {low: -10, high: -8});
> +
> +      // If there is a value defined but no min, the step base is the value.

// If @value is defined but not min, the step base is @value.

@@ +565,5 @@
> +      input.step = '3';
> +      // no min!
> +      input.max = '1';
> +      input.defaultValue = '-1';
> +      checkValidity(input, false, apply, {low: -1, high: -1});

Could you add:

// <input type='range' max='5' step='3' min='4'>
input = getFreshElement(test.type);
input.min = 4;
input.max = 1;
input.step = 3;
checkValidity(input, false, apply, {low: 4, high: 4});

@@ +567,5 @@
> +      input.max = '1';
> +      input.defaultValue = '-1';
> +      checkValidity(input, false, apply, {low: -1, high: -1});
> +
> +      // Check that when the higher value is higher than max, we don't show it.

"don't show it" is confusing. You mean "don't clamp on it" or "don't round to it".

@@ +575,5 @@
> +      input.max = '10.9';
> +      input.value = '10';
> +
> +      is(input.validationMessage, "",
> +         "The validation message should be empty.");

Could you add before:
is(input.value, 9, "value should have been changed to the nearest valid value");

::: content/html/content/test/forms/test_stepup_stepdown.html
@@ +203,5 @@
> +    [ '1',  '1',    'foo', null,  null, '0',    false ],
> +    [ '1',  null,   '-10', null,  null, '0',    false ],
> +    [ '1',  null,   '0',   null,  null, '0',    false ],
> +    [ '1',  null,   '10',  null,  null, '10',   false ],
> +    [ '1',  null,   '2',   null,  null, '2',    false ],

I understand what you meant when you said this test is very different for type=range... when the value is set higher to @min, it is changed to be equal to min while other elements would simply keep the value and would not have changed it after the call to stepDown(). Interesting :)

@@ +411,5 @@
>          } else {
>            element.stepDown();
>          }
>  
> +        is(element.value, data[5], "The value for type=" + test.type + " should be " + data[5]);

Could you do the same change for stepUp() tests?

::: content/html/content/test/forms/test_valueasnumber_attribute.html
@@ +217,5 @@
> +  for (data of testData) {
> +    element.value = data[0];
> +
> +    // Given that NaN != NaN, we have to use null when the expected value is NaN.
> +    if (data[1] != null) {

I guess you can remove the code checking that data[1] is null ;)

::: content/html/content/test/test_bug590363.html
@@ +41,2 @@
>    // 'file' is treated separatly.
> +  // 'range' is treated separatly.

Actually, it's not really trested separatly. File is really treated separatly given that it's not part of the main loop. I think you could remove that comment.

@@ +54,5 @@
>      var e = document.createElement('input');
>      e.type = testData[i][0];
>  
>      var expectedValue;
> +    

nit: trailing whitespaces.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch with review comments addressed

(In reply to Mounir Lamouri (:mounir) from comment #4)
> However, I think we should create a test dedicated to the step/min/max
> rounding crazyness. For example, I wonder how we would handle that while
> parsing:
>   <input type='range' max='foo' min='bar' value='foobar' step='tulip'>
> Would .value have a different value depending on the order of those
> attributes? It should not IMO.
> 
> For the moment, I'm fine with landing this as-is (and likely the related
> code) and have a follow-up that takes care of testing this and fixing if
> needed.

Okay, I'll file a follow-up bug.

> I would prefer:
> // 'range' has a default minimum and maximum so it has a min and max by
> default.
> if (input.type == 'range') {
>   checkValidity(input, true, apply, true);
> } else {
>   checkValidity(input, true, apply, false);
> }

I tweaked the comment slightly, but made this change.

> > +      // We don't check the conversion of input.min to a double in
> > +      // validationMessage for 'range' since range sanitizes, so there is no
> > +      // validationMessage.
> 
> Same comment as for _max test.

Actually it's not possible for type=range to get underflow, so we can't get a validationMessage that will mention the min value as far as I can tell.

I'm afraid I went a bit mad on the test_step_attribute.html changes and made a lot more improvements than you requested. I'm not sure if you want to review that file again, but here it is before I check it in in case you do.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> (In reply to Mounir Lamouri (:mounir) from comment #4)
> > However, I think we should create a test dedicated to the step/min/max
> > rounding crazyness. For example, I wonder how we would handle that while
> > parsing:
> >   <input type='range' max='foo' min='bar' value='foobar' step='tulip'>
> > Would .value have a different value depending on the order of those
> > attributes? It should not IMO.
> > 
> > For the moment, I'm fine with landing this as-is (and likely the related
> > code) and have a follow-up that takes care of testing this and fixing if
> > needed.
> 
> Okay, I'll file a follow-up bug.

Filed bug 841941.

Comment 7

[Jonathan Watt [:jwatt]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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