Bug 61857 – Value of a range input does not update visually when using setAttribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

http://code.google.com/p/chromium/issues/detail?id=77779

Value of <range> doesn't change when you setAttribute a new value even when it is not dirty. This only happens when the range has a min/max attribute.

[Keishi Hattori]

Created attachment 95592 [details]
patch

[Keishi Hattori]

Sorry wrong chromium bug.
http://code.google.com/p/chromium/issues/detail?id=73541

[Dimitri Glazkov (Google)]

Comment on attachment 95592 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95592&action=review

> LayoutTests/fast/forms/script-tests/range-set-attribute.js:1
> +description('Test to see if setting the value attribute updates the value.');

Please move this code into range-set-attribute.html. There is no need to create separate files and make tests harder to read.

> Source/WebCore/html/HTMLInputElement.cpp:979
> +void HTMLInputElement::setValue(const String& value, bool sendChangeEvent, bool onlyIfDirty)

Boolean parameters are bad, bad, bad. That's right: three times bad. At the callsite, we have no idea what is happening. If you are going to modify this function signature, I would convert both parameters into enums as cleanup.

> Source/WebCore/html/HTMLInputElement.h:137
> +    void setValue(const String&, bool sendChangeEvent = false, bool onlyIfDirty = false);

onlyIfDirty is confusing to me. What's clean? What's dirty? It's a flag of some sort, but I don't understand why it is here. The ChangeLog doesn't explain it either.

[Kent Tamura]

Comment on attachment 95592 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95592&action=review

> LayoutTests/fast/forms/script-tests/range-set-attribute.js:19
> +// value attribute should not change the calue after you set a value

Typo: calue

> Source/WebCore/html/RangeInputType.cpp:245
> -    element()->setValue(element()->value());
> +    element()->setValue(element()->value(), false, true);

In this case, I think adding HTMLInputElement::isDirty() and modify this line as if (element()->isDirty())element()->setValue(...) is better.

[Keishi Hattori]

Created attachment 95701 [details]
fixed patch

[Keishi Hattori]

Comment on attachment 95701 [details]
fixed patch

I shouldn't have put the setNeedsStyleRecalc inside the if

[Keishi Hattori]

Created attachment 95703 [details]
fixed patch #2

[Kent Tamura]

Comment on attachment 95703 [details]
fixed patch #2

View in context: https://bugs.webkit.org/attachment.cgi?id=95703&action=review

> LayoutTests/fast/forms/range-set-attribute.html:23
> +// rewriting the value attribute should update the value
> +input.setAttribute('value', '20');
> +shouldBe('input.value', '"20"');

I don't understand why this patch fixes this case.  Would you explain it please?

> Source/WebCore/html/HTMLInputElement.h:142
> +    bool isDirtyValue() const;

I'm sorry, hasDirtyValue() is better.

[Keishi Hattori]

Created attachment 95719 [details]
hasDirtyValue, better test

[Keishi Hattori]

The first setAttribute("value") should be before setting the min/max, so I fixed that. 

> > +// rewriting the value attribute should update the value
> > +input.setAttribute('value', '20');
> > +shouldBe('input.value', '"20"');
>
> I don't understand why this patch fixes this case.  Would you explain it please?

Right now setting min/max attributes causes the element to have dirty values. The test will fail with an input.value of 10. (For "fixed patch #2" input.value would be 50)

This patches fixes this, so as long as you don't set the input.value, you can change the value through setAttribute.

[Kent Tamura]

Comment on attachment 95719 [details]
hasDirtyValue, better test

ok, I understand.
Thank you for fixing this.

[Alexey Proskuryakov]

Comment on attachment 95719 [details]
hasDirtyValue, better test

+bool HTMLInputElement::hasDirtyValue() const
+{
+    return !m_value.isNull();
+}

Could you please explain why any value is dirty value?

[Keishi Hattori]

(In reply to comment #12)
> (From update of attachment 95719 [details])
> +bool HTMLInputElement::hasDirtyValue() const
> +{
> +    return !m_value.isNull();
> +}
> 
> Could you please explain why any value is dirty value?

Basically an input control element becomes a dirty value once the user interacts  with it. 

The "value" attribute is the initial value, so setAttribute("value", "bar") will change the value while the value is clean. But after the user moves the slider, setAttribute("value", "bar") won't work anymore.

http://www.w3.org/TR/html5/the-input-element.html#concept-input-value-dirty-flag

Do I need more explanation in the changelog?

[Alexey Proskuryakov]

I still don't understand. For <input value="test">, m_value is not null, but it's obviously not dirty until the user modifies it. Am I missing something?

[Keishi Hattori]

(In reply to comment #14)
> I still don't understand. For <input value="test">, m_value is not null, but it's obviously not dirty until the user modifies it. Am I missing something?

If the element is clean m_value is null and HTMLInputElement::value takes the value from the "value" atribute.
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L935

[Kent Tamura]

(In reply to comment #14)
> I still don't understand. For <input value="test">, m_value is not null, but it's obviously not dirty until the user modifies it. Am I missing something?

In this case, m_value is null.
m_value becomes non-null when the user modifies it or JavaScript code update HTMLInputElement::value.

[Alexey Proskuryakov]

> In this case, m_value is null.
> m_value becomes non-null when the user modifies it or JavaScript code update HTMLInputElement::value.

Having an m_value data member that doesn't necessarily contain the value is incredibly confusing. Is this a recent change?

You say that m_value becomes non-null when JavaScript code modifies it. But according to HTML5, the dirty flag is only set when it's the user modifying the input. So, is the function added in this patch still wrong?

[Kent Tamura]

(In reply to comment #17)
> > In this case, m_value is null.
> > m_value becomes non-null when the user modifies it or JavaScript code update HTMLInputElement::value.
> 
> Having an m_value data member that doesn't necessarily contain the value is incredibly confusing. Is this a recent change?

?? I don't catch the first sentence.  m_value contains the value.
m_value's meaning have never been changed at least for two years.


> 
> You say that m_value becomes non-null when JavaScript code modifies it. But according to HTML5, the dirty flag is only set when it's the user modifying the input. So, is the function added in this patch still wrong?

The implementation and my explanation in Comment #16 are correct, and the first sentence in Keishi's Comment #13 was incomplete.

[Kent Tamura]

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#dom-input-value-value
> On getting, it must return the current value of the element. On setting, it must set the element's value to the new value, set the element's dirty value flag to true, and then invoke the value sanitization algorithm, if the element's type attribute's current state defines one.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#concept-input-value-dirty-flag
> Each input element has a boolean dirty value flag. The dirty value flag must be initially set to false when the element is created, and must be set to true whenever the user interacts with the control in a way that changes the value.

[Alexey Proskuryakov]

> > > In this case, m_value is null.

> ?? I don't catch the first sentence.  m_value contains the value.

You just said that m_value was null, even though element's value was "test". The example we are talking about is <input value="test">.

[Alexey Proskuryakov]

> > On setting, it must set the element's value to the new value, set the element's dirty value flag to true, and then invoke the value sanitization algorithm, if the element's type attribute's current state defines one.

The test doesn't ever change HTMLInputElement value property. It only tests setAttribute, which is different.

FWIW, the two spec quotations you gave do appear to contradict each other. I've submitted a spec review comment.

For content attribute that's tested here, the spec says the following:

"The value content attribute gives the default value of the input element. When the value content attribute is added, set, or removed, if the control's dirty value flag is false, the user agent must set the value of the element to the value of the value content attribute, if there is one, or the empty string otherwise, and then run the current value sanitization algorithm, if one is defined."

[Kent Tamura]

(In reply to comment #20)
> You just said that m_value was null, even though element's value was "test". The example we are talking about is <input value="test">.

Le's call the content of HTML 'value' attribute 'default value', and call the value edited by a user or set by JavaScript 'dirty value'.
m_value always represents the dirty value, and never represents the default value.
HTMLInputElement::value() returns m_value if m_value is not null, or the default value otherwise.

(In reply to comment #21)
> The test doesn't ever change HTMLInputElement value property. It only tests setAttribute, which is different.

Yes.  We'd like to fix the bug about setAttribute case.

[Alexey Proskuryakov]

Comment on attachment 95719 [details]
hasDirtyValue, better test

I don't understand why you are saying this. We should not rename data members in our heads, they should have good names!

From your description, it appears that m_value might need to be renamed to m_valueIfDirty. Let's explore that before landing this patch, because in the current state, it makes the code strictly worse. Marking r- to that effect.

[Keishi Hattori]

> From your description, it appears that m_value might need to be renamed to m_valueIfDirty. Let's explore that before landing this patch, because in the current state, it makes the code strictly worse. Marking r- to that effect.

I agree with changing the variable name. 

I filed Bug 61990 and uploaded a patch. I added the HTMLInputELement::hasDirtyValue() in that patch because it makes it easier to understand what dirty means.

[Keishi Hattori]

Created attachment 96392 [details]
new patch

[Kent Tamura]

Comment on attachment 96392 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96392&action=review

> LayoutTests/fast/forms/range-set-attribute.html:24
> +// rewriting the value attribute should update the value
> +input.setAttribute('value', '20');
> +shouldBe('input.value', '"20"');

You had better print this with debug() for test result readability, or
show what is happening by shouldBe('input.setAttribute("value", "20"); input.value', '"20"');

> LayoutTests/fast/forms/range-set-attribute.html:26
> +// changing the max should effect value

ditto.

> LayoutTests/fast/forms/range-set-attribute.html:30
> +// value attribute should not change the value after you set a value

ditto.

[Keishi Hattori]

Created attachment 96394 [details]
final patch

[Kent Tamura]

Don't you need review?

[Keishi Hattori]

Thanks. Landed it myself.

Committed r89006: <http://trac.webkit.org/changeset/89006>

All reviewed patches have been landed.  Closing bug.