841941 - Create a test for the rounding crazyness of <input type=range>

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Follow-up from bug 836323 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.

We have quite a lot of test coverage for this in test_step_attribute.html and test_stepup_stepdown.html, so I'm not exactly sure how we'd decide what goes in which file.

Comment 1

[Mounir Lamouri (:mounir)]

Indeed, test_step_attribute.html and test_stepup_stepdown.html should cover a lot of stuff but I'm worried that at parsing time we might have some inconsistency: if you have <input type='range' value='x' min='y' max='z' step='alpha'>, we should always have the same output whatever the order of the attributes is. I mostly want to test that.

Comment 2

[Jonathan Watt [:jwatt]]

Hmmm...

If in script someone did:

  let input = document.createElement('input');
  input.max = 49.9; // value -> 49
  input.step = 0.1; // value = 49

the result would be 49. But if they reversed the order in which @max and @step are set:

  let input = document.createElement('input');
  input.step = 0.1; // value = 50
  input.max = 49.9; // value -> 49.9

then the order would be 49.9.

And I think that's okay, or at least unavoidable. It's just the nature of how range has to work really.

For markup, I agree it would suck if the order of the attributes in the markup mattered.

One way to prevent that would be to have a callback from the parser that informs the element that all the attributes in the markup have now been set. It could then call SetValueInternal with the @value value, or else the default value. Offhand I'm not sure if such a callback exists though.

Comment 3

[Mounir Lamouri (:mounir)]

You can use :DoneCreatingElement() which is the callback called by the parser when the element is created.
mParserCreating let you know that you are being created by the parser.

What I would try is the following:
- move the value changing when invalid from ::SanitizeValue() to the max/min/step error handling;
- don't do any value change if mParserCreating is true;
- :DoneCreatingElement() sanitizes the value so hopefully it will just set the value to the correct one.

Comment 4

[Jonathan Watt [:jwatt]]

Excellent, sounds good. I'll come back to this after the layout and thumb dragging patches are landed (to get eyes on the basic implementation sooner rather than later).

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> 
> What I would try is the following:
> - move the value changing when invalid from ::SanitizeValue() to the
> max/min/step error handling;

This part doesn't seem to make sense, BTW. If you still feel this is correct, can you clarify exactly what you mean, and why we would do this?

> - don't do any value change if mParserCreating is true;

I think I've done this sufficiently, since the pre-existing mParserCreating checks seem to do what we need.

Comment 7

[Mounir Lamouri (:mounir)]

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

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

Please, remove the C++ code (that isn't needed to pass the tests) and fix the tests ;)

::: content/html/content/test/forms/test_input_range_attr_order.html
@@ +37,5 @@
> +
> +function test() {
> +  var ranges = document.querySelectorAll("input[type=range]");
> +  for (var i = 0; i < ranges.length; i++) {
> +    is(ranges.item(0).value, "1.5", "Check sanitization order for range " + i);

I guess that should be: ranges.item(i)

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #7)
> I guess that should be: ranges.item(i)

Good catch!

Comment 10

[Mounir Lamouri (:mounir)]

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

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

r=me, assuming that the tests values are chosen such as if they were applied in a specific order, the tests would fail.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> r=me, assuming that the tests values are chosen such as if they were applied
> in a specific order, the tests would fail.

Indeed. If in script someone did:

  let input = document.createElement('input');
  input.value = 2;
  input.max = 1.5;  // value -> 1
  input.step = 0.5; // value -> 1

Whereas if they reversed the order in which @max and @step are set:

  let input = document.createElement('input');
  input.value = 2;
  input.step = 0.5; // value -> 2
  input.max = 1.5;  // value -> 1.5

Comment 12

[Jonathan Watt [:jwatt]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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