Bug 15251 - REGRESSION: <input type=range> doesn't respond to form.reset() or setting input.value
Summary: REGRESSION: <input type=range> doesn't respond to form.reset() or setting inp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-09-21 11:31 PDT by Adam Roben (:aroben)
Modified: 2007-09-22 21:13 PDT (History)
1 user (show)

See Also:


Attachments
testcase (543 bytes, text/html)
2007-09-21 12:00 PDT, Adam Roben (:aroben)
no flags Details
potential fix (592 bytes, patch)
2007-09-21 12:07 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2007-09-21 11:31:23 PDT
Setting input.value for <input type=range> does not update the position of the slider thumb.
Comment 1 Adam Roben (:aroben) 2007-09-21 11:33:51 PDT
Hm, I can't reproduce this in a smaller testcase. Maybe it's a bug with the page I was working on...
Comment 2 Adam Roben (:aroben) 2007-09-21 11:39:58 PDT
Turns out I was wrong. This works! \o/
Comment 3 Adam Roben (:aroben) 2007-09-21 11:58:51 PDT
Ok, so this bug does happen, but only if you've dragged the thumb once. Attaching a testcase.
Comment 4 Adam Roben (:aroben) 2007-09-21 12:00:51 PDT
Created attachment 16341 [details]
testcase
Comment 5 Adam Roben (:aroben) 2007-09-21 12:07:28 PDT
Created attachment 16342 [details]
potential fix

So this patch seems to fix the bug, though I'd like Adele's opinion on whether this is the right way to go about it.
Comment 6 Adam Roben (:aroben) 2007-09-21 12:28:12 PDT
Adele said she didn't think the approach in my first patch was right, so I debugged a bit more, and I think I have found the real cause.

It looks as though the thumb's position will only get updated if HTMLInputElement::setChanged() is called at some point. When setting the value through JS, this happens in HTMLInputElement::parseMappedAttribute:

    } else if (attr->name() == valueAttr) {
        // We only need to setChanged if the form is looking at the default value right now.
        if (m_value.isNull())
            setChanged();
        setValueMatchesRenderer(false);

Note the check for if (m_value.isNull()). Normally, modifications to m_value only happen if HTMLInputElement::storesValueSeparateFromAttribute() returns true (which it does not for <input type=range>). So for an <input type=range>, we would expect m_value.isNull() to always be true, and therefore setChanged() to always be called when input.value is modified.

However, there is one place where m_value is set without first checking storesValueSeparateFromAttribute(): HTMLInputElement::setValueFromRenderer(). This method gets called from RenderSlider::setValueForPosition(), which is called when you drag the thumb. This sets m_value, so setChanged() is never subsequently called when input.value is set from JS.

It seems like RenderSlider really wants to set the value attribute directly, not HTMLInputElement::m_value.
Comment 7 Dave Hyatt 2007-09-21 13:30:34 PDT
"It seems like RenderSlider really wants to set the value attribute directly,
not HTMLInputElement::m_value."

I don't think so... you need to be able to reset a slider back to its default value.  The value attribute represents the default value that you will reset to (as well as the initial value).  If the user drags the slider thumb, then this should not change the default value.  It is just changing the current value.

If the user then resets the form, the slider should revert back to its default value.
Comment 8 Dave Hyatt 2007-09-21 13:33:26 PDT
The patch in this bug is absolutely correct, although you'll need to make sure Safari RSS doesn't break.

Comment 9 Dave Hyatt 2007-09-21 14:19:45 PDT
This is a regression from Safari 2.

Comment 10 Dave Hyatt 2007-09-21 14:23:14 PDT
<rdar://problem/5498169>
Comment 11 Adam Roben (:aroben) 2007-09-22 18:14:44 PDT
Looks like resetting via form.reset() is also broken.
Comment 12 Adam Roben (:aroben) 2007-09-22 21:13:26 PDT
Fixed in r25701