[MD Bookmarks] Convert bookmark list to iron-list.

[MD Bookmarks] Convert bookmark list to iron-list.

This CL makes the bookmark list an iron-list in order to deal with
excessively large folders and in preparation for implementing keyboard
navigation of the bookmark list.

As is, this patch will make a single item focusable in the bookmark list
which is then changed via the up and down arrows. The focus ring is only
shown for keyboard focus events.

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

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

[calamity, 2017-05-12 06:07:42]

Description was changed from

==========
[MD Bookmarks] Convert bookmark list to iron-list.

This CL makes the bookmark list an iron-list in order to deal with
excessively large folders and in preparation for implementing keyboard
navigation of the bookmark list.

As is, this patch will make a single item focusable in the bookmark list
which is then changed via the up and down arrows. The focus ring is only
shown for keyboard focus events.

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

to

==========
[MD Bookmarks] Convert bookmark list to iron-list.

This CL makes the bookmark list an iron-list in order to deal with
excessively large folders and in preparation for implementing keyboard
navigation of the bookmark list.

As is, this patch will make a single item focusable in the bookmark list
which is then changed via the up and down arrows. The focus ring is only
shown for keyboard focus events.

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

[calamity, 2017-05-12 06:14:57]

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

[commit-bot: I haz the power, 2017-05-12 06:15:17]

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-12 06:17:27]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-12 06:18:20]

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

[commit-bot: I haz the power, 2017-05-12 06:18:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[calamity, 2017-05-12 06:38:20]

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

[tsergeant, 2017-05-15 01:14:27]

The good news: iron-list seems to do its job well. Hooray! It makes opening a
folder with 1000 elements actually possible.

The bad news: The thing you alluded to with certain iron-list methods accepting
either an index or an element is really bad for us.

My test profile has a root folder that looks like this:

["6", "593", "9", "49", "31", "607", "25"]

When I press down from "607" (index 5), trying to get to index 6, iron-list will
instead focus the element with value "6" (at index 0).

This confusion between index and value seems pretty pervasive in the iron-list
API, and probably wouldn't be easy to fix.

One workaround I can think of is to wrap the values given to iron-list in dummy
objects:

[{itemId: "6"}, {itemId: "593"}, ...]

This is far from ideal, but might be necessary to fix the problem.

https://codereview.chromium.org/2869273011/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2869273011/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.js:125: this.mouseFocus_ = true;
I think this is going to be buggy. For example, I can mousedown on an item, drag
my mouse away from it and then mouseup on a different item. This will apply
mouseFocus_ without ever focusing the item, and if I then keyboard focus the
item, no outline will show up.

You could check the focus-ring polyfill to see the approach it takes (kind of
the opposite).

[calamity, 2017-05-16 02:47:54]

Patchset #3 (id:60001) has been deleted

[calamity, 2017-05-16 02:54:15]

Added the Object wrapper to items and the splice notifier.

https://codereview.chromium.org/2869273011/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2869273011/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.js:125: this.mouseFocus_ = true;
On 2017/05/15 01:14:27, tsergeant wrote:
> I think this is going to be buggy. For example, I can mousedown on an item,
drag
> my mouse away from it and then mouseup on a different item. This will apply
> mouseFocus_ without ever focusing the item, and if I then keyboard focus the
> item, no outline will show up.
> 
> You could check the focus-ring polyfill to see the approach it takes (kind of
> the opposite).

Added the focus handler to the button as discussed.

[tsergeant, 2017-05-16 03:41:51]

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:54: onDisplayedIdsChanged_:
function(newValue, oldValue) {
Comment here with why all this is necessary/useful

(+ closure annotations)

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:70: ...additions);
Check if uglify errors when parsing this file (I suspect it probably will). If
it does, you'll need to use call() to avoid it.

[calamity, 2017-05-16 05:51:18]

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

[commit-bot: I haz the power, 2017-05-16 05:51:40]

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-16 06:26:45]

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

[commit-bot: I haz the power, 2017-05-16 06:26:46]

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

[calamity, 2017-05-17 03:28:27]

ArraySplice not in Polymer externs. Is it kosher to add it to bookmarks externs
until I get my pull request merged?

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:54: onDisplayedIdsChanged_:
function(newValue, oldValue) {
On 2017/05/16 03:41:51, tsergeant wrote:
> Comment here with why all this is necessary/useful
> 
> (+ closure annotations)

Done.

https://codereview.chromium.org/2869273011/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:70: ...additions);
On 2017/05/16 03:41:51, tsergeant wrote:
> Check if uglify errors when parsing this file (I suspect it probably will). If
> it does, you'll need to use call() to avoid it.

Done.

[tsergeant, 2017-05-17 03:43:59]

Yup, I think it's okay to put it in types.js with a comment pointing to your
closure pull request.

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

https://codereview.chromium.org/2869273011/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:58: * focus on incremental update.
I think you should also comment somewhere (here or elsewhere) about why we wrap
the `id` in a dummy object.

[calamity, 2017-05-17 05:32:19]

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

[commit-bot: I haz the power, 2017-05-17 05:33:33]

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-17 05:33:53]

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

https://codereview.chromium.org/2869273011/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:58: * focus on incremental update.
On 2017/05/17 03:43:59, tsergeant wrote:
> I think you should also comment somewhere (here or elsewhere) about why we
wrap
> the `id` in a dummy object.

Added to displayedList_ above.

[tsergeant, 2017-05-17 07:05:38]

lgtm

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

https://codereview.chromium.org/2869273011/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.js:58: * focus on incremental update.
On 2017/05/17 05:33:53, calamity wrote:
> On 2017/05/17 03:43:59, tsergeant wrote:
> > I think you should also comment somewhere (here or elsewhere) about why we
> wrap
> > the `id` in a dummy object.
> 
> Added to displayedList_ above.

Great, thanks

[commit-bot: I haz the power, 2017-05-17 07:21:48]

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

[commit-bot: I haz the power, 2017-05-17 07:21:49]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-05-18 02:35:18]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-05-18 02:37:20]

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-18 03:31:06]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495074918464250,
"parent_rev": "9190eb27531ee100bcb83450c9c32c5568531afd", "commit_rev":
"f4af9a035994eace456a72a30d90186479f15238"}

[commit-bot: I haz the power, 2017-05-18 03:31:19]

Description was changed from

==========
[MD Bookmarks] Convert bookmark list to iron-list.

This CL makes the bookmark list an iron-list in order to deal with
excessively large folders and in preparation for implementing keyboard
navigation of the bookmark list.

As is, this patch will make a single item focusable in the bookmark list
which is then changed via the up and down arrows. The focus ring is only
shown for keyboard focus events.

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

to

==========
[MD Bookmarks] Convert bookmark list to iron-list.

This CL makes the bookmark list an iron-list in order to deal with
excessively large folders and in preparation for implementing keyboard
navigation of the bookmark list.

As is, this patch will make a single item focusable in the bookmark list
which is then changed via the up and down arrows. The focus ring is only
shown for keyboard focus events.

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

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

[commit-bot: I haz the power, 2017-05-18 03:31:20]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f4af9a035994eace456a72a30d90...