WebUI: Stop keypresses on the close icon from firing the Enter handler.

WebUI: Stop keypresses on the close icon from firing the Enter handler.

Previously, Enter keypresses on the close icon could trigger the
whole dialog's default Enter handler.

This CL prevents that.

BUG=719387
TEST=MANUAL
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2884113002
Cr-Commit-Position: refs/heads/master@{#472159}
Committed: https://chromium.googlesource.com/chromium/src/+/d760627f0528f7ac3969c4109ba33abb2265b5c3

[tommycli, 2017-05-15 23:11:03]

Description was changed from

==========
WebUI: Stop keypresses on the close icon from firing the Enter handler.

Previously, Enter keypresses on the close icon could trigger the
whole dialog's default Enter handler.

This CL prevents that.

BUG=719387
TEST=MANUAL
R=dpapad@chromium.org
==========

to

==========
WebUI: Stop keypresses on the close icon from firing the Enter handler.

Previously, Enter keypresses on the close icon could trigger the
whole dialog's default Enter handler.

This CL prevents that.

BUG=719387
TEST=MANUAL
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-05-15 23:12:07]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 23:13:03]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-15 23:20:56]

Thanks for the fix. Questions:

1) Do we need to merge this to M59?
2) Is there a way to test this logic somewhere at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/cr_elements/cr_di...

https://codereview.chromium.org/2884113002/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right):

https://codereview.chromium.org/2884113002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:149: stopPropagation_:
function(e) {
Nit: Since this handler is only attached to the 'x' close button, can we rename
this to onCloseKeypress_ to make it more explicit?

[tommycli, 2017-05-15 23:27:00]

Thanks!

1) Worth a merge if it's clean. If it's unclean, we should just leave 59 alone.

2) I added a test. Thanks!

https://codereview.chromium.org/2884113002/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right):

https://codereview.chromium.org/2884113002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:149: stopPropagation_:
function(e) {
On 2017/05/15 23:20:56, dpapad wrote:
> Nit: Since this handler is only attached to the 'x' close button, can we
rename
> this to onCloseKeypress_ to make it more explicit?

Done.

[tommycli, 2017-05-15 23:27:45]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 23:29:06]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-15 23:38:57]

LGTM with nits.

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:62: var closeButton =
document.body.querySelector('* /deep/ #close');
Can we use dialog.getCloseButton() instead? See
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog....

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:68:
MockInteractions.tap(actionButton, 'keypress', 13, undefined, 'Enter');
Is the dialog closed (because of the code added above), when this line executes?
I guess that does not create any issue for this test.

[tommycli, 2017-05-15 23:43:04]

good suggestions, thanks!

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:62: var closeButton =
document.body.querySelector('* /deep/ #close');
On 2017/05/15 23:38:56, dpapad wrote:
> Can we use dialog.getCloseButton() instead? See
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog....

Done.

https://codereview.chromium.org/2884113002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:68:
MockInteractions.tap(actionButton, 'keypress', 13, undefined, 'Enter');
On 2017/05/15 23:38:56, dpapad wrote:
> Is the dialog closed (because of the code added above), when this line
executes?
> I guess that does not create any issue for this test.

I reordered the tests just to be safe.

[tommycli, 2017-05-15 23:43:09]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-15 23:43:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2884113002/#ps40001
(title: "update tests")

[commit-bot: I haz the power, 2017-05-15 23:44:41]

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-16 01:58:51]

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

[commit-bot: I haz the power, 2017-05-16 01:58:52]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommycli, 2017-05-16 16:18:32]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-05-16 16:19:30]

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-16 18:07:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494951512062900,
"parent_rev": "f7efcd141c49fca16e706ade7172d16826ee7edc", "commit_rev":
"d760627f0528f7ac3969c4109ba33abb2265b5c3"}

[commit-bot: I haz the power, 2017-05-16 18:07:22]

Description was changed from

==========
WebUI: Stop keypresses on the close icon from firing the Enter handler.

Previously, Enter keypresses on the close icon could trigger the
whole dialog's default Enter handler.

This CL prevents that.

BUG=719387
TEST=MANUAL
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: Stop keypresses on the close icon from firing the Enter handler.

Previously, Enter keypresses on the close icon could trigger the
whole dialog's default Enter handler.

This CL prevents that.

BUG=719387
TEST=MANUAL
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-05-16 18:07:23]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d760627f0528f7ac3969c4109ba3...