Profile Avatar Selector: Allow arrow keys to be used for moving between avatars (2nd edition)

Profile Avatar Selector: Allow user to navigate between avatars using arrow keys

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

[Moe, 2016-09-07 15:43:53]

Description was changed from

==========
Profile Avatar Selector: Allow user to navigate between avatars using arrow keys

BUG=639843
==========

to

==========
Profile Avatar Selector: Allow user to navigate between avatars using arrow keys

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

[Moe, 2016-09-07 15:44:31]

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

[commit-bot: I haz the power, 2016-09-07 15:44:52]

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

[Moe, 2016-09-07 15:45:36]

Description was changed from

==========
Profile Avatar Selector: Allow user to navigate between avatars using arrow keys

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

to

==========
Profile Avatar Selector: Allow user to navigate between avatars using arrow keys

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

[Moe, 2016-09-07 15:45:36]

mahmadi@chromium.org changed reviewers:
+ dbeam@chromium.org

[commit-bot: I haz the power, 2016-09-07 17:17:15]

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

[commit-bot: I haz the power, 2016-09-07 17:17:16]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-09-12 14:07:37]

Hi Dan,

just a quick ping that this CL is an alternative to
https://codereview.chromium.org/2269743002 and needs your review.

Context:

We would like all four arrow keys to be used to navigate between the profile
avatar icons.

Left/Right are handled in IronMenubarBehavior which is not what paper-lisbox
uses (it uses IronMenuBehavior). However IronMenubarBehavior overrides the
up/down key handlers to be used for selection rather than
navigation which isn't what we want.

This CL creates a new selector element that
has both behaviors in a particular order which allows the four arrow keys to be
used for navigation (and correctly handling RTL).

[Dan Beam, 2016-09-15 16:42:05]

can we just update the upstream code to use addOwnKeyBinding()?  is it
intentional that they're fighting eachother?

https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
(right):

https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:18:
// Polymer.IronMenuBehavior.
ehhhhhh, depending on order is kinda lame

[Moe, 2016-09-15 17:34:14]

On 2016/09/15 16:42:05, Dan Beam wrote:
> can we just update the upstream code to use addOwnKeyBinding()?  is it
> intentional that they're fighting eachother?
> 
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> File
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> (right):
> 
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:18:
> // Polymer.IronMenuBehavior.
> ehhhhhh, depending on order is kinda lame

is this what you're proposing?

if (RLT)
  avatarList.addOwnKeyBinding('left', '_onDownKey');
  avatarList.addOwnKeyBinding('right', '_onUpKey');
else
  avatarList.addOwnKeyBinding('left', '_onUpKey');
  avatarList.addOwnKeyBinding('right', '_onDownKey');

[Dan Beam, 2016-09-15 17:39:14]

On 2016/09/15 17:34:14, moe wrote:
> On 2016/09/15 16:42:05, Dan Beam wrote:
> > can we just update the upstream code to use addOwnKeyBinding()?  is it
> > intentional that they're fighting eachother?
> > 
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > File
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:18:
> > // Polymer.IronMenuBehavior.
> > ehhhhhh, depending on order is kinda lame
> 
> is this what you're proposing?
> 
> if (RLT)
>   avatarList.addOwnKeyBinding('left', '_onDownKey');
>   avatarList.addOwnKeyBinding('right', '_onUpKey');
> else
>   avatarList.addOwnKeyBinding('left', '_onUpKey');
>   avatarList.addOwnKeyBinding('right', '_onDownKey');

so, in these behaviors, there are keyBindings properties.  when you mix in 2
behaviors with the same keyBinding properties, do they clobber eachother?

[Dan Beam, 2016-09-15 17:41:28]

On 2016/09/15 17:39:14, Dan Beam wrote:
> On 2016/09/15 17:34:14, moe wrote:
> > On 2016/09/15 16:42:05, Dan Beam wrote:
> > > can we just update the upstream code to use addOwnKeyBinding()?  is it
> > > intentional that they're fighting eachother?
> > > 
> > >
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > > File
> > >
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > >
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:18:
> > > // Polymer.IronMenuBehavior.
> > > ehhhhhh, depending on order is kinda lame
> > 
> > is this what you're proposing?
> > 
> > if (RLT)
> >   avatarList.addOwnKeyBinding('left', '_onDownKey');
> >   avatarList.addOwnKeyBinding('right', '_onUpKey');
> > else
> >   avatarList.addOwnKeyBinding('left', '_onUpKey');
> >   avatarList.addOwnKeyBinding('right', '_onDownKey');
> 
> so, in these behaviors, there are keyBindings properties.  when you mix in 2
> behaviors with the same keyBinding properties, do they clobber eachother?

ahhh, they should be able to coexist
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

[Dan Beam, 2016-09-15 17:47:03]

https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
(right):

https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:19:
Polymer.IronMenubarBehavior,
can we just just NOT use the full IronMenubarBehavior?  and just copy the parts
that aren't silly for our use case?

[Moe, 2016-09-15 17:47:29]

On 2016/09/15 17:41:28, Dan Beam wrote:
> On 2016/09/15 17:39:14, Dan Beam wrote:
> > On 2016/09/15 17:34:14, moe wrote:
> > > On 2016/09/15 16:42:05, Dan Beam wrote:
> > > > can we just update the upstream code to use addOwnKeyBinding()?  is it
> > > > intentional that they're fighting eachother?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > > > File
> > > >
> > >
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > > >
> > >
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:18:
> > > > // Polymer.IronMenuBehavior.
> > > > ehhhhhh, depending on order is kinda lame
> > > 
> > > is this what you're proposing?
> > > 
> > > if (RLT)
> > >   avatarList.addOwnKeyBinding('left', '_onDownKey');
> > >   avatarList.addOwnKeyBinding('right', '_onUpKey');
> > > else
> > >   avatarList.addOwnKeyBinding('left', '_onUpKey');
> > >   avatarList.addOwnKeyBinding('right', '_onDownKey');
> > 
> > so, in these behaviors, there are keyBindings properties.  when you mix in 2
> > behaviors with the same keyBinding properties, do they clobber eachother?
> 
> ahhh, they should be able to coexist
>
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

correct. the problem are the handlers that are being overridden.

[Moe, 2016-09-15 17:51:35]

On 2016/09/15 17:47:03, Dan Beam wrote:
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> File
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> (right):
> 
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:19:
> Polymer.IronMenubarBehavior,
> can we just just NOT use the full IronMenubarBehavior?  and just copy the
parts
> that aren't silly for our use case?

sgtm. I'll go ahead and update the version 1 CL with the following and copy get
_isRTL() from the IronMenubarBehavior.

if (this._isRTL)
  avatarList.addOwnKeyBinding('left', '_onDownKey');
  avatarList.addOwnKeyBinding('right', '_onUpKey');
else
  avatarList.addOwnKeyBinding('left', '_onUpKey');
  avatarList.addOwnKeyBinding('right', '_onDownKey');

[Moe, 2016-09-15 17:52:00]

On 2016/09/15 17:51:35, moe wrote:
> On 2016/09/15 17:47:03, Dan Beam wrote:
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> > File
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2319123002/diff/1/ui/webui/resources/cr_eleme...
> >
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_listbox.js:19:
> > Polymer.IronMenubarBehavior,
> > can we just just NOT use the full IronMenubarBehavior?  and just copy the
> parts
> > that aren't silly for our use case?
> 
> sgtm. I'll go ahead and update the version 1 CL with the following and copy
get
> _isRTL() from the IronMenubarBehavior.
> 
> if (this._isRTL)
>   avatarList.addOwnKeyBinding('left', '_onDownKey');
>   avatarList.addOwnKeyBinding('right', '_onUpKey');
> else
>   avatarList.addOwnKeyBinding('left', '_onUpKey');
>   avatarList.addOwnKeyBinding('right', '_onDownKey');

closing this CL...