WebUI: For cr-dialog, make Enter click the action-button, if it exists.

WebUI: For cr-dialog, make Enter click the action-button, if it exists.

Within cr-dialogs, this makes Enter click the action-button, if it
exists. It conservatively rejects any Enter from a target other than
the dialog.

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

Review-Url: https://codereview.chromium.org/2788673003
Cr-Commit-Position: refs/heads/master@{#461585}
Committed: https://chromium.googlesource.com/chromium/src/+/0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b

[tommycli, 2017-03-31 00:13:39]

Description was changed from

==========
WebUI: For cr-dialog, make Enter click the action-button, if it exists.

Within cr-dialogs, this makes Enter click the action-button, if it
exists. It conservatively rejects any Enter from a target other than
the dialog.

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

to

==========
WebUI: For cr-dialog, make Enter click the action-button, if it exists.

Within cr-dialogs, this makes Enter click the action-button, if it
exists. It conservatively rejects any Enter from a target other than
the dialog.

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

[tommycli, 2017-03-31 00:16:54]

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

[tommycli, 2017-03-31 00:16:55]

dpapad: PTAL, thanks!
dbeam: CCing you since this is core WebUI stuff, thanks!

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

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:78: if (e.target != this)
This is more conservative than the Options equivalent code [1], but I suggest we
roll with this for now, and amp-up the aggression later.

We will probably have to amp up the aggression to eliminate all these redundant
handlers [2] already within Settings, but I think we should save that for a
followup CL if you like this one.

[1] 
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/overlay.js?t...
[2]
https://cs.chromium.org/search/?q=onKeypress+file:%5Esrc/chrome/browser/resou...

[dpapad, 2017-03-31 02:08:29]

Can we add a test for this functionality in cr_dialog_tests.js?

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

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:83: var actionButton =
this.querySelector('.action-button');
Should we add an assertion() for this, instead of silently doing nothing? Do we
have dialogs that don't have an action button, in Settings or some other WebUI
page?

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:84: if (actionButton &&
!actionButton.disabled) {
Do we need to check for |disabled| at all? I believe that click() is a no-op if
the element is disabled.

It would be nice if we can keep the dialog oblivious to the state of the action
button. At least until (if ever) we decide to address the multiple redundant
handlers you linked above.

[tommycli, 2017-04-03 16:53:34]

dpapad: thanks!

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

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:83: var actionButton =
this.querySelector('.action-button');
On 2017/03/31 02:08:29, dpapad wrote:
> Should we add an assertion() for this, instead of silently doing nothing? Do
we
> have dialogs that don't have an action button, in Settings or some other WebUI
> page?

Hey demetrios, here's an example where there's no action-button:

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/blueto...

In any case, I don't think we should assert, since it's not in the contract for
a cr-dialog that it -must- have a button with the action-button class.

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:84: if (actionButton &&
!actionButton.disabled) {
On 2017/03/31 02:08:29, dpapad wrote:
> Do we need to check for |disabled| at all? I believe that click() is a no-op
if
> the element is disabled.
> 
> It would be nice if we can keep the dialog oblivious to the state of the
action
> button. At least until (if ever) we decide to address the multiple redundant
> handlers you linked above.

Hi, disabled buttons can still be 'click()' ed. See example:
https://jsfiddle.net/4rdzc3tf/1/

[dpapad, 2017-04-03 18:20:07]

Thanks for the explanations. One comment left unaddressed from previous batch of
comments,
"Can we add a test for this functionality in cr_dialog_tests.js?"

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

https://codereview.chromium.org/2788673003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:84: if (actionButton &&
!actionButton.disabled) {
On 2017/04/03 at 16:53:34, tommycli wrote:
> On 2017/03/31 02:08:29, dpapad wrote:
> > Do we need to check for |disabled| at all? I believe that click() is a no-op
if
> > the element is disabled.
> > 
> > It would be nice if we can keep the dialog oblivious to the state of the
action
> > button. At least until (if ever) we decide to address the multiple redundant
> > handlers you linked above.
> 
> Hi, disabled buttons can still be 'click()' ed. See example:
> https://jsfiddle.net/4rdzc3tf/1/

Ok. I recall having problems "clicking" disabled buttons in the past. Maybe I am
thinking of MockInteractions.tap(), not sure.

[tommycli, 2017-04-03 19:21:47]

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

[tommycli, 2017-04-03 19:22:03]

dpapad: Thanks! I added a test. The inter-patch changes in cr_dialog.js are due
to a merge (not my changes).

[commit-bot: I haz the power, 2017-04-03 19:22:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-04-03 19:30:24]

Thanks for adding tests. LGTM with optional suggestion.

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

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:52: dialog, 'keypress', 13,
undefined, 'Enter');
Nit: Indent off by 2

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:58: dialog, 'keypress', 13,
undefined, 'Enter');
Nit: Indent off by 2

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:66: otherButton,
'keypress', 13, undefined, 'Enter');
Nit: Indent off by 2

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:68: });
Optional suggestion for one more test case.

 - Change |clicked| from a boolean to a counter.
 - Set it to 0
 - Trigger "enter" directly on actionButton.
 - Assert that clicked counter is 1 (ensuring that dialog's handler is not
accidentally handling bubbling event from the action button.

[commit-bot: I haz the power, 2017-04-03 19:43:42]

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

[commit-bot: I haz the power, 2017-04-03 19:43:43]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[tommycli, 2017-04-03 22:57:55]

Thanks!!

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

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:52: dialog, 'keypress', 13,
undefined, 'Enter');
On 2017/04/03 19:30:24, dpapad wrote:
> Nit: Indent off by 2

Done. I ran git cl format --js.

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:58: dialog, 'keypress', 13,
undefined, 'Enter');
On 2017/04/03 19:30:24, dpapad wrote:
> Nit: Indent off by 2

Done. Ran git cl format --js.

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:66: otherButton,
'keypress', 13, undefined, 'Enter');
On 2017/04/03 19:30:24, dpapad wrote:
> Nit: Indent off by 2

Done.

https://codereview.chromium.org/2788673003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:68: });
On 2017/04/03 19:30:24, dpapad wrote:
> Optional suggestion for one more test case.
> 
>  - Change |clicked| from a boolean to a counter.
>  - Set it to 0
>  - Trigger "enter" directly on actionButton.
>  - Assert that clicked counter is 1 (ensuring that dialog's handler is not
> accidentally handling bubbling event from the action button.

Done.

[tommycli, 2017-04-03 22:57:58]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-04-03 22:57:58]

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/2788673003/#ps40001
(title: "change closure annotations")

[commit-bot: I haz the power, 2017-04-03 22:58: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-04-04 00:10:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491260278576900,
"parent_rev": "20e2934528d1d7332529212f6d7295ac87198107", "commit_rev":
"0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b"}

[commit-bot: I haz the power, 2017-04-04 00:12:07]

Description was changed from

==========
WebUI: For cr-dialog, make Enter click the action-button, if it exists.

Within cr-dialogs, this makes Enter click the action-button, if it
exists. It conservatively rejects any Enter from a target other than
the dialog.

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

to

==========
WebUI: For cr-dialog, make Enter click the action-button, if it exists.

Within cr-dialogs, this makes Enter click the action-button, if it
exists. It conservatively rejects any Enter from a target other than
the dialog.

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

Review-Url: https://codereview.chromium.org/2788673003
Cr-Commit-Position: refs/heads/master@{#461585}
Committed:
https://chromium.googlesource.com/chromium/src/+/0be2cd95f834e9ee7c46bcc7cf40...
==========

[commit-bot: I haz the power, 2017-04-04 00:12:08]

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