DevTools: pressing enter in bound input fields should not fire change handler

DevTools: pressing enter in bound input fields should not fire change handler

When a user modifies a bound input's text and presses Enter, the bound callback
would fire once on keydown and once on the following 'change' event.  This CL
prevents the default 'change' event from following up to avoid two calls.

BUG=621922
Review-Url: https://codereview.chromium.org/2843203003
Cr-Commit-Position: refs/heads/master@{#468433}
Committed: https://chromium.googlesource.com/chromium/src/+/b0fb333255e386d83f6d1b69d7239011e780ae4b

[luoe, 2017-04-27 20:15:17]

Description was changed from

==========
DevTools: bound input fields should not fire handler on enter

BUG=621922
==========

to

==========
DevTools: bound input fields should not fire handler on enter

A few places use UIUtils.bindInput(), which registers listeners for both
'change' and 'keydown' events. When a user modifies a field's text and presses
'Enter', this triggered two calls to the bound handler.

This CL modifies the behavior so that pressing 'Enter' repeatedly will no longer
trigger multiple handler invocations. Instead, it will only be invoked upon
actual changes.

BUG=621922
==========

[luoe, 2017-04-27 20:19:01]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org

[luoe, 2017-04-27 20:19:02]

This CL updates behavior of bindInput first introduced here:
https://codereview.chromium.org/1642233002

While the bug only refers to the Geolocation input fields, the 2 invocations
behavior is actually seen in the other bindInput cases too, (orientation inputs,
device mode, workspace folder exclude pattern in settings).

Please take a look

[dgozman, 2017-04-27 22:58:55]

https://codereview.chromium.org/2843203003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (left):

https://codereview.chromium.org/2843203003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1561: if
(isEnterKey(event)) {
Why was this introduced in first place? Was it there for old non-instant mode?
Could you find a patch which did it?

Let's double check all the clients to see whether they continue to work with
enter key.

[luoe, 2017-04-29 03:29:49]

Description was changed from

==========
DevTools: bound input fields should not fire handler on enter

A few places use UIUtils.bindInput(), which registers listeners for both
'change' and 'keydown' events. When a user modifies a field's text and presses
'Enter', this triggered two calls to the bound handler.

This CL modifies the behavior so that pressing 'Enter' repeatedly will no longer
trigger multiple handler invocations. Instead, it will only be invoked upon
actual changes.

BUG=621922
==========

to

==========
DevTools: pressing enter in bound input fields should not fire change handler

When a user modifies a bound input's text and presses Enter, the bound callback
would fire once on keydown and once on the following 'change' event.  This CL
prevents the default 'change' event from following up to avoid two calls.

BUG=621922
==========

[luoe, 2017-04-29 03:29:59]

https://codereview.chromium.org/2843203003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (left):

https://codereview.chromium.org/2843203003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1561: if
(isEnterKey(event)) {
On 2017/04/27 22:58:55, dgozman wrote:
> Why was this introduced in first place? Was it there for old non-instant mode?
> Could you find a patch which did it?
> 
> Let's double check all the clients to see whether they continue to work with
> enter key.

As far as I can tell, the history looks like this:

https://codereview.chromium.org/45363004
The first instance of listening for Enter on keydown is introduced in
OverridesView.js for 'createInput()'.  No 'change' listeners or validators, here
we only applied the value on Enter and blur.

https://codereview.chromium.org/304693002
Instant flag introduced on createSettingInputField, we only apply values if the
input is 'instant' or we 'blur'.  We start listening for 'change' events, but
only do validation and not apply on changes.

https://codereview.chromium.org/315003008
createSettingInputField now gets 'keydown' for applying on Enter.  All clients
of createSettingInputField provided either a validator or instant == true, so
they all should have listened to 'change' events.

https://codereview.chromium.org/1642233002
Finally, createSettingInputField was removed during a large cleanup, and we
ended up with today's bindInput as its replacement.

Commentary:
Today, both 'change' events and pressing Enter can apply the new value.  For
DeviceModeToolbar's width/height input and Workspace's exclude file pattern, it
doesn't really make sense to apply on Enter if the 'change' listener is already
at work.  After playing around with the geolocation/orientation inputs, I
realize why this doesn't always apply.  If we're doing
geolocation.watchPosition(), I can imagine a workflow where pressing Enter is
useful to forcefully trigger your callback to run.

As such, I've instead kept the enter logic and just added a preventDefault so
that the change handler doesn't fire after it.

[dgozman, 2017-05-01 18:37:57]

Thank you for digging the information, that is very helpful! lgtm

[luoe, 2017-05-01 18:39:56]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2017-05-01 18:40:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 21:50:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493663996381800,
"parent_rev": "a9cfcb53166c1ad54a7c8d07e205b13b970b2b93", "commit_rev":
"b0fb333255e386d83f6d1b69d7239011e780ae4b"}

[commit-bot: I haz the power, 2017-05-01 21:50:28]

Description was changed from

==========
DevTools: pressing enter in bound input fields should not fire change handler

When a user modifies a bound input's text and presses Enter, the bound callback
would fire once on keydown and once on the following 'change' event.  This CL
prevents the default 'change' event from following up to avoid two calls.

BUG=621922
==========

to

==========
DevTools: pressing enter in bound input fields should not fire change handler

When a user modifies a bound input's text and presses Enter, the bound callback
would fire once on keydown and once on the following 'change' event.  This CL
prevents the default 'change' event from following up to avoid two calls.

BUG=621922

Review-Url: https://codereview.chromium.org/2843203003
Cr-Commit-Position: refs/heads/master@{#468433}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0fb333255e386d83f6d1b69d723...
==========

[commit-bot: I haz the power, 2017-05-01 21:50:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b0fb333255e386d83f6d1b69d723...