Dragging text from 'chrome://settings/searchEngines' leaves input empty.

Dragging text from 'chrome://settings/searchEngines' leaves input empty.

Delay 'focus' event handler to ensure that 'input' event
is processed first.

BUG=263743
R=arv@chromium.org, tkent@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233268

[tkent, 2013-10-24 21:42:14]

> Replace synchronous 'focus' with queued 'focusin' to make events appear 
> in expected order. 

Would you explain more please?  What was the problem, and why focusin fixes the
problem?
Note that focusin is also a synchronous event.  It's dispatched just after
dispatching focus event.

dbeam, do we have a reasonable way to test mouse interaction of WebUI?

[Dan Beam, 2013-10-24 23:49:43]

On 2013/10/24 21:42:14, tkent wrote:
> > Replace synchronous 'focus' with queued 'focusin' to make events appear 
> > in expected order. 
> 
> Would you explain more please?  What was the problem, and why focusin fixes
the
> problem?
> Note that focusin is also a synchronous event.  It's dispatched just after
> dispatching focus event.
> 
> dbeam, do we have a reasonable way to test mouse interaction of WebUI?

yes, we have a browser test framework. here's some example tests:

$ find chrome/ -type f -iname '*browsertest.js'
chrome/browser/ui/webui/options/settings_format_browsertest.js
chrome/browser/ui/webui/options/options_browsertest.js
chrome/browser/ui/webui/options/settings_app_browsertest.js
chrome/browser/ui/webui/options/browser_options_browsertest.js
chrome/browser/ui/webui/options/content_settings_exception_area_browsertest.js
chrome/browser/ui/webui/options/cookies_view_browsertest.js
chrome/browser/ui/webui/options/password_manager_browsertest.js
chrome/browser/ui/webui/options/font_settings_browsertest.js
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.js
chrome/browser/ui/webui/options/chromeos/bluetooth_options_browsertest.js
chrome/browser/ui/webui/options/content_options_browsertest.js
chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js
chrome/browser/ui/webui/options/language_options_browsertest.js
chrome/browser/ui/webui/options/search_engine_manager_browsertest.js
chrome/browser/ui/webui/options/certificate_manager_browsertest.js
chrome/browser/ui/webui/options/manage_profile_browsertest.js
chrome/browser/ui/webui/options/edit_dictionary_browsertest.js
chrome/browser/ui/webui/options/profile_settings_reset_browsertest.js
chrome/browser/ui/webui/options/autofill_options_browsertest.js
chrome/browser/ui/webui/downloads_ui_browsertest.js
chrome/browser/ui/webui/help/help_browsertest.js
chrome/browser/ui/webui/sync_setup_browsertest.js
chrome/browser/ui/webui/identity_internals_ui_browsertest.js
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_browsertest.js
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
chrome/browser/ui/webui/app_list/start_page_browsertest.js
chrome/test/data/chromeos/oobe_webui_browsertest.js
chrome/test/data/webui/chrome_send_browsertest.js
chrome/test/data/webui/history_browsertest.js
chrome/test/data/webui/accessibility_audit_browsertest.js
chrome/test/data/webui/mock4js_browsertest.js
chrome/test/data/webui/sandboxstatus_browsertest.js
chrome/test/data/local_ntp_browsertest.js

[eustas, 2013-10-25 05:37:02]

On 2013/10/24 21:42:14, tkent wrote:
> > Replace synchronous 'focus' with queued 'focusin' to make events appear 
> > in expected order. 
> 
> Would you explain more please?  What was the problem, and why focusin fixes
the
> problem?
> Note that focusin is also a synchronous event.  It's dispatched just after
> dispatching focus event.
> 
> dbeam, do we have a reasonable way to test mouse interaction of WebUI?

There are actually 2 subclasses of synchronous events:
 - events that are dispatched as soon as fired
 - events whose dispatching is delayed until browser has finished some "atomic"
work.

'focus' and 'blur' are of 1-st kind.
'focusin', 'focusout', 'input', 'change' are of 2-nd kind.

When edit command is applied many events are fired.
Specifically - 'selection' and 'input'.

To make DOM state consistent from the point of view of event handlers those
events are
taken in the event scope - so they will be dispatched when all work is done.

The subtle thing is that selection causes change of focus.
Change of focus is represented by several events.
'focus' will be dispatched before scoped 'input'.

As a result, when 'focus' is dispatched, list item we've dragged from is not
sent to revalidation,
so we do 'commitedit' with wrong data.

If we use 'focusin' instead of 'focus' all events come in order. So, 'input'
arrives first and marks
list item we dragged from as not-validated. Then 'focusin' comes and do
'canceledit' that restores
field values.

[tkent, 2013-10-25 05:54:02]

On 2013/10/25 05:37:02, eustas.ru wrote:
> On 2013/10/24 21:42:14, tkent wrote:
> > > Replace synchronous 'focus' with queued 'focusin' to make events appear 
> > > in expected order. 
> > 
> > Would you explain more please?  What was the problem, and why focusin fixes
> the
> > problem?
> > Note that focusin is also a synchronous event.  It's dispatched just after
> > dispatching focus event.
> > 
> > dbeam, do we have a reasonable way to test mouse interaction of WebUI?
> 
> There are actually 2 subclasses of synchronous events:
>  - events that are dispatched as soon as fired
>  - events whose dispatching is delayed until browser has finished some
"atomic"
> work.
> 
> 'focus' and 'blur' are of 1-st kind.
> 'focusin', 'focusout', 'input', 'change' are of 2-nd kind.
> 
> When edit command is applied many events are fired.
> Specifically - 'selection' and 'input'.
> 
> To make DOM state consistent from the point of view of event handlers those
> events are
> taken in the event scope - so they will be dispatched when all work is done.
> 
> The subtle thing is that selection causes change of focus.
> Change of focus is represented by several events.
> 'focus' will be dispatched before scoped 'input'.
> 
> As a result, when 'focus' is dispatched, list item we've dragged from is not
> sent to revalidation,
> so we do 'commitedit' with wrong data.
> 
> If we use 'focusin' instead of 'focus' all events come in order. So, 'input'
> arrives first and marks
> list item we dragged from as not-validated. Then 'focusin' comes and do
> 'canceledit' that restores
> field values.

Unfortunately, Patch Set 1 strongly depends on Blink implementation and Blink
bugs.
Both of 'focus' and 'focusin' should be sync events. Using dispatchScopedEvent
for 'focusin' in Blink looks a bug.
Also, the standard says 'focusin' should be dispatched before 'focus'. Blink
doesn't follow it.

http://www.w3.org/TR/DOM-Level-3-Events/#event-type-focus

[eustas, 2013-10-25 06:17:56]

> Unfortunately, Patch Set 1 strongly depends on Blink implementation and Blink
> bugs.
> Both of 'focus' and 'focusin' should be sync events. Using dispatchScopedEvent
> for 'focusin' in Blink looks a bug.
> Also, the standard says 'focusin' should be dispatched before 'focus'. Blink
> doesn't follow it.
> 
> http://www.w3.org/TR/DOM-Level-3-Events/#event-type-focus

Agree.

So, we are to add a hook that checks if data is still valid?
(Do not depend on 'input' event).

[tkent, 2013-10-25 06:31:47]

var handler = this.handleFocus_.bind(this);
inputEl.addEventListener('focus', function(event) { setTimeout(handler, 0); });

Does this work?

[eustas, 2013-10-25 07:21:00]

On 2013/10/25 06:31:47, tkent wrote:
> var handler = this.handleFocus_.bind(this);
> inputEl.addEventListener('focus', function(event) { setTimeout(handler, 0);
});
> 
> Does this work?

Of course.
But I'm not sure if it is a good solution.

[tkent, 2013-10-28 03:21:54]

lgtm  It's better than focusin.
- We should have a code comment to explain the delay.
- The CL description should be updated.

[eustas, 2013-10-28 05:32:36]

On 2013/10/28 03:21:54, tkent wrote:
> lgtm  It's better than focusin.
> - We should have a code comment to explain the delay.
> - The CL description should be updated.

Done.

I've noticed that subclasses do not use event parameter in 'handleFocus_'.
However some of them declare it.

To make things clear we should either remove that parameter or propagate it.

WDYT?

[commit-bot: I haz the power, 2013-10-29 06:09:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eustas@chromium.org/39843002/150001

[commit-bot: I haz the power, 2013-10-29 06:43:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[eustas, 2013-11-05 13:39:02]

On 2013/10/29 06:43:44, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Ping to owners.

[arv (Not doing code reviews), 2013-11-05 14:36:08]

LGTM

[commit-bot: I haz the power, 2013-11-06 05:36:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eustas@chromium.org/39843002/150001

[commit-bot: I haz the power, 2013-11-06 09:17:11]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[eustas, 2013-11-06 14:10:29]

Committed patchset #3 manually as r233268 (presubmit successful).