Bug 61857 - Value of a range input does not update visually when using setAttribute
Summary: Value of a range input does not update visually when using setAttribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-01 06:37 PDT by Keishi Hattori
Modified: 2011-06-15 22:16 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.12 KB, patch)
2011-06-01 06:42 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed patch (5.55 KB, patch)
2011-06-01 19:07 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed patch #2 (5.46 KB, patch)
2011-06-01 19:12 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
hasDirtyValue, better test (5.46 KB, patch)
2011-06-01 21:54 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
new patch (4.09 KB, patch)
2011-06-08 00:49 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
final patch (4.41 KB, patch)
2011-06-08 01:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-06-01 06:37:20 PDT
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.
Comment 1 Keishi Hattori 2011-06-01 06:42:05 PDT
Created attachment 95592 [details]
patch
Comment 2 Keishi Hattori 2011-06-01 06:43:13 PDT
Sorry wrong chromium bug.
http://code.google.com/p/chromium/issues/detail?id=73541
Comment 3 Dimitri Glazkov (Google) 2011-06-01 09:30:51 PDT
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.
Comment 4 Kent Tamura 2011-06-01 18:24:19 PDT
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.
Comment 5 Keishi Hattori 2011-06-01 19:07:17 PDT
Created attachment 95701 [details]
fixed patch
Comment 6 Keishi Hattori 2011-06-01 19:10:54 PDT
Comment on attachment 95701 [details]
fixed patch

I shouldn't have put the setNeedsStyleRecalc inside the if
Comment 7 Keishi Hattori 2011-06-01 19:12:40 PDT
Created attachment 95703 [details]
fixed patch #2
Comment 8 Kent Tamura 2011-06-01 19:33:45 PDT
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.
Comment 9 Keishi Hattori 2011-06-01 21:54:18 PDT
Created attachment 95719 [details]
hasDirtyValue, better test
Comment 10 Keishi Hattori 2011-06-01 22:04:32 PDT
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.
Comment 11 Kent Tamura 2011-06-01 22:15:30 PDT
Comment on attachment 95719 [details]
hasDirtyValue, better test

ok, I understand.
Thank you for fixing this.
Comment 12 Alexey Proskuryakov 2011-06-01 23:58:33 PDT
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?
Comment 13 Keishi Hattori 2011-06-02 00:09:07 PDT
(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?
Comment 14 Alexey Proskuryakov 2011-06-02 00:20:08 PDT
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?
Comment 15 Keishi Hattori 2011-06-02 00:24:29 PDT
(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
Comment 16 Kent Tamura 2011-06-02 00:28:39 PDT
(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.
Comment 17 Alexey Proskuryakov 2011-06-02 00:46:44 PDT
> 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?
Comment 18 Kent Tamura 2011-06-02 01:21:29 PDT
(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.
Comment 19 Kent Tamura 2011-06-02 01:29:25 PDT
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.
Comment 20 Alexey Proskuryakov 2011-06-02 08:46:43 PDT
> > > 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">.
Comment 21 Alexey Proskuryakov 2011-06-02 08:54:22 PDT
> > 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."
Comment 22 Kent Tamura 2011-06-02 15:13:22 PDT
(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.
Comment 23 Alexey Proskuryakov 2011-06-02 17:52:15 PDT
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.
Comment 24 Keishi Hattori 2011-06-02 20:59:33 PDT
> 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.
Comment 25 Keishi Hattori 2011-06-08 00:49:07 PDT
Created attachment 96392 [details]
new patch
Comment 26 Kent Tamura 2011-06-08 00:53:33 PDT
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.
Comment 27 Keishi Hattori 2011-06-08 01:03:28 PDT
Created attachment 96394 [details]
final patch
Comment 28 Kent Tamura 2011-06-15 21:45:59 PDT
Don't you need review?
Comment 29 Keishi Hattori 2011-06-15 22:16:48 PDT
Thanks. Landed it myself.

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

All reviewed patches have been landed.  Closing bug.