[MD History] Add keyboard navigation to the main history list.

[MD History] Add keyboard navigation to the main history list.

This CL adds arrow-key navigation to the main history list and makes a
single history item tab-navigable by using cr.ui.FocusRow and
iron-list's tab index property.

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

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

[calamity, 2016-09-12 05:02:24]

Description was changed from

==========
[MD History] Add keyboard navigation to the main history list.

This CL adds arrow-key navigation to the main history list and makes a
single history item tab-navigable by using cr.ui.FocusRow and
iron-list's tab index property.

BUG=623011
==========

to

==========
[MD History] Add keyboard navigation to the main history list.

This CL adds arrow-key navigation to the main history list and makes a
single history item tab-navigable by using cr.ui.FocusRow and
iron-list's tab index property.

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

[calamity, 2016-09-12 07:17:31]

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

[calamity, 2016-09-12 07:17:32]

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:12: function
HistoryFocusRow(root, boundary, delegate) {
This is outside to make closure happy.

[tsergeant, 2016-09-13 00:55:08]

I can no longer press enter to activate the #title link, so something has gone
wrong there.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:24: switch
(this.getTypeForElement(sampleElement)) {
Is the switch statement necessary here?

Can you just do

if (this.getTypeForElement(sampleElement) == 'star')

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:120: /** @type
{HistoryFocusRow} */
Nit: @private {HistoryFocusRow}

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:121: row_: Object,
Does this have any need to be a Polymer property? If not, just move it out into
the element as a private member variable.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:126: 'click': 'onFocus_',
It seems like this click handler isn't necessary? Just the focus handler by
itself seems to have the same effect.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:130: attached: function() {
/** @override */

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:131: this.row_ = new
HistoryFocusRow(
I think it would be good to defer this work until after next render. A profile
is showing at an aggregate of 15ms is spent creating focus rows, plus a little
bit for the listeners above.

Instead,

Polymer.RenderStatus.afterNextRender(this, function() {
  this.row_ = new HistoryFocusRow(...)
  this.listen(this, 'focus', 'onFocus_');
  ...
});

[calamity, 2016-09-15 02:00:23]

The enter issue has been fixed by not propagating Enter keyevents to the
iron-list which was taking them and refocusing the history item.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:24: switch
(this.getTypeForElement(sampleElement)) {
On 2016/09/13 00:55:08, tsergeant wrote:
> Is the switch statement necessary here?
> 
> Can you just do
> 
> if (this.getTypeForElement(sampleElement) == 'star')

Oops, I just cargo culted from the old history page. Done.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:120: /** @type
{HistoryFocusRow} */
On 2016/09/13 00:55:08, tsergeant wrote:
> Nit: @private {HistoryFocusRow}

Done.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:121: row_: Object,
On 2016/09/13 00:55:08, tsergeant wrote:
> Does this have any need to be a Polymer property? If not, just move it out
into
> the element as a private member variable.

Done.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:126: 'click': 'onFocus_',
On 2016/09/13 00:55:08, tsergeant wrote:
> It seems like this click handler isn't necessary? Just the focus handler by
> itself seems to have the same effect.

Clicking again on a focused row defocuses the row. That was kinda weird. Up to
you if it's worth dealing with.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:130: attached: function() {
On 2016/09/13 00:55:08, tsergeant wrote:
> /** @override */

Done.

https://codereview.chromium.org/2334553002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:131: this.row_ = new
HistoryFocusRow(
On 2016/09/13 00:55:08, tsergeant wrote:
> I think it would be good to defer this work until after next render. A profile
> is showing at an aggregate of 15ms is spent creating focus rows, plus a little
> bit for the listeners above.
> 
> Instead,
> 
> Polymer.RenderStatus.afterNextRender(this, function() {
>   this.row_ = new HistoryFocusRow(...)
>   this.listen(this, 'focus', 'onFocus_');
>   ...
> });

Good catch! Done.

[tsergeant, 2016-09-15 03:00:46]

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:71: e.stopPropagation();
Maybe put a little comment here, along the lines of

// Prevent iron-list from attempting to handle the event

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:121: listeners: {
Remove this and the blank lines after it

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:138: },
I think that we'll probably need a detached function which unlistens to these
things, eg:
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html....

I don't know if we ever detach history-items at the moment (maybe in tests?),
but it's better safe than sorry.

[calamity, 2016-09-15 08:33:58]

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

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

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

[calamity, 2016-09-15 08:35:10]

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

[calamity, 2016-09-15 08:35:11]

+dbeam for tiny focus_row.js change.

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:71: e.stopPropagation();
On 2016/09/15 03:00:46, tsergeant wrote:
> Maybe put a little comment here, along the lines of
> 
> // Prevent iron-list from attempting to handle the event

Done.

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:121: listeners: {
On 2016/09/15 03:00:45, tsergeant wrote:
> Remove this and the blank lines after it

Done.

https://codereview.chromium.org/2334553002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:138: },
On 2016/09/15 03:00:46, tsergeant wrote:
> I think that we'll probably need a detached function which unlistens to these
> things, eg:
>
https://github.com/PolymerElements/paper-ripple/blob/master/paper-ripple.html....
> 
> I don't know if we ever detach history-items at the moment (maybe in tests?),
> but it's better safe than sorry.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-09-15 16:51:10]

https://codereview.chromium.org/2334553002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/2334553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:45: * @param {{lastFocused:
Object}} historyItemElement
why this type? avoid circular dependencies?

can we make this an @interface and @implement it by HistoryItemElement?  does
that work yet?

https://codereview.chromium.org/2334553002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/2334553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.html:39:
tab-index="[[tabIndex]]"
can we just name this something else?  iron-list-tab-index?

this might invoke HTMLElement#tabIndex, which is a DOM 0 accessor
semi-equivalent to setAttribute('tabindex', ...)

[Dan Beam, 2016-09-15 16:51:44]

ultimately focus_row lgtm and this overall looks like it'll improve the a11y
story

[tsergeant, 2016-09-19 07:02:41]

lgtm

[calamity, 2016-09-20 03:03:40]

Patchset #4 (id:60001) has been deleted

[calamity, 2016-09-20 03:06:54]

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

[commit-bot: I haz the power, 2016-09-20 03:07:10]

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-09-20 03:09:22]

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

[commit-bot: I haz the power, 2016-09-20 03:09:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[calamity, 2016-09-20 06:42:25]

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

[commit-bot: I haz the power, 2016-09-20 06:42:42]

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-09-20 07:46:01]

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

[commit-bot: I haz the power, 2016-09-20 07:46:02]

Dry run: This issue passed the CQ dry run.

[calamity, 2016-09-21 02:41:24]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-09-21 02:41:25]

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

[commit-bot: I haz the power, 2016-09-21 02:41:52]

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-09-21 02:47:07]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-21 02:48:28]

Description was changed from

==========
[MD History] Add keyboard navigation to the main history list.

This CL adds arrow-key navigation to the main history list and makes a
single history item tab-navigable by using cr.ui.FocusRow and
iron-list's tab index property.

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

to

==========
[MD History] Add keyboard navigation to the main history list.

This CL adds arrow-key navigation to the main history list and makes a
single history item tab-navigable by using cr.ui.FocusRow and
iron-list's tab index property.

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

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

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

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

[Dan Beam, 2016-09-28 22:13:09]

ultimately we should port this list-list tab-index observation to downloads
(which shipped before this existed, afaik)