Bug 36259 – <input type=range> does not validate correctly without a renderer and the tests are incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Joseph Pecoraro]

Created attachment 50984 [details]
[TEST] - Shows Unexpected <input type=range> Behavior

The HTML5 Specification currently states:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html

For underflow (and likewise for overflow) part of the value sanitization algorithm:

> [[ When the element is suffering from an underflow, the user agent must set the
>   element's value to a valid floating point number that represents the minimum. ]]

I got clarification from Hixie that the range input, when stable, should never be
in the underflow / overflow state. IRC Log:

> Hixie: it can momentarily suffer from an underflow if you use script to set it
> to a value below the minimum. But that sentence means the steady state can't
> suffer from an underflow. if that makes sense

The spec says to run the "value sanitization algorithm" when (paraphrased):
  - an input element's type attribute changes state
  - when the element is first created
  - when the value content attribute is added, set, or removed

Some of the current tests in the tree do not exhibit correct behavior, and also
are providing incorrect values as expected values. To name a few:

  LayoutTests/fast/forms/ValidityState-rangeOverflow-range.html
  LayoutTests/fast/forms/ValidityState-rangeUnderflow-range.html

I believe just a little refactoring of code is needed. Informal test attached,
view the source to see expected values.

[Joseph Pecoraro]

Created attachment 50986 [details]
[PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange

This doesn't yet fix anything, but I don't want to do too many things at once (move code and make changes) so I making the move separate.

[WebKit Review Bot]

Attachment 50986 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/StepRange.h:32:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 50986 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/979001

[Kent Tamura]

Comment on attachment 50986 [details]
[PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 0c4cd0f..19491b2 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog

> +        Part 1: Refactoring the SliderRange struct out of rendering/RenderSlider
> +        into a more appropriate place. Changed the named to StepRange.

Would you explain what will be done in further patches please?

> +        * WebCore.xcodeproj/project.pbxproj: Added new files.

Please change other build files.

The code change looks good to me.

[Joseph Pecoraro]

(In reply to comment #4)
> > +        Part 1: Refactoring the SliderRange struct out of rendering/RenderSlider
> > +        into a more appropriate place. Changed the named to StepRange.
> 
> Would you explain what will be done in further patches please?

(In reply to comment #4)
> > +        Part 1: Refactoring the SliderRange struct out of rendering/RenderSlider
> > +        into a more appropriate place. Changed the named to StepRange.

Yes, I am going to start using StepRange inside HTMLInputElement to validate <input type=range> values instead of with the renderer. I guess this could have gone directly into HTMLInputElement, but I figured it would be generic and more manageable as a separate file. I may make small changes to the  StepRange struct which are much easier to notice in a separate patch instead of at the same time as a move.

> > +        * WebCore.xcodeproj/project.pbxproj: Added new files.
> 
> Please change other build files.

Whoops! Thanks for the reminder.


Kent TAMURA, I see you have done a lot with validation in the past. Would you agree that with the current spec rangeOverflow, rangeUnderflow, and stepMismatch should never be true with an <input type=range>? That seems to be where I am heading (removing the range specific tests completely). The spec has a lot of "must"s which are "may" in the date / time inputs you worked on.

> When the element is suffering from an underflow, the user agent must set the element's value to a valid floating point number that represents the minimum.

> When the element is suffering from an overflow, if the maximum is not less than the minimum, the user agent must set the element's value to a valid floating point number that represents the maximum.

> When the element is suffering from a step mismatch, the user agent must round the element's value to the nearest number for which the element would not suffer from a step mismatch, and which is greater than or equal to the minimum, and, if the maximum is not less than the minimum, which is less than or equal to the maximum.

[Kent Tamura]

(In reply to comment #5)
> Yes, I am going to start using StepRange inside HTMLInputElement to validate
> <input type=range> values instead of with the renderer. I guess this could have
> gone directly into HTMLInputElement, but I figured it would be generic and more
> manageable as a separate file. I may make small changes to the  StepRange
> struct which are much easier to notice in a separate patch instead of at the
> same time as a move.

Ok.  So we'll have just one more patch?

> Kent TAMURA, I see you have done a lot with validation in the past. Would you
> agree that with the current spec rangeOverflow, rangeUnderflow, and
> stepMismatch should never be true with an <input type=range>? That seems to be
> where I am heading (removing the range specific tests completely). The spec has
> a lot of "must"s which are "may" in the date / time inputs you worked on.

Yes, I agree with it.

[David Kilzer (:ddkilzer)]

Comment on attachment 50986 [details]
[PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange

r- based on Comment #4 and Comment #5.

[Joseph Pecoraro]

Created attachment 51082 [details]
[PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange

Handles other build files. Do I need to handle DerivedSources at all?

[Joseph Pecoraro]

Created attachment 51083 [details]
[PATCH] Part 2 - Move Sanitization to HTMLInputElement - Fix Incorrect Tests

The bulk of the work. The code in the new `clampValue` is copied from `valueFromElement` but it cannot easily be shared as intermediate values are used in the `valueFromElement` code. I could create a method with in/out parameters if desired. These changes end up fixing Bug 16990.

[WebKit Review Bot]

Attachment 51082 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/StepRange.h:32:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

(In reply to comment #6)
> Ok.  So we'll have just one more patch?

Yes. Just two patches total.

[Joseph Pecoraro]

(In reply to comment #10)
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/html/StepRange.h:32:  One space before end of line comments 
> [whitespace/comments] [5]

Ahhh. I thought this was whitespace at the end of a line. I see the problem now, it was in the copied code. I have a fix for this, but it is not necessary for uploading an extra patch for review.

[Joseph Pecoraro]

Created attachment 51118 [details]
[PATCH] Part 2 - Added ASSERTs, Fixed Style Issue in Part 1

Although I updated the tests to say that Range doesn't overflow / underflow, I still left the code in the path where it checks for an overflow / underflow. I moved RANGE out to the automatically false case, but I also put in an ASSERT just in case. This also updates some comments and fixes some style issues.

[Kent Tamura]

(In reply to comment #8)
> Created an attachment (id=51082) [details]
> [PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange
> 
> Handles other build files. Do I need to handle DerivedSources at all?

You don't need to update the DerivedSources stuff.
I don't have additional comments except the style error.  I'd give r+ if I was a reviewer.

[David Kilzer (:ddkilzer)]

Comment on attachment 51082 [details]
[PATCH] Part 1 - Refactor: Move SliderRange out and rename to StepRange

> diff --git a/WebCore/html/StepRange.cpp b/WebCore/html/StepRange.cpp
> new file mode 100644
> index 0000000..78f0365
> --- /dev/null
> +++ b/WebCore/html/StepRange.cpp
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Nit:  Update copyright to include 2010.

> +double sliderPosition(HTMLInputElement* element)
> +{
> +    StepRange range(element);
> +    return range.proportionFromValue(range.valueFromElement(element));
> +}

I think this method should remain in RenderSlider.cpp.  It's of no use outside that class.

> diff --git a/WebCore/html/StepRange.h b/WebCore/html/StepRange.h
> new file mode 100644
> index 0000000..7b2388e
> --- /dev/null
> +++ b/WebCore/html/StepRange.h
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Nit:  Update copyright to include 2010.

> +struct StepRange {

Just make it a class.  I don't see an advantage to leaving it a struct.

I would also inherit from non-copyable to make it explicit that we don't support copying these objects.

> +    double valueFromElement(HTMLInputElement*, bool* wasClamped = 0);

Again, I think this method should stay in RenderSlider.cpp as a static method.

+++ b/WebCore/rendering/RenderSlider.cpp

Update the copyright here, too.  :)

r=me with the above changes.

[Joseph Pecoraro]

(In reply to comment #14)
> You don't need to update the DerivedSources stuff.

Awesome. Can you give me a quick lesson on what the files are for and when I need to update them? I'd appreciate it!

> I don't have additional comments except the style error.

Part 2 actually fixes the style issue.

> I'd give r+ if I was a reviewer.

Thanks!

[Kent Tamura]

(In reply to comment #13)
> Created an attachment (id=51118) [details]
> [PATCH] Part 2 - Added ASSERTs, Fixed Style Issue in Part 1
> 
> Although I updated the tests to say that Range doesn't overflow / underflow, I
> still left the code in the path where it checks for an overflow / underflow. I
> moved RANGE out to the automatically false case, but I also put in an ASSERT
> just in case. This also updates some comments and fixes some style issues.

I recommend you post one patch per one bug.  Our commit-queue doesn't work well
with multiple patches in one bug, and having review comments for multiple
patches in one bug is confusing.

[Joseph Pecoraro]

(In reply to comment #17)
> I recommend you post one patch per one bug.  Our commit-queue doesn't work well
> with multiple patches in one bug, and having review comments for multiple
> patches in one bug is confusing.

Fixing the other bug is a side-affect of this patch. They aren't technically the same bug, but they are both both solved at the same time. Moving the code from RenderSlider to HTMLInputElement meant no-longer calling HTMLInputElement::setValueFromRenderer, which was what the other bug was about. This bug was about the fact that no validation happened unless the range input had a renderer.

Does this make sense? Is there a way I could rewrite the ChangeLog to better handle this?

Also, I have committer rights, so I can land this.

[David Kilzer (:ddkilzer)]

Comment on attachment 51118 [details]
[PATCH] Part 2 - Added ASSERTs, Fixed Style Issue in Part 1

> diff --git a/WebCore/html/HTMLInputElement.cpp b/WebCore/html/HTMLInputElement.cpp

Nit:  Update copyright.

> +    case RANGE: // Guarenteed by sanitization.
> +        ASSERT(parseToDouble(value(), nan) <= maximum());

Typo:  Guaranteed

> +    case RANGE: // Guarenteed by sanitization.
> +        ASSERT(parseToDouble(value(), nan) <= maximum());

Typo:  Guaranteed

> diff --git a/WebCore/html/StepRange.cpp b/WebCore/html/StepRange.cpp
> +double StepRange::clampValue(const String& stringValue)
> +{
> +    double oldValue;

I would call this value since "oldValue" implies a "newValue", and we don't have one of those here.

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (In reply to comment #17)
> > I recommend you post one patch per one bug.  Our commit-queue doesn't work well
> > with multiple patches in one bug, and having review comments for multiple
> > patches in one bug is confusing.
> 
> Fixing the other bug is a side-affect of this patch. They aren't technically
> the same bug, but they are both both solved at the same time. Moving the code
> from RenderSlider to HTMLInputElement meant no-longer calling
> HTMLInputElement::setValueFromRenderer, which was what the other bug was about.
> This bug was about the fact that no validation happened unless the range input
> had a renderer.
> 
> Does this make sense? Is there a way I could rewrite the ChangeLog to better
> handle this?

Prefix one ChangeLog with "Part 1 of 2:" and the other with "Part 2 of 2:".

I don't see why you need another bug for a series of two patches in this particular case.  It's easier to review the patches separately anyway.

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > I recommend you post one patch per one bug.  Our commit-queue doesn't work well
> > > with multiple patches in one bug, and having review comments for multiple
> > > patches in one bug is confusing.
> > 
> > Fixing the other bug is a side-affect of this patch. They aren't technically
> > the same bug, but they are both both solved at the same time. Moving the code
> > from RenderSlider to HTMLInputElement meant no-longer calling
> > HTMLInputElement::setValueFromRenderer, which was what the other bug was about.
> > This bug was about the fact that no validation happened unless the range input
> > had a renderer.
> > 
> > Does this make sense? Is there a way I could rewrite the ChangeLog to better
> > handle this?
> 
> Prefix one ChangeLog with "Part 1 of 2:" and the other with "Part 2 of 2:".
> 
> I don't see why you need another bug for a series of two patches in this
> particular case.  It's easier to review the patches separately anyway.

To be more specific, if the second patch required more discussion, it would be nice to have a second bug to do that in.  If it doesn't require additional discussion, I think it's fine to leave it on this bug.

[Joseph Pecoraro]

Part 1:
Committed r56241
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	A	WebCore/html/StepRange.h
	A	WebCore/html/StepRange.cpp
	M	WebCore/rendering/RenderSlider.cpp
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r56241 = bc6f7e657c47f2c7efb04a3a4e08d90fdc7fd1ae (trunk)
http://trac.webkit.org/changeset/56241

Part 2:
Committed r56242
	M	WebCore/ChangeLog
	M	WebCore/html/StepRange.h
	M	WebCore/html/HTMLInputElement.cpp
	M	WebCore/html/StepRange.cpp
	M	WebCore/rendering/RenderSlider.cpp
	M	LayoutTests/fast/forms/ValidityState-rangeOverflow-expected.txt
	M	LayoutTests/fast/forms/script-tests/ValidityState-rangeOverflow.js
	M	LayoutTests/fast/forms/script-tests/validationMessage.js
	M	LayoutTests/fast/forms/script-tests/ValidityState-rangeUnderflow.js
	M	LayoutTests/fast/forms/script-tests/input-stepup-stepdown.js
	M	LayoutTests/fast/forms/range-reset.html
	A	LayoutTests/fast/forms/range-input-dynamic-oninput-expected.txt
	A	LayoutTests/fast/forms/range-input-dynamic-oninput.html
	M	LayoutTests/fast/forms/ValidityState-rangeUnderflow-expected.txt
	M	LayoutTests/fast/forms/validationMessage-expected.txt
	M	LayoutTests/fast/forms/input-stepup-stepdown-expected.txt
	M	LayoutTests/fast/forms/range-reset-expected.txt
	M	LayoutTests/ChangeLog
r56242 = 1569c5e33346f0e1681d92a925be02480504e8d5 (trunk)
http://trac.webkit.org/changeset/56242

Bots look good, no-one failed to build or failed the test changes!

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> Bots look good, no-one failed to build or failed the test changes!

Leopard Debug Test Bot is crashing on an assertion failure.  See Bug 36376.