Set element value to its default value on reset

Ignore changing value of m_textAsOfLastFormControlChangeEvent during reset

On reset, m_textAsOfLastFormControlChangeEvent value is changed in setValue of HTMLInputElement and TextFieldInputType. 

Since reset sets value the value to null String, value of m_textAsOfLastFormControlChangeEvent also set to null string. Changed behavior to not change value of m_textAsOfLastFormControlChangeEvent, when in reset state. 

R=tkent, keishi
BUG=425408
TEST=fast/forms/text/text-reset-click-delete-text-change-event.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185914

[Habib Virji, 2014-10-28 11:01:23]

habib.virji@samsung.com changed reviewers:
+ vsevik@chromium.org

[Habib Virji, 2014-10-28 11:01:23]

PTAL.

[tkent, 2014-10-29 00:28:29]

https://codereview.chromium.org/679273004/diff/1/Source/core/html/HTMLInputEl...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:842: setValue(defaultValue());
This is a wrong fix.
Setting a null string to the value means the value refers to the default value. 
This change breaks the long-standing assumption.

[Habib Virji, 2014-10-29 08:43:53]

Default value will it be not the value of the value content attribute?

As per WHATWG: "The reset algorithm ... set the value of the element to the
value of the value content attribute, if there is one, or the empty string
otherwise"

My changes that's why uses defaultValue(), as observed defaultValue() returns
value of the value content and if no value is present, it returns an empty
string.

As raised in the bug, if reset value sets the null strings, and value content
attribute is deleted, it will be not able to trigger change event as previous
value(set by reset) is null string and current value is also a null string.

[tkent, 2014-10-31 02:08:21]

On 2014/10/29 08:43:53, Habib Virji wrote:
> Default value will it be not the value of the value content attribute?
> 
> As per WHATWG: "The reset algorithm ... set the value of the element to the
> value of the value content attribute, if there is one, or the empty string
> otherwise"
> 
> My changes that's why uses defaultValue(), as observed defaultValue() returns
> value of the value content and if no value is present, it returns an empty
> string.

You're right.  I didn't say the behavior of Patch Set 1 was wrong.  However, the
implementation of Patch Set 1 is wrong and I'm afraid it makes other
regressions.


> As raised in the bug, if reset value sets the null strings, and value content
> attribute is deleted, it will be not able to trigger change event as previous
> value(set by reset) is null string and current value is also a null string.

m_valueIfDirty must be a null string after reset() or just after HTML parsing. 
This is a long-standing assumption of WebKit and Blink.
I guess m_textAsOfLastFormControlChangeEvent is different in a case of reset()
and in a case of the HTML parsing.

[vsevik, 2014-11-03 07:46:01]

vsevik@chromium.org changed reviewers:
- vsevik@chromium.org

[Habib Virji, 2014-11-05 16:21:14]

"m_textAsOfLastFormControlChangeEvent is different in a case of reset() and in a
case of the HTML parsing."

--> During reset, setValue was changing value of
m_textAsOfLastFormControlChangeEvent. Have added an extra parameter isReset to
ignore setting value of m_textAsOfLastFormControlChangeEvent in case if it is
reset setValue call.

[tkent, 2014-11-06 00:11:17]

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1029: if (valueChanged && eventBehavior ==
DispatchNoEvent && !isReset)
Skipping setTextAsOfLastFormControlChangeEvent doesn't look correct.  What
should be set to m_extAsOfLastFormControlChangeEvent in the reset case?

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.h:111: void setValue(const String&,
TextFieldEventBehavior = DispatchNoEvent, bool isReset = false);
Please do not add new bool argument to a public function.
See #10 of http://www.chromium.org/blink/coding-style#TOC-Names

[Habib Virji, 2014-11-06 14:23:08]

Thanks tkent for reviewing. 

As described below, m_textAsOfLastFormControlChangeEvent is "" after reset. Thus
if value is changed after reset to "", it will skip change event. The change was
to not change value when in reset state.

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1029: if (valueChanged && eventBehavior ==
DispatchNoEvent && !isReset)
On 2014/11/06 00:11:16, tkent wrote:
> Skipping setTextAsOfLastFormControlChangeEvent doesn't look correct.  What
> should be set to m_extAsOfLastFormControlChangeEvent in the reset case?
<input type=text value=seed>
During HTMLParsing, value of m_textAsOfLastFormControlChangeEvent is set to
"seed", the value of the element. 

On reset, value of m_textAsOfLastFormControlChangeEvent is set to "", because
reset setValue to null string.

So when the element value after reset, if changed to "", although it  displays
"seed", but m_textAsOfLastFormControlChangeEvent value is "", so no change event
is discharged.  

I was trying to get value of m_textAsLastFromControlChangeEvent, not to change
during reset. Changing value of m_textAsOfLastFormControlChangeEvent, when no
change event is to be discharged, also looks bit unappropriate during reset.

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.h:111: void setValue(const String&,
TextFieldEventBehavior = DispatchNoEvent, bool isReset = false);
On 2014/11/06 00:11:16, tkent wrote:
> Please do not add new bool argument to a public function.
> See #10 of http://www.chromium.org/blink/coding-style#TOC-Names

Acknowledged.

[tkent, 2014-11-07 00:34:19]

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1029: if (valueChanged && eventBehavior ==
DispatchNoEvent && !isReset)
On 2014/11/06 14:23:08, Habib Virji wrote:
> On 2014/11/06 00:11:16, tkent wrote:
> > Skipping setTextAsOfLastFormControlChangeEvent doesn't look correct.  What
> > should be set to m_extAsOfLastFormControlChangeEvent in the reset case?
> <input type=text value=seed>
> During HTMLParsing, value of m_textAsOfLastFormControlChangeEvent is set to
> "seed", the value of the element.

Ok, HTMLTextFormControlElement::insertedInto() does it.

> On reset, value of m_textAsOfLastFormControlChangeEvent is set to "", because
> reset setValue to null string.
> 
> So when the element value after reset, if changed to "", although it  displays
> "seed", but m_textAsOfLastFormControlChangeEvent value is "", so no change
event
> is discharged.  
> 
> I was trying to get value of m_textAsLastFromControlChangeEvent, not to change
> during reset. Changing value of m_textAsOfLastFormControlChangeEvent, when no
> change event is to be discharged, also looks bit unappropriate during reset. 

Well, skipping setTextAsOfLastFormControlChangeEvent is wrong. If we skipped it,
it would have a value before reset().
m_textAsOfLastFormControlChangeEvent should be consistent with the parsing case.
 So, it should have defaultValue().
How about changing the next line to the following?
    setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ? value() :
sanitizedValue);

[Habib Virji, 2014-11-07 14:10:43]

Thanks suggestion does work. Upload changes accordingly.

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/20001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1029: if (valueChanged && eventBehavior ==
DispatchNoEvent && !isReset)
Thanks above suggested change does work and looks more appropriate. I have use
defaultValue() instead of value(), as that's the available function that gives
same value as parsing. Also the value in the textField sets to visibleValue().

[tkent, 2014-11-10 01:54:39]

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:842: setValue(String(), DispatchNoEvent);
This change is unnecessary.

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:192:
input->setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ?
visibleValue() : sanitizedValue);
Why is it visibleValue(), instead of defaultValue()?

[Habib Virji, 2014-11-13 15:00:56]

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:842: setValue(String(), DispatchNoEvent);
On 2014/11/10 01:54:39, tkent wrote:
> This change is unnecessary.

Acknowledged.

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:192:
input->setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ?
visibleValue() : sanitizedValue);
On 2014/11/10 01:54:39, tkent wrote:
> Why is it visibleValue(), instead of defaultValue()?

As defaultValue() and sanitizedValue both hold null string.

[tkent, 2014-11-14 00:23:56]

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:192:
input->setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ?
visibleValue() : sanitizedValue);
On 2014/11/13 15:00:56, Habib Virji wrote:
> On 2014/11/10 01:54:39, tkent wrote:
> > Why is it visibleValue(), instead of defaultValue()?
> 
> As defaultValue() and sanitizedValue both hold null string.   

Using visibleValue() is semantically incorrect.  If you need a non-null empty
string, use emptyString() like HTMLTextFormControlElement::insertedInto does.

[Habib Virji, 2014-11-14 10:05:26]

On 2014/11/14 00:23:56, tkent wrote:
>
https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
> File Source/core/html/forms/TextFieldInputType.cpp (right):
> 
>
https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
> Source/core/html/forms/TextFieldInputType.cpp:192:
> input->setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ?
> visibleValue() : sanitizedValue);
> On 2014/11/13 15:00:56, Habib Virji wrote:
> > On 2014/11/10 01:54:39, tkent wrote:
> > > Why is it visibleValue(), instead of defaultValue()?
> > 
> > As defaultValue() and sanitizedValue both hold null string.   
> 
> Using visibleValue() is semantically incorrect.  If you need a non-null empty
> string, use emptyString() like HTMLTextFormControlElement::insertedInto does.

Sorry was not clear, defaultValue holds similar value as sanitizedValue. So no
use of changing, if we want to get value of value attribute, it is not returned
via default value() but visible value() returns it

[tkent, 2014-11-17 01:29:22]

On 2014/11/14 10:05:26, Habib Virji wrote:
> > > > Why is it visibleValue(), instead of defaultValue()?
> > > 
> > > As defaultValue() and sanitizedValue both hold null string.   
> > 
> > Using visibleValue() is semantically incorrect.  If you need a non-null
empty
> > string, use emptyString() like HTMLTextFormControlElement::insertedInto
does.
> 
> Sorry was not clear, defaultValue holds similar value as sanitizedValue. So no
> use of changing, if we want to get value of value attribute, it is not
returned
> via default value() but visible value() returns it

My last comment is still valid.  Please don't use visibleValue() here. It's
incorrect even if the behavior is what you want.

[Habib Virji, 2014-11-24 11:05:32]

Thanks, updated to use defaultValue. Apologise for delay in updating.

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/679273004/diff/40001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:192:
input->setTextAsOfLastFormControlChangeEvent(sanitizedValue.isNull() ?
visibleValue() : sanitizedValue);
On 2014/11/14 00:23:56, tkent wrote:
> On 2014/11/13 15:00:56, Habib Virji wrote:
> > On 2014/11/10 01:54:39, tkent wrote:
> > > Why is it visibleValue(), instead of defaultValue()?
> > 
> > As defaultValue() and sanitizedValue both hold null string.   
> 
> Using visibleValue() is semantically incorrect.  If you need a non-null empty
> string, use emptyString() like HTMLTextFormControlElement::insertedInto does.

Done.

[tkent, 2014-11-25 00:50:27]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-11-25 00:50:28]

lgtm

[commit-bot: I haz the power, 2014-11-25 00:50:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/679273004/80001

[commit-bot: I haz the power, 2014-11-25 02:31:00]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185914