MD Settings: fix incorrect behavior with Enter on bluetooth item's icon-button.

MD Settings: fix incorrect behavior with Enter on bluetooth item's icon-button.

Pressing enter key on the icon-button is expected to open the action-menu, but instead it incorrectly focuses the row-item.

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

Review-Url: https://codereview.chromium.org/2945433002
Cr-Commit-Position: refs/heads/master@{#486641}
Committed: https://chromium.googlesource.com/chromium/src/+/4c0b234413f027df2d8a0b1f638bdf5d14ee7a2b

[scottchen, 2017-06-15 19:36:47]

Description was changed from

==========
MD Settings: fix bluetooth item button bugging out with Enter key.

BUG=None
==========

to

==========
MD Settings: fix bluetooth item button bugging out with Enter key.

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

[scottchen, 2017-06-15 19:37:09]

scottchen@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dschuyler, 2017-06-20 18:46:51]

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:40:
},
When we have a chance, I'd like to chat about whether this should be made into a
behavior (stevenjb@'s suggestion), or if it's really a one-off. And if it's a
one-off, I'd like to explore (or push for) adjusting the UI here so that it
doesn't need special handling.

[scottchen, 2017-06-20 19:17:20]

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:40:
},
On 2017/06/20 18:46:51, dschuyler wrote:
> When we have a chance, I'd like to chat about whether this should be made into
a
> behavior (stevenjb@'s suggestion), or if it's really a one-off. And if it's a
> one-off, I'd like to explore (or push for) adjusting the UI here so that it
> doesn't need special handling.

I think we might've talked about this before, but basically I had to do this in
FocusRowBehavior to avoid iron-list's Enter-key listener causing problems, but
FocusRowBehavior can't be applied here, because the behavior assumes the row
itself should not focusable (which is not the case here).

So possible solutions:
1) tweak focus-row-behavior to optionally allow the row itself to be focused
(maybe based on a flag?) and behave accordingly, then apply the behavior here.
2) make the UI such that "row itself is focusable & actionable, but also has
focusable & actionable controls as children"  is not a thing.

Also as a side note, I personally feel like network/bluetooth's row behavior is
weird, because pressing the Enter key on a focused row does completely different
things (i.e. connects if not already connected, or goes to a subpage if it is
connected).

[scottchen, 2017-07-08 00:07:00]

scottchen@chromium.org changed reviewers:
+ stevenjb@chromium.org

[scottchen, 2017-07-08 00:07:00]

Just wanted to ping this CL to see if further discussion is to be had.

[stevenjb, 2017-07-10 18:27:58]

On 2017/07/08 00:07:00, scottchen wrote:
> Just wanted to ping this CL to see if further discussion is to be had.

Is there an issue for this? What specifically is the problematic behavior here
(i.e. please define 'bugging out'), and does the problem exist for any of the
network lists?

[scottchen, 2017-07-10 22:34:27]

Description was changed from

==========
MD Settings: fix bluetooth item button bugging out with Enter key.

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

to

==========
MD Settings: fix incorrect behavior with Enter on bluetooth item's icon-button.

Pressing enter key on the icon-button is expected to open the action-menu, but
instead it incorrectly focuses the row-item.

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

[scottchen, 2017-07-10 22:36:46]

On 2017/07/10 18:27:58, stevenjb wrote:
> On 2017/07/08 00:07:00, scottchen wrote:
> > Just wanted to ping this CL to see if further discussion is to be had.
> 
> Is there an issue for this? What specifically is the problematic behavior here
> (i.e. please define 'bugging out'), and does the problem exist for any of the
> network lists?

Just updated the description and created a bug to link to. Basically, when you
tab  to focus the icon-button and press enter, instead of opening the
action-menu as expected, it'll focus the row item instead.

I did not see any network lists with a similar UI that opens any action-menu.

[stevenjb, 2017-07-10 22:48:53]

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:40:
},
On 2017/06/20 19:17:19, scottchen wrote:
> On 2017/06/20 18:46:51, dschuyler wrote:
> > When we have a chance, I'd like to chat about whether this should be made
into
> a
> > behavior (stevenjb@'s suggestion), or if it's really a one-off. And if it's
a
> > one-off, I'd like to explore (or push for) adjusting the UI here so that it
> > doesn't need special handling.
> 
> I think we might've talked about this before, but basically I had to do this
in
> FocusRowBehavior to avoid iron-list's Enter-key listener causing problems, but
> FocusRowBehavior can't be applied here, because the behavior assumes the row
> itself should not focusable (which is not the case here).
> 
> So possible solutions:
> 1) tweak focus-row-behavior to optionally allow the row itself to be focused
> (maybe based on a flag?) and behave accordingly, then apply the behavior here.
> 2) make the UI such that "row itself is focusable & actionable, but also has
> focusable & actionable controls as children"  is not a thing.
> 
> Also as a side note, I personally feel like network/bluetooth's row behavior
is
> weird, because pressing the Enter key on a focused row does completely
different
> things (i.e. connects if not already connected, or goes to a subpage if it is
> connected).

Can this be done more simply like:

<button ... on-keydown="doNothing_">

doNothing_: function(event) {
  event.stopPropagation();
}

?

[scottchen, 2017-07-12 19:09:51]

Patchset #3 (id:40001) has been deleted

[scottchen, 2017-07-12 19:12:49]

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:40:
},
On 2017/07/10 22:48:53, stevenjb wrote:
> On 2017/06/20 19:17:19, scottchen wrote:
> > On 2017/06/20 18:46:51, dschuyler wrote:
> > > When we have a chance, I'd like to chat about whether this should be made
> into
> > a
> > > behavior (stevenjb@'s suggestion), or if it's really a one-off. And if
it's
> a
> > > one-off, I'd like to explore (or push for) adjusting the UI here so that
it
> > > doesn't need special handling.
> > 
> > I think we might've talked about this before, but basically I had to do this
> in
> > FocusRowBehavior to avoid iron-list's Enter-key listener causing problems,
but
> > FocusRowBehavior can't be applied here, because the behavior assumes the row
> > itself should not focusable (which is not the case here).
> > 
> > So possible solutions:
> > 1) tweak focus-row-behavior to optionally allow the row itself to be focused
> > (maybe based on a flag?) and behave accordingly, then apply the behavior
here.
> > 2) make the UI such that "row itself is focusable & actionable, but also has
> > focusable & actionable controls as children"  is not a thing.
> > 
> > Also as a side note, I personally feel like network/bluetooth's row behavior
> is
> > weird, because pressing the Enter key on a focused row does completely
> different
> > things (i.e. connects if not already connected, or goes to a subpage if it
is
> > connected).
> 
> Can this be done more simply like:
> 
> <button ... on-keydown="doNothing_">
> 
> doNothing_: function(event) {
>   event.stopPropagation();
> }
> 
> ?

Looks like yes we can! PTAL.

[dschuyler, 2017-07-13 00:47:13]

LGTM with a nit and suggestion.

https://codereview.chromium.org/2945433002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:24:
doNothing_: function() {
/** @private */

and maybe(?) rename to something like ignoreEnterKey_.

[scottchen, 2017-07-13 18:34:00]

https://codereview.chromium.org/2945433002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
(right):

https://codereview.chromium.org/2945433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:24:
doNothing_: function() {
On 2017/07/13 00:47:13, dschuyler wrote:
> /** @private */
> 
> and maybe(?) rename to something like ignoreEnterKey_.

Done.

[scottchen, 2017-07-13 18:34:20]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-07-13 18:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2945433002/#ps80001
(title: "be explicit about event param")

[commit-bot: I haz the power, 2017-07-13 18:34:35]

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

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

[commit-bot: I haz the power, 2017-07-13 18:42:14]

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

[scottchen, 2017-07-13 22:22:29]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-07-13 22:23:03]

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-07-14 02:32:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499984549609910,
"parent_rev": "13b15a8d195c2adb9247bece2fcf7540350fde3d", "commit_rev":
"4c0b234413f027df2d8a0b1f638bdf5d14ee7a2b"}

[commit-bot: I haz the power, 2017-07-14 02:32:20]

Description was changed from

==========
MD Settings: fix incorrect behavior with Enter on bluetooth item's icon-button.

Pressing enter key on the icon-button is expected to open the action-menu, but
instead it incorrectly focuses the row-item.

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

to

==========
MD Settings: fix incorrect behavior with Enter on bluetooth item's icon-button.

Pressing enter key on the icon-button is expected to open the action-menu, but
instead it incorrectly focuses the row-item.

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

Review-Url: https://codereview.chromium.org/2945433002
Cr-Commit-Position: refs/heads/master@{#486641}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c0b234413f027df2d8a0b1f638b...
==========

[commit-bot: I haz the power, 2017-07-14 02:32:22]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4c0b234413f027df2d8a0b1f638b...