WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

Followup to https://codereview.chromium.org/2788673003/ that enhances
the Enter key handler to ignore disabled or hidden buttons.

BUG=626420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[tommycli, 2017-04-04 00:12:34]

Description was changed from

==========
WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

Followup to https://codereview.chromium.org/2788673003/ that enhances
the Enter key handler to ignore disabled or hidden buttons.

BUG=626420
==========

to

==========
WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

Followup to https://codereview.chromium.org/2788673003/ that enhances
the Enter key handler to ignore disabled or hidden buttons.

BUG=626420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-04-04 00:12:58]

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

[tommycli, 2017-04-04 00:13:13]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org

[tommycli, 2017-04-04 00:13:13]

dpapad: PTAL, thanks!

[commit-bot: I haz the power, 2017-04-04 00:13:33]

Dry run: 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-04-04 02:41:39]

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

[commit-bot: I haz the power, 2017-04-04 02:41:40]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-04-04 17:10:18]

LGTM, with nits.

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:50: // Enter key should
ignore disabled buttons.
Are lines 50-57 now redundant, meaning that the new test already covers those
cases? If so, perhaps consider removing from here.

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:91: throw new Error('Hidden
button received a click.');
Nit: Can we use assertNotReached() instead? We actually have two such functions
and (unfortunately) whichever gets defined last overwrites the previous
definitions (see links below).

https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?q&l=940
https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=55

[tommycli, 2017-04-04 17:44:11]

dpapad: thanks!

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:50: // Enter key should
ignore disabled buttons.
On 2017/04/04 17:10:17, dpapad wrote:
> Are lines 50-57 now redundant, meaning that the new test already covers those
> cases? If so, perhaps consider removing from here.

Done.

https://codereview.chromium.org/2792853003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:91: throw new Error('Hidden
button received a click.');
On 2017/04/04 17:10:17, dpapad wrote:
> Nit: Can we use assertNotReached() instead? We actually have two such
functions
> and (unfortunately) whichever gets defined last overwrites the previous
> definitions (see links below).
> 
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?q&l=940
> https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=55

Done. And I can confirm this fails the test if I remove the :not([hidden])
selector qualifier.

[tommycli, 2017-04-04 17:44:14]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-04-04 17:44:14]

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/2792853003/#ps20001
(title: "update test descriptions")

[commit-bot: I haz the power, 2017-04-04 17:44:43]

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-04-04 20:21:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491327854020830,
"parent_rev": "936a64459466aea94d5782255bc0570d9d764dae", "commit_rev":
"f55816a64a901b81f70c128361fc5df16ceb40f6"}

[commit-bot: I haz the power, 2017-04-04 20:22:42]

Description was changed from

==========
WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

Followup to https://codereview.chromium.org/2788673003/ that enhances
the Enter key handler to ignore disabled or hidden buttons.

BUG=626420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: For cr-dialog, make Enter key ignore disabled or hidden buttons

Followup to https://codereview.chromium.org/2788673003/ that enhances
the Enter key handler to ignore disabled or hidden buttons.

BUG=626420
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-04-04 20:22:45]

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