Value set in onblur should not keep element in focus

Value set in onblur should not keep element in focus

Value set in onblur was resulting in element still in focus due to reset
of the layout called on setValue. This patch fixes on reset of layout to remove
focus.

R=tkent, keishi1
BUG=347684
TEST=Background color differ when on blur value is set.

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

[Habib Virji, 2014-03-10 10:42:04]

Setting value results in layout reset and resetFields() resets m_fieldOwner,
which result in onblur failing. 

This patch removes focus before resetting the field.

[tkent, 2014-03-11 04:41:58]

Would you explain why EditControlOwner::didBlurFromControl is not called (or
extra didFocusOnField is called)?

https://codereview.chromium.org/191293011/diff/1/Source/core/html/shadow/Date...
File Source/core/html/shadow/DateTimeEditElement.cpp (right):

https://codereview.chromium.org/191293011/diff/1/Source/core/html/shadow/Date...
Source/core/html/shadow/DateTimeEditElement.cpp:704: didBlurFromField();
I don't think this works fine.  This looks we loose :focus style whenever the
value is changed.

[Habib Virji, 2014-03-11 11:46:32]

"Would you explain why EditControlOwner::didBlurFromControl is not called (or
extra didFocusOnField is called)?

It  never reaches didBlurFromControl when value is set inside onblur. Setting
value results in resetField, which calls removeEventHandler (Which is bit
misleading as it reset m_fieldOwner). When didBlur is called m_fieldOwner
condition fails and didBlurFromField is not invoked.

[Habib Virji, 2014-03-11 17:27:29]

On 2014/03/11 04:41:58, tkent wrote:
> Would you explain why EditControlOwner::didBlurFromControl is not called (or
> extra didFocusOnField is called)?
> 
>
https://codereview.chromium.org/191293011/diff/1/Source/core/html/shadow/Date...
> File Source/core/html/shadow/DateTimeEditElement.cpp (right):
> 
>
https://codereview.chromium.org/191293011/diff/1/Source/core/html/shadow/Date...
> Source/core/html/shadow/DateTimeEditElement.cpp:704: didBlurFromField();
> I don't think this works fine.  This looks we loose :focus style whenever the
> value is changed.

Got to the same impression first, but as callflow first sets blur (i.e calls
resetField from build), then layout again sets back focus
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
if still in same field else fails.

[Habib Virji, 2014-03-11 17:31:20]

Combined above comments:
"Would you explain why EditControlOwner::didBlurFromControl is not called (or
extra didFocusOnField is called)?"
It never reaches didBlurFromControl when value is set inside onblur. Setting
value results in resetField, which calls removeEventHandler (Which is bit
misleading as it reset m_fieldOwner). When didBlur is called m_fieldOwner
condition fails and didBlurFromField is not invoked.

"resetField change:"
Got to the same impression first, but as callflow first sets blur (i.e calls
resetField from build), then layout again sets back focus
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
if still in same field else it fails.

[tkent, 2014-03-12 00:22:28]

Thanks.  I understand the situation.

I still think Patch Set 1 is not good.  I think PatchSet 1 produces unnecessary
'blur' and 'focus' events when the value is updated while the INPUT is focused. 
Also, the code doesn't look reasonable even if it works correctly.

IMO, it's not good that DateTimeFieldElement relies on 'blur' and 'focus'
events.  These events are delivered to the element after JavaScript handlers are
executed, and the handlers might change the element status like this bug.  I'd
like to change the code so that DateTimeFieldElement overrides setFocus(bool),
and remove blur/focus handling in DateTimeFieldElement::defaultEventHandler.

[Habib Virji, 2014-03-17 16:07:53]

An update, still investigating. 

This is my current finding till now for using setFocus instead of
didBlur/didFocus:

- BaseMultipleFieldsDateAndTime has handleFocusEvent, which is called from
setFocused and this allows individual field to be focused but setFocus is called
after handleFocusEvent. Since shadow element are already focused, element focus
is not done. I am investigating possibility of setting setFocus before setting
individual field in handleFocusEvent.

- In current code, didFocus does results in calling of setFocus. Reason probably
for using didBlur/didFocus in defaultHandler was setFocus override function is
called after setFocused and setting it before field focus is a right way.

[Habib Virji, 2014-03-27 16:20:59]

Apologies it took a long time to work out the suggestion.

Was missing ContainerNode::setFocus call in override setFocus and was seeing all
kinds of issue. 

Also changes in DateFieldNumericFieldElement to override setFocus as it was
relying on didBlur.

[tkent, 2014-03-31 04:17:07]

lgtm with nits

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
File
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html
(right):

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:16:
testInput.value="";
nit: Needs spaces around '='

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:22:
shouldBe("style.getPropertyValue('background-color')", "'rgb(0, 0, 255)'");
nit: Use shouldBeEqualToString

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:25:
shouldBe("style.getPropertyValue('background-color')", "'rgb(255, 255, 255)'");
Ditto.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.cpp (right):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.cpp:129: value ?
m_fieldOwner->didFocusOnField():m_fieldOwner->didBlurFromField();
nit: Needs spaces around ':'.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.h (left):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.h:99: 
nit: Please do not remove the blank line.  We usually add one blank line between
functions and fields.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeNumericFieldElement.h (right):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeNumericFieldElement.h:83: virtual void
setFocus(bool) OVERRIDE FINAL;
The above comment says setFocus(bool) is a member function of
DateTimeFieldElement.  But it's not.  Please make a new section below "virtual
String value()" like:

    virtual String value() const OVERRIDE FINAL;

    // Node functions.
    virtual void setFocus(bool) OVERRIDE FINAL;

[Habib Virji, 2014-03-31 09:05:51]

Thanks, updated as per review comments.

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
File
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html
(right):

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:16:
testInput.value="";
On 2014/03/31 04:17:08, tkent wrote:
> nit: Needs spaces around '='

Done.

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:22:
shouldBe("style.getPropertyValue('background-color')", "'rgb(0, 0, 255)'");
On 2014/03/31 04:17:08, tkent wrote:
> nit: Use shouldBeEqualToString

Done.

https://codereview.chromium.org/191293011/diff/20001/LayoutTests/fast/forms/d...
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-onblur-setvalue-onfocusremoved.html:25:
shouldBe("style.getPropertyValue('background-color')", "'rgb(255, 255, 255)'");
On 2014/03/31 04:17:08, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.cpp (right):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.cpp:129: value ?
m_fieldOwner->didFocusOnField():m_fieldOwner->didBlurFromField();
On 2014/03/31 04:17:08, tkent wrote:
> nit: Needs spaces around ':'.

Done.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.h (left):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.h:99: 
On 2014/03/31 04:17:08, tkent wrote:
> nit: Please do not remove the blank line.  We usually add one blank line
between
> functions and fields.

Done.

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeNumericFieldElement.h (right):

https://codereview.chromium.org/191293011/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeNumericFieldElement.h:83: virtual void
setFocus(bool) OVERRIDE FINAL;
On 2014/03/31 04:17:08, tkent wrote:
> The above comment says setFocus(bool) is a member function of
> DateTimeFieldElement.  But it's not.  Please make a new section below "virtual
> String value()" like:
> 
>     virtual String value() const OVERRIDE FINAL;
> 
>     // Node functions.
>     virtual void setFocus(bool) OVERRIDE FINAL;
> 

Done.

[tkent, 2014-03-31 09:13:54]

lgtm.  Thank you!

[tkent, 2014-03-31 09:13:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-31 09:14:02]

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

[commit-bot: I haz the power, 2014-03-31 09:41:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 09:41:02]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[tkent, 2014-03-31 09:42:19]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-31 09:42:20]

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

[commit-bot: I haz the power, 2014-03-31 10:22:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 10:22:35]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-31 11:54:11]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-31 11:54:21]

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

[commit-bot: I haz the power, 2014-03-31 11:54:59]

Change committed as 170441