MD Settings: ignore modified key events in the profile avatar grid

MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b6daa9df237e81ebc10a0f176685582f3924cd9c
Cr-Commit-Position: refs/heads/master@{#439932}

[Dan Beam, 2016-12-19 00:18:38]

Description was changed from

==========
MD Settings: ignore modified key events in the profile avatar grid

R=mahmadi@chromium.org
BUG=675383
==========

to

==========
MD Settings: ignore modified key events in the profile avatar grid

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-12-19 01:11:37]

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

[commit-bot: I haz the power, 2016-12-19 01:11:46]

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, 2016-12-19 01:26:05]

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

[commit-bot: I haz the power, 2016-12-19 01:26:06]

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...)

[michaelpg, 2016-12-19 10:02:46]

thanks for fixing, will defer to moe for whether this is the intended behavior
but the modifier key test happened to catch my eye

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:27:
* @param {CustomEvent} event
! (since IronMenubarBehaviorImpl assumes it's non-null)

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:38:
* IronMenubarBehaviorImpl#_onLeftKey.
onRightKey

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:39:
* @param {CustomEvent} event
!

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:83:
return keyEv.altKey || keyEv.ctrlKey || keyEv.metaKey || keyEv.shiftKey;
maybe it's time for a function in util.js?
https://cs.chromium.org/search/?q=%5Bctrlaltmetashift%5D%7B3,5%7Dkey.*(%5C%7C...

[Dan Beam, 2016-12-20 07:07:49]

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

[Dan Beam, 2016-12-20 07:07:57]

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:27:
* @param {CustomEvent} event
On 2016/12/19 10:02:46, michaelpg wrote:
> ! (since IronMenubarBehaviorImpl assumes it's non-null)

Done.

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:38:
* IronMenubarBehaviorImpl#_onLeftKey.
On 2016/12/19 10:02:46, michaelpg wrote:
> onRightKey

Done.

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:39:
* @param {CustomEvent} event
On 2016/12/19 10:02:46, michaelpg wrote:
> !

Done.

https://codereview.chromium.org/2586113002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:83:
return keyEv.altKey || keyEv.ctrlKey || keyEv.metaKey || keyEv.shiftKey;
On 2016/12/19 10:02:45, michaelpg wrote:
> maybe it's time for a function in util.js?
>
https://cs.chromium.org/search/?q=%5Bctrlaltmetashift%5D%7B3,5%7Dkey.*(%5C%7C...

Done.

[commit-bot: I haz the power, 2016-12-20 07:08:01]

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, 2016-12-20 07:21:01]

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

[commit-bot: I haz the power, 2016-12-20 07:21:02]

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...)

[Dan Beam, 2016-12-20 07:39:02]

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

[commit-bot: I haz the power, 2016-12-20 07:39:26]

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, 2016-12-20 07:50:44]

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

[commit-bot: I haz the power, 2016-12-20 07:50:44]

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...)

[Dan Beam, 2016-12-20 18:01:31]

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

[commit-bot: I haz the power, 2016-12-20 18:02:03]

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, 2016-12-20 18:15:43]

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

[commit-bot: I haz the power, 2016-12-20 18:15: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...)

[Moe, 2016-12-20 19:07:40]

lgtm with nits.

https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:105:
selector.ignoreModifiedKeyEvents = true;
maybe set the property on avatarSelector to test the avatarSelector->selector
binding as well.

https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:31:
if (this.ignoreModifiedKeyEvents && this.hasKeyModifiers_(event))
nit: please add a comment why this is being overridden. for example why we check
for key modifiers in _onLeftKey and _onRightKey and not _onUpKey and _onDownKey.

[Moe, 2016-12-20 19:08:59]

Also, I think it helps to expand the description of the CL why we're doing this.
The purpose of it wasn't clear to me until I patched and tested the CL :)

[Dan Beam, 2016-12-20 19:24:18]

Description was changed from

==========
MD Settings: ignore modified key events in the profile avatar grid

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-12-20 19:31:15]

On 2016/12/20 19:08:59, moe wrote:
> Also, I think it helps to expand the description of the CL why we're doing
this.
> The purpose of it wasn't clear to me until I patched and tested the CL :)

Done.

https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:105:
selector.ignoreModifiedKeyEvents = true;
On 2016/12/20 19:07:40, moe wrote:
> maybe set the property on avatarSelector to test the avatarSelector->selector
> binding as well.

Done.

https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:31:
if (this.ignoreModifiedKeyEvents && this.hasKeyModifiers_(event))
On 2016/12/20 19:07:40, moe wrote:
> nit: please add a comment why this is being overridden. for example why we
check
> for key modifiers in _onLeftKey and _onRightKey and not _onUpKey and
_onDownKey.

Done.

[Dan Beam, 2016-12-20 19:31:26]

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

[Moe, 2016-12-20 19:32:22]

On 2016/12/20 19:31:15, Dan Beam wrote:
> On 2016/12/20 19:08:59, moe wrote:
> > Also, I think it helps to expand the description of the CL why we're doing
> this.
> > The purpose of it wasn't clear to me until I patched and tested the CL :)
> 
> Done.
> 
>
https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
> File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
> (right):
> 
>
https://codereview.chromium.org/2586113002/diff/100001/chrome/test/data/webui...
> chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:105:
> selector.ignoreModifiedKeyEvents = true;
> On 2016/12/20 19:07:40, moe wrote:
> > maybe set the property on avatarSelector to test the
avatarSelector->selector
> > binding as well.
> 
> Done.
> 
>
https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
> File
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
> (right):
> 
>
https://codereview.chromium.org/2586113002/diff/100001/ui/webui/resources/cr_...
>
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:31:
> if (this.ignoreModifiedKeyEvents && this.hasKeyModifiers_(event))
> On 2016/12/20 19:07:40, moe wrote:
> > nit: please add a comment why this is being overridden. for example why we
> check
> > for key modifiers in _onLeftKey and _onRightKey and not _onUpKey and
> _onDownKey.
> 
> Done.

Thanks. lgtm.

[commit-bot: I haz the power, 2016-12-20 19:32:26]

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

[Dan Beam, 2016-12-20 19:34:12]

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

[commit-bot: I haz the power, 2016-12-20 19:34:56]

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

[Dan Beam, 2016-12-20 22:02:09]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2016-12-20 22:02:13]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-12-20 22:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mahmadi@chromium.org
Link to the patchset: https://codereview.chromium.org/2586113002/#ps140001
(title: "closure")

[commit-bot: I haz the power, 2016-12-20 22:03:02]

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, 2016-12-20 23:48:56]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482271333698770,
"parent_rev": "43fc7e1a49b625b59ef743a2ad4488eae828718e", "commit_rev":
"ad00424f9940d61f45b63a929a0cbac1097f1495"}

[commit-bot: I haz the power, 2016-12-20 23:49:22]

Description was changed from

==========
MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2586113002
==========

[commit-bot: I haz the power, 2016-12-20 23:49:24]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-20 23:51:59]

Description was changed from

==========
MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2586113002
==========

to

==========
MD Settings: ignore modified key events in the profile avatar grid

It responds to left and right arrow, which can sometimes prevent
browser shortcuts like Back and Forward if not done correctly.

R=mahmadi@chromium.org
BUG=675383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b6daa9df237e81ebc10a0f176685582f3924cd9c
Cr-Commit-Position: refs/heads/master@{#439932}
==========

[commit-bot: I haz the power, 2016-12-20 23:52:00]

Patchset 8 (id:??) landed as
https://crrev.com/b6daa9df237e81ebc10a0f176685582f3924cd9c
Cr-Commit-Position: refs/heads/master@{#439932}