Settings People Revamp: Change Picture: Add accessibility-key usage.

Settings People Revamp: Change Picture: Add accessibility-key usage.

When the grid area is focused, arrow keys navigate between icons.
Space and Enter can be used to take pictures, discard pictures, and
select a new file. (Flipping photos requires tabbing to the flip button).

This behavior was pretty easy to implement, but has a caveat compared
to the old more advanced behavior.

The caveat is described in a mega-comment inside change_picture.js

BUG=563721
Committed: https://crrev.com/e2d58400ee8fcfd27b5927e0d00031ccb24df6ba
Cr-Commit-Position: refs/heads/master@{#378278}

[tommycli, 2016-02-19 23:59:00]

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

[tommycli, 2016-02-19 23:59:01]

dbeam: PTAL, this makes the it possible to navigate among choices in the image
grid using arrow keys, spacebar, and enter.

Tommy

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/camera.js (right):

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/camera.js:74: takePhoto:
function() {
Code is same, just made public and moved to public section.

[Dan Beam, 2016-02-22 20:41:31]

lgtm

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:226: if
(this.selectedItem_ == undefined)
nit: just if (!this.selectedItem_)

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:245: } while
(this.selectedItem_.hidden);
what assurance do we have that this breaks out of the loop?

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:248: break;
nit: \n

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:256: break;
nit: \n

[tommycli, 2016-02-29 19:18:33]

dbeam: Thanks!

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:226: if
(this.selectedItem_ == undefined)
On 2016/02/22 20:41:31, Dan Beam wrote:
> nit: just if (!this.selectedItem_)

Done.

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:245: } while
(this.selectedItem_.hidden);
On 2016/02/22 20:41:31, Dan Beam wrote:
> what assurance do we have that this breaks out of the loop?

I added a comment: This loop always terminates because the file and profile
icons are never hidden.

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:248: break;
On 2016/02/22 20:41:31, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/1720533002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:256: break;
On 2016/02/22 20:41:31, Dan Beam wrote:
> nit: \n

Done.

[tommycli, 2016-02-29 19:18:42]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-02-29 19:18:43]

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

[tommycli, 2016-02-29 19:18:46]

The CQ bit was unchecked by tommycli@chromium.org

[Dan Beam, 2016-02-29 19:26:01]

just fyi, still lgtm

https://codereview.chromium.org/1720533002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/camera.js (right):

https://codereview.chromium.org/1720533002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/camera.js:78: /** @type
{HTMLCanvasElement} */ (document.createElement('canvas'));
nit: don't put spaces between */ and (

[tommycli, 2016-02-29 20:28:25]

dbeam: thanks. I also had to update to compiled_resources2 and import a polymer
element to make the closure compiler happy.

https://codereview.chromium.org/1720533002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/camera.js (right):

https://codereview.chromium.org/1720533002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/camera.js:78: /** @type
{HTMLCanvasElement} */ (document.createElement('canvas'));
On 2016/02/29 19:26:01, Dan Beam wrote:
> nit: don't put spaces between */ and (

Hi. This was the result of git cl format, and seems to match the example set in
the closure annotations docs:
https://developers.google.com/closure/compiler/docs/js-for-compiler#instantia...

Thanks.

[tommycli, 2016-02-29 20:28:35]

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

[commit-bot: I haz the power, 2016-02-29 20:28:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1720533002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1720533002/100001

[commit-bot: I haz the power, 2016-02-29 21:09:56]

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

[commit-bot: I haz the power, 2016-02-29 21:09:56]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-02-29 21:25:18]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-02-29 21:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1720533002/#ps100001
(title: "Update people to compiled_resources2 to allow passing of closure")

[commit-bot: I haz the power, 2016-02-29 21:25:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1720533002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1720533002/100001

[commit-bot: I haz the power, 2016-02-29 21:38:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-29 21:40:16]

Description was changed from

==========
Settings People Revamp: Change Picture: Add accessibility-key usage.

When the grid area is focused, arrow keys navigate between icons.
Space and Enter can be used to take pictures, discard pictures, and
select a new file. (Flipping photos requires tabbing to the flip button).

This behavior was pretty easy to implement, but has a caveat compared
to the old more advanced behavior.

The caveat is described in a mega-comment inside change_picture.js

BUG=563721
==========

to

==========
Settings People Revamp: Change Picture: Add accessibility-key usage.

When the grid area is focused, arrow keys navigate between icons.
Space and Enter can be used to take pictures, discard pictures, and
select a new file. (Flipping photos requires tabbing to the flip button).

This behavior was pretty easy to implement, but has a caveat compared
to the old more advanced behavior.

The caveat is described in a mega-comment inside change_picture.js

BUG=563721

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

[commit-bot: I haz the power, 2016-02-29 21:40:17]

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

[Dan Beam, 2016-03-01 22:58:24]

https://codereview.chromium.org/1720533002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1720533002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:275: }
we should probably test this as well, btw

[tommycli, 2016-03-01 23:10:37]

https://codereview.chromium.org/1720533002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1720533002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/change_picture.js:275: }
On 2016/03/01 22:58:24, Dan Beam wrote:
> we should probably test this as well, btw

I'm down with that. I'll add that in a follow up CL.