[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added
upstream as stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

Review-Url: https://codereview.chromium.org/2885723002
Cr-Commit-Position: refs/heads/master@{#474618}
Committed: https://chromium.googlesource.com/chromium/src/+/f959b2ddad3703105af2a3caf02944b58eabd260

[calamity, 2017-05-16 04:46:38]

Description was changed from

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL also makes a method in iron-list public and exposes 2 properties
through getters.

BUG=692844
==========

to

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL also makes a method in iron-list public and exposes 2 properties
through getters.

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

[calamity, 2017-05-19 06:47:47]

Patchset #2 (id:20001) has been deleted

[calamity, 2017-05-19 06:51:01]

Patchset #2 (id:40001) has been deleted

[calamity, 2017-05-19 06:51:12]

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

[calamity, 2017-05-19 06:51:22]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[commit-bot: I haz the power, 2017-05-19 06:51:50]

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-05-19 07:05:43]

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

[commit-bot: I haz the power, 2017-05-19 07:05: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...)

[tsergeant, 2017-05-22 05:19:00]

Generally looks good.

My main concern is with the iron-list changes. A quick check of the Github seems
that they're also considering adding the focus method that you need:
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315

Have you contacted them to see whether they can accommodate these changes? It
would be ideal for us to not need to fork our implementation.

Failing that, I think the preferred way to add things to Polymer elements is
with third_party/polymer/v1_0/chromium.patch, which theoretically will allow the
patch to stick when the element is updated.

https://codereview.chromium.org/2885723002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right):

https://codereview.chromium.org/2885723002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:296:
assertDeepEquals(['2'], normalizeSet(store.data.selection.items));
Should you be checking that the anchor changes here?

[calamity, 2017-05-23 05:23:38]

Patchset #3 (id:80001) has been deleted

[calamity, 2017-05-23 05:25:47]

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

[commit-bot: I haz the power, 2017-05-23 05:26:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-05-23 05:27:29]

Added the focusItem method to chromium.patch and removed dependency on focused
index from within iron-list.

This patchset also fixes some weirdness brought on by other recent changes,
particularly with how tabbing through the page worked.

https://codereview.chromium.org/2885723002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right):

https://codereview.chromium.org/2885723002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:296:
assertDeepEquals(['2'], normalizeSet(store.data.selection.items));
On 2017/05/22 05:19:00, tsergeant wrote:
> Should you be checking that the anchor changes here?

Done.

[tsergeant, 2017-05-23 07:25:17]

CC dbeam@ so you're aware of the little change we're making to iron-list, in a
way that Polymer seems to approve of.

Chris, you'll need to update the commit message where it mentions what has been
changed in iron-list.

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/item.html:69: <a id="url"
href="[[openItemUrl_]]" tabindex="-1">
As discussed, maybe change this <a> back into a <div> in this CL (😢), and then
we can follow up to re-enable middle click later.

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:113: 
Nit: There's an extra \n here

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:114: getIndexForItemElement_:
function(el) {
Nit: Closure annotations here.

[commit-bot: I haz the power, 2017-05-23 07:27:03]

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

[commit-bot: I haz the power, 2017-05-23 07:27:04]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-23 16:25:51]

On 2017/05/23 07:25:17, tsergeant wrote:
> CC dbeam@ so you're aware of the little change we're making to iron-list, in a
> way that Polymer seems to approve of.

are you referring to this?
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315
https://github.com/PolymerElements/iron-list/compare/blur-removed-item

[Dan Beam, 2017-05-23 16:26:28]

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

[Dan Beam, 2017-05-23 16:26:29]

if so that makes sense

lgtm but please reference the upstream stuff so there's a prayer we unfork this
usefully in the future

[tsergeant, 2017-05-23 23:07:49]

> are you referring to this?
> https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315
> https://github.com/PolymerElements/iron-list/compare/blur-removed-item

Yup, that's where this change comes from. I believe Chris also emailed Keanu to
talk about getting that landed.

> lgtm but please reference the upstream stuff so there's a prayer we unfork
this
> usefully in the future

Where is the best place to reference it? In the patch directly, or in the README
file?

[Dan Beam, 2017-05-24 02:04:48]

On 2017/05/23 23:07:49, tsergeant wrote:
> > are you referring to this?
> > https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315
> > https://github.com/PolymerElements/iron-list/compare/blur-removed-item
> 
> Yup, that's where this change comes from. I believe Chris also emailed Keanu
to
> talk about getting that landed.
> 
> > lgtm but please reference the upstream stuff so there's a prayer we unfork
> this
> > usefully in the future
> 
> Where is the best place to reference it? In the patch directly, or in the
README
> file?

just slap those ^ links in the CL desc?

[calamity, 2017-05-24 03:57:59]

Description was changed from

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL also makes a method in iron-list public and exposes 2 properties
through getters.

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

to

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added upstream as
stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

[calamity, 2017-05-24 03:58:37]

Description was changed from

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added upstream as
stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

to

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added
upstream as stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

[calamity, 2017-05-24 04:13:37]

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

[commit-bot: I haz the power, 2017-05-24 04:14:24]

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-05-24 05:07:15]

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

[commit-bot: I haz the power, 2017-05-24 05:07:16]

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

[calamity, 2017-05-24 08:06:24]

Patchset #4 (id:120001) has been deleted

[calamity, 2017-05-24 08:06:25]

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

[commit-bot: I haz the power, 2017-05-24 08:06:44]

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-05-24 09:42:22]

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

[commit-bot: I haz the power, 2017-05-24 09:42:23]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-05-25 07:36:31]

CL description updated.

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/item.html:69: <a id="url"
href="[[openItemUrl_]]" tabindex="-1">
On 2017/05/23 07:25:17, tsergeant wrote:
> As discussed, maybe change this <a> back into a <div> in this CL (😢), and then
> we can follow up to re-enable middle click later.

Acknowledged.

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:113: 
On 2017/05/23 07:25:17, tsergeant wrote:
> Nit: There's an extra \n here

Done.

https://codereview.chromium.org/2885723002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:114: getIndexForItemElement_:
function(el) {
On 2017/05/23 07:25:17, tsergeant wrote:
> Nit: Closure annotations here.

Done.

[tsergeant, 2017-05-25 07:47:50]

lgtm

[calamity, 2017-05-25 08:41:12]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-05-25 08:41:13]

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/2885723002/#ps140001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-25 08:41:30]

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-05-25 10:10:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495701672994470,
"parent_rev": "a161d3858abb3b14048fbbfc76d164f62dcdd15a", "commit_rev":
"f959b2ddad3703105af2a3caf02944b58eabd260"}

[commit-bot: I haz the power, 2017-05-25 10:10:40]

Description was changed from

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added
upstream as stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

to

==========
[MD Bookmarks] Add keyboard navigation and selection to bookmark list.

This CL adds keyboard navigation to the bookmarks list via the Up and
Down arrow keys, Shift for range select, and Ctrl for adding individual
items.

This CL adds a method to iron-list which will eventually be added
upstream as stated in
https://github.com/PolymerElements/iron-list/pull/427#issuecomment-300020315.

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

Review-Url: https://codereview.chromium.org/2885723002
Cr-Commit-Position: refs/heads/master@{#474618}
Committed:
https://chromium.googlesource.com/chromium/src/+/f959b2ddad3703105af2a3caf029...
==========

[commit-bot: I haz the power, 2017-05-25 10:10:41]

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