MD Settings: Update some dialogs to accept Enter key.

MD Settings: Update some dialogs to accept Enter key.

Updates the following dialogs to accept the Enter key:
 1. Certificate decryption.
 2. Certificate encryption.
 3. Startup URL.
 4. Search Engine.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2653023003
Cr-Commit-Position: refs/heads/master@{#446402}
Committed: https://chromium.googlesource.com/chromium/src/+/2b25cef27d965ba56848727cf3097be410403cb6

[tommycli, 2017-01-24 22:15:26]

Description was changed from

==========
MD Settings: Update Search Engines Dialog to accept Enter key.

BUG=622816
R=dpapad@chromium.org
==========

to

==========
MD Settings: Update Search Engines Dialog to accept Enter key.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-01-24 22:15:36]

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

[commit-bot: I haz the power, 2017-01-24 22:16:18]

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

[tommycli, 2017-01-24 22:34:24]

Description was changed from

==========
MD Settings: Update Search Engines Dialog to accept Enter key.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Update some dialogs to accept Enter key.

Updates the following dialogs to accept the Enter key:
 1. Certificate decryption.
 2. Certificate encryption.
 3. Startup URL.
 4. Search Engine.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-01-24 22:34:43]

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

[commit-bot: I haz the power, 2017-01-24 22:35:39]

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

[tommycli, 2017-01-24 22:55:37]

dpapad: PTAL, thanks!

[dpapad, 2017-01-25 00:47:29]

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
(right):

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:18:
<iron-a11y-keys keys="enter" on-keys-pressed="onActionButtonTap_">
What is the benefit of using <iron-a11y-keys> instead of a simple native
keypress listener?

By reading the Docs the only benefit it seems to be cross-browser compatibility,
which is uninteresting to our Settings page.

"iron-a11y-keys provides a cross-browser interface for processing keyboard
commands."

[commit-bot: I haz the power, 2017-01-25 00:56:12]

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

[commit-bot: I haz the power, 2017-01-25 00:56:12]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-01-25 01:01:47]

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
(right):

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:18:
<iron-a11y-keys keys="enter" on-keys-pressed="onActionButtonTap_">
On 2017/01/25 00:47:29, dpapad wrote:
> What is the benefit of using <iron-a11y-keys> instead of a simple native
> keypress listener?
> 
> By reading the Docs the only benefit it seems to be cross-browser
compatibility,
> which is uninteresting to our Settings page.
> 
> "iron-a11y-keys provides a cross-browser interface for processing keyboard
> commands."

No very strong reason, just:

 1. Precedent... we use iron-a11y-keys in a number of other places in Settings
for key bindings.

 2. Somewhat more declarative... no need to add a (e.type == 'keypress' && e.key
!= 'Enter') check in the body of the method. Therefore we can often reuse the OK
tap handler.

[Dan Beam, 2017-01-25 01:04:52]

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
(right):

https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:18:
<iron-a11y-keys keys="enter" on-keys-pressed="onActionButtonTap_">
On 2017/01/25 01:01:47, tommycli wrote:
> On 2017/01/25 00:47:29, dpapad wrote:
> > What is the benefit of using <iron-a11y-keys> instead of a simple native
> > keypress listener?
> > 
> > By reading the Docs the only benefit it seems to be cross-browser
> compatibility,
> > which is uninteresting to our Settings page.
> > 
> > "iron-a11y-keys provides a cross-browser interface for processing keyboard
> > commands."
> 
> No very strong reason, just:
> 
>  1. Precedent... we use iron-a11y-keys in a number of other places in Settings
> for key bindings.
> 
>  2. Somewhat more declarative... no need to add a (e.type == 'keypress' &&
e.key
> != 'Enter') check in the body of the method. Therefore we can often reuse the
OK
> tap handler.

fwiw: i had this same question (why do you need an element and shadow DOM and
...?)

[tommycli, 2017-01-25 01:09:16]

On 2017/01/25 01:04:52, Dan Beam wrote:
>
https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html
> (right):
> 
>
https://codereview.chromium.org/2653023003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/on_startup_page/startup_url_dialog.html:18:
> <iron-a11y-keys keys="enter" on-keys-pressed="onActionButtonTap_">
> On 2017/01/25 01:01:47, tommycli wrote:
> > On 2017/01/25 00:47:29, dpapad wrote:
> > > What is the benefit of using <iron-a11y-keys> instead of a simple native
> > > keypress listener?
> > > 
> > > By reading the Docs the only benefit it seems to be cross-browser
> > compatibility,
> > > which is uninteresting to our Settings page.
> > > 
> > > "iron-a11y-keys provides a cross-browser interface for processing keyboard
> > > commands."
> > 
> > No very strong reason, just:
> > 
> >  1. Precedent... we use iron-a11y-keys in a number of other places in
Settings
> > for key bindings.
> > 
> >  2. Somewhat more declarative... no need to add a (e.type == 'keypress' &&
> e.key
> > != 'Enter') check in the body of the method. Therefore we can often reuse
the
> OK
> > tap handler.
> 
> fwiw: i had this same question (why do you need an element and shadow DOM and
> ...?)

Bah... I see your guys' point.

I will convert this (and the other places) to use the native on-keypress
handlers. Thanks!

Tommy

[tommycli, 2017-01-25 01:21:09]

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

[commit-bot: I haz the power, 2017-01-25 01:21:57]

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

[tommycli, 2017-01-25 01:22:08]

dpapad: PTAL again, this version uses the native on-keypress event

[dpapad, 2017-01-25 01:46:13]

https://codereview.chromium.org/2653023003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_decryption_dialog.js
(right):

https://codereview.chromium.org/2653023003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_decryption_dialog.js:39:
onOkTap_: function(e) {
Normally you would need to add an @param here, and things would get complicated,
since this can be invoked both with a mouse and a keyboard.


This might be tedious, but, can we split the button handling to a separate
method?

value="{{password_}}" on-keypress="onKeyPress_">

/** @param {!KeyboardEvent} e */
onKeyPress_: function(e) {
  // No need to check for the |e.type| anymore.
  if (e.key == 'Enter') {
    // No need to check for ok.disabled.
    // I believe tap() does nothing if the button is disabled but need to
verify.
    this.$.ok.tap();                 
  }    
}

[commit-bot: I haz the power, 2017-01-25 05:24:05]

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

[commit-bot: I haz the power, 2017-01-25 05:24:06]

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

[tommycli, 2017-01-25 19:15:53]

dpapad: thanks!

https://codereview.chromium.org/2653023003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_decryption_dialog.js
(right):

https://codereview.chromium.org/2653023003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_decryption_dialog.js:39:
onOkTap_: function(e) {
On 2017/01/25 01:46:13, dpapad wrote:
> Normally you would need to add an @param here, and things would get
complicated,
> since this can be invoked both with a mouse and a keyboard.
> 
> 
> This might be tedious, but, can we split the button handling to a separate
> method?
> 
> value="{{password_}}" on-keypress="onKeyPress_">
> 
> /** @param {!KeyboardEvent} e */
> onKeyPress_: function(e) {
>   // No need to check for the |e.type| anymore.
>   if (e.key == 'Enter') {
>     // No need to check for ok.disabled.
>     // I believe tap() does nothing if the button is disabled but need to
> verify.
>     this.$.ok.tap();                 
>   }    
> }

Done.

tap() was not a method. I saw click(), but it isn't used anywhere else in the
code, and also works even when disabled, so I just called the handler directly.
Thanks!

[dpapad, 2017-01-25 19:32:00]

LGTM, but should we update some tests too, simulating the "Enter" button?

[tommycli, 2017-01-25 22:30:52]

On 2017/01/25 19:32:00, dpapad wrote:
> LGTM, but should we update some tests too, simulating the "Enter" button?

I added a test in PS5 for one of the dialogs, but in general, I think writing 16
lines to test 10 lines of non-critical logic is kind of a losing proposition.

What do you think?

[tommycli, 2017-01-25 22:32:10]

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

[commit-bot: I haz the power, 2017-01-25 22:33:31]

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-01-25 22:41:19]

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

[commit-bot: I haz the power, 2017-01-25 22:41:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2017-01-25 22:43:23]

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

[commit-bot: I haz the power, 2017-01-25 22:44:13]

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

[dpapad, 2017-01-25 23:16:09]

On 2017/01/25 at 22:30:52, tommycli wrote:
> On 2017/01/25 19:32:00, dpapad wrote:
> > LGTM, but should we update some tests too, simulating the "Enter" button?
> 
> I added a test in PS5 for one of the dialogs, but in general, I think writing
16 lines to test 10 lines of non-critical logic is kind of a losing proposition.
> 
> What do you think?

Thanks for adding the test. I would argue that there is no such thing as
non-critical logic. Or in reverse, if logic is non critical, should not be added
in the first place. But I do see your point about repeating the same test
multiple times being a losing proposition. Here is a thought (for a potential
future CL):

Perhaps we should add that logic in the parent cr-dialog class, and test it
there. It could be implemented as follows:

1. Add a new 'submit-on-enter' attribute on cr-dialog.

Example usage
<dialog is="cr-dialog" submit-on-enter>
  ...
</dialog>

2. Force every cr-dialog subclass to implement two methods.
   - canSubmit()
   - submit()

3. Inside cr-dialog, we would listen for the Enter keypress and we would do
   onKeyPress_: function(e) {
     if (e.key != 'Enter' || !this.canSubmit())
       return;

     this.submit();
   },

We would test that functionality in cr-dialog unit tests only.

Still LGTM.

[tommycli, 2017-01-25 23:19:16]

On 2017/01/25 23:16:09, dpapad wrote:
> On 2017/01/25 at 22:30:52, tommycli wrote:
> > On 2017/01/25 19:32:00, dpapad wrote:
> > > LGTM, but should we update some tests too, simulating the "Enter" button?
> > 
> > I added a test in PS5 for one of the dialogs, but in general, I think
writing
> 16 lines to test 10 lines of non-critical logic is kind of a losing
proposition.
> > 
> > What do you think?
> 
> Thanks for adding the test. I would argue that there is no such thing as
> non-critical logic. Or in reverse, if logic is non critical, should not be
added
> in the first place. But I do see your point about repeating the same test
> multiple times being a losing proposition. Here is a thought (for a potential
> future CL):
> 
> Perhaps we should add that logic in the parent cr-dialog class, and test it
> there. It could be implemented as follows:
> 
> 1. Add a new 'submit-on-enter' attribute on cr-dialog.
> 
> Example usage
> <dialog is="cr-dialog" submit-on-enter>
>   ...
> </dialog>
> 
> 2. Force every cr-dialog subclass to implement two methods.
>    - canSubmit()
>    - submit()
> 
> 3. Inside cr-dialog, we would listen for the Enter keypress and we would do
>    onKeyPress_: function(e) {
>      if (e.key != 'Enter' || !this.canSubmit())
>        return;
> 
>      this.submit();
>    },
> 
> We would test that functionality in cr-dialog unit tests only.
> 
> Still LGTM.

dpapad: I have also heard this theory (about the need for complete coverage).
Could be valid...

I have also thought about a generic / abstract key handler.

Unfortunately, the key handling is pretty dialog specific. To give some
examples: If the focus is in a TEXTAREA in the Address Dialog or on the cancel
button, we definitely would not want Enter to call submit(). We can chat more
offline, I'm going to send in this particular CL. I'd be interested if we could
come up with a way to handle this generically.

[tommycli, 2017-01-25 23:19:22]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-01-25 23:19:23]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-01-25 23:19:45]

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-01-26 01:52:13]

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

[commit-bot: I haz the power, 2017-01-26 01:52:14]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommycli, 2017-01-26 17:53:47]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-01-26 17:54:23]

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-01-26 19:45:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485453227075500,
"parent_rev": "4bc67b96bd6c94833b002162e0bab92ff17a9752", "commit_rev":
"2b25cef27d965ba56848727cf3097be410403cb6"}

[commit-bot: I haz the power, 2017-01-26 19:45:58]

Description was changed from

==========
MD Settings: Update some dialogs to accept Enter key.

Updates the following dialogs to accept the Enter key:
 1. Certificate decryption.
 2. Certificate encryption.
 3. Startup URL.
 4. Search Engine.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Update some dialogs to accept Enter key.

Updates the following dialogs to accept the Enter key:
 1. Certificate decryption.
 2. Certificate encryption.
 3. Startup URL.
 4. Search Engine.

BUG=622816
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2653023003
Cr-Commit-Position: refs/heads/master@{#446402}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b25cef27d965ba56848727cf309...
==========

[commit-bot: I haz the power, 2017-01-26 19:46:00]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2b25cef27d965ba56848727cf309...