Set value of m_textAsOfLastFormControlChangeEvent before triggering change event

Set value of m_textAsOfLastFormControlChangeEvent before triggering change event

Value updated inside change event, get overwritten by setTextAsOfLastFormControlChangeEvent.

New value if similar to previous value was not triggering change event.

R=tkent, keishi
BUG=378871
TEST=OnChange event should be fired, when value is changed inside change
handler and same text is re-entered.

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

[Habib Virji, 2014-06-04 10:17:27]

This patch fixes the issue with the value changed inside the change event
handler. 

Scenario it fixes is:
1. Value A set inside text field.
2. Inside change event handler, value changed to ' '
3. After execution of dispatchChangeEvent, 
setTextAsOfLastFormControlChangeEvent stores value as A not as ' '.
4. Again when value A is entered but m_textAsOfLastFormControlChangeEvent is A,
change event is not triggered.

This happens due to inconsistency created with
m_textAsOfLastFormControlChangeEvent value. This patch sets the value of setText
before calling change event to avoid above scenario.

[keishi, 2014-06-04 20:43:53]

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:14: log('change event fired');
nit: inconsistent string literal quotes. You used double quotes before.

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:28: document.execCommand("InsertText",
false, "foo bar baz");
This test is not reproducing the bug.
InsertText just inserts the text at the caret so the value becomes " foo bar
baz" and m_textAsOfLastFormControlChangeEvent is "foo bar baz".

[keishi, 2014-06-04 21:26:21]

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:3: <input id="input" type="text"
onchange="changeHandler()"></input>
nit: input doesn't need end tag in html

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:15:
document.getElementById('input').value = ' ';
nit: use input variable

[Habib Virji, 2014-06-05 08:43:22]

@kieshi: What I wanted to simulate is user input and when out of focus, change
event is triggered as value is changed inside change event handler. 

To simulate user input, I have used inserttext. So basic behavior expected from
this test is inserttext not to generate event but blur to generate event. Let me
know if it is okay.

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:3: <input id="input" type="text"
onchange="changeHandler()"></input>
On 2014/06/04 21:26:22, keishi wrote:
> nit: input doesn't need end tag in html

Done.

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:14: log('change event fired');
On 2014/06/04 20:43:54, keishi wrote:
> nit: inconsistent string literal quotes. You used double quotes before.

Done.

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:28: document.execCommand("InsertText",
false, "foo bar baz");
On 2014/06/04 20:43:54, keishi wrote:
> This test is not reproducing the bug.
> InsertText just inserts the text at the caret so the value becomes " foo bar
> baz" and m_textAsOfLastFormControlChangeEvent is "foo bar baz".

Not sure, what I am trying to do with test is put same text as before change
event to see if change event triggers when blur is called.  Without the change
on second blur, change event is not fired. 

In the bug too out of focus is used to simulate.

[keishi, 2014-06-05 09:02:16]

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:28: document.execCommand("InsertText",
false, "foo bar baz");
On 2014/06/05 08:43:23, Habib Virji wrote:
> On 2014/06/04 20:43:54, keishi wrote:
> > This test is not reproducing the bug.
> > InsertText just inserts the text at the caret so the value becomes " foo bar
> > baz" and m_textAsOfLastFormControlChangeEvent is "foo bar baz".
> 
> Not sure, what I am trying to do with test is put same text as before change
> event to see if change event triggers when blur is called.  Without the change
> on second blur, change event is not fired. 
> 
> In the bug too out of focus is used to simulate. 

I tried this test on Chrome Canary and it passes so this test isn't triggering
the bug.
In order to trigger the bug, input.value after calling InsertText needs to
match.
The input.value after line 24 is "foo bar baz" but the input.value after line 28
is " foo bar baz" (notice the space before foo).

You should set the value to "" (empty string) inside changeHandler() in order to
fix this (just like the original bug).

[Habib Virji, 2014-06-05 09:33:30]

Thanks Keishi, I have updated test to now check strings at all points. Last
changeEventCounter should fail in latest canary and with the patch will pass.

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/1/LayoutTests/fast/events/onch...
LayoutTests/fast/events/onchange-js.html:28: document.execCommand("InsertText",
false, "foo bar baz");
On 2014/06/05 09:02:17, keishi wrote:
> On 2014/06/05 08:43:23, Habib Virji wrote:
> > On 2014/06/04 20:43:54, keishi wrote:
> > > This test is not reproducing the bug.
> > > InsertText just inserts the text at the caret so the value becomes " foo
bar
> > > baz" and m_textAsOfLastFormControlChangeEvent is "foo bar baz".
> > 
> > Not sure, what I am trying to do with test is put same text as before change
> > event to see if change event triggers when blur is called.  Without the
change
> > on second blur, change event is not fired. 
> > 
> > In the bug too out of focus is used to simulate. 
> 
> I tried this test on Chrome Canary and it passes so this test isn't triggering
> the bug.
> In order to trigger the bug, input.value after calling InsertText needs to
> match.
> The input.value after line 24 is "foo bar baz" but the input.value after line
28
> is " foo bar baz" (notice the space before foo).
> 
> You should set the value to "" (empty string) inside changeHandler() in order
to
> fix this (just like the original bug).

Thanks Keishi, I have updated test to now check strings at all points. Last
changeEventCounter should fail in latest canary and with the patch will pass.

[keishi, 2014-06-05 09:42:01]

LGTM

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/onchange-js.html:15: input.value = "";
nit: only place using double quotes

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/onchange-js.html:19: testRunner.dumpAsText();
nit: dumpAsText is called in js-test.js so you can remove this.

[Habib Virji, 2014-06-05 10:18:14]

Thanks Keishi. Done.

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/onchange-js.html (right):

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/onchange-js.html:15: input.value = "";
On 2014/06/05 09:42:02, keishi wrote:
> nit: only place using double quotes

Done.

https://codereview.chromium.org/313013005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/onchange-js.html:19: testRunner.dumpAsText();
On 2014/06/05 09:42:02, keishi wrote:
> nit: dumpAsText is called in js-test.js so you can remove this.

Done.

[Habib Virji, 2014-06-05 10:18:20]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-06-05 10:19:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/habib.virji@samsung.com/313013005/60001

[commit-bot: I haz the power, 2014-06-05 12:52:41]

Change committed as 175565