MD Settings: adjust iron-list focus row behaviors.

MD Settings: adjust iron-list focus row behaviors.

In an effort to bring iron-list focus behaviors closer to other webUIs, this CL extracts code relating to "cr.ui.FocusRow" from MD history's <history-item>, and encapsulates it in a new FocusRowBehavior to be reused by other iron-list row custom elements.

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

Review-Url: https://codereview.chromium.org/2749513004
Cr-Commit-Position: refs/heads/master@{#458930}
Committed: https://chromium.googlesource.com/chromium/src/+/12e779b85deea4e5752fc02631aa9c3718aeb63e

[scottchen, 2017-03-14 22:03:38]

Description was changed from

==========
MD Settings: adjust iron-list focus row behaviors.

BUG=700608
==========

to

==========
MD Settings: adjust iron-list focus row behaviors.

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

[scottchen, 2017-03-14 23:09:22]

Description was changed from

==========
MD Settings: adjust iron-list focus row behaviors.

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

to

==========
MD Settings: adjust iron-list focus row behaviors.

In an effort to bring iron-list focus behaviors closer to other webUIs, this CL
extracts code relating to "cr.ui.FocusRow" from MD history's <history-item>, and
encapsulates it in a new FocusRowBehavior to be reused by other iron-list row
custom elements.

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

[scottchen, 2017-03-14 23:37:00]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, hcarmona@chromium.org

[scottchen, 2017-03-14 23:37:00]

I'm going to add unit tests, but wanted to kick off the review process since the
CL is not a small one. Feel free to start reviewing now or wait until I upload
tests.

I tried taking some screen capture of the new behavior, but since you can't
really see what I'm clicking/keying, I highly encourage patching locally and
playing with it. The linked bug also has descriptions of the desired behaviors.

Links to screen captures:
https://drive.google.com/open?id=0ByIc0PEad7DmNjZhbXB4LWtzWms
https://drive.google.com/open?id=0ByIc0PEad7DmcXN3a0hWSmRDeGc

[scottchen, 2017-03-15 23:49:09]

tests added along with minor cleanup in the focusRowBehavior code itself.

[hcarmona, 2017-03-16 21:31:54]

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:46: function
VirtualFocusRow(root, boundary, delegate) {
boundary is not used. Can we get rid of it and add later if we need it?

[Optional] You can change the next line to:
cr.ui.FocusRow.call(this, root, /* boundary */ null, delegate) so that the
meaning isn't lost.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:83: ironListTabIndex: {
This seems a bit redundant, can we use |tabindex| to avoid having two properties
that are the same?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:99: this.addItems_();
Should addItems_ be called before makeActive?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:104: this.listen(this,
'dom-change', 'onDomChange_');
Can this just call addItems_ directly?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:107: });
I think you're missing a .bind(this) on your function

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:146: this.tabIndex = -1;
Why are we setting the tabindex to -1 here?
Do we ever set it back to the original value?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.html
(right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.html:26:
<template is="dom-if" if="[[editable]]">
What happens if nothing is actionable? Where does focus go?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:34:
lastFocused_: Object,
Does this need to notify when changed?

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focus_row_behavior_test.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:12: <style></style>
Nit: style is optional, it can be removed when empty.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:49:
testElement.fire('focus');
I think events are async. This test might need to be changed to:

test('...', function(done) {
  testElement.$.controll.addEventListener('focus', done);
  testElement.focus();
});

This will fail if done isn't called, so no need to assert anything :-)

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:53: test('will focus
a similar item that was last focused', function() {
This test can be updated in a similar manner to what I mentioned above

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:72: // iron-list is
responsible for firing 'focus' after taps, but is not used
Does this apply to the other tests above? Maybe move this documentation to the
polymer element that you created.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:76: });
Lists can be empty, how should focus be handled in that case? Can we add a test
for that?

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(left):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:5:
suite('focusable-iron-list-item-behavior', function() {
Do we need to port over some of these tests?

click vs keyboard seems important

https://codereview.chromium.org/2749513004/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:108: * @param {string|HTMLElement}
query The selector of the element from this
nit: rename |query| to |selector|

[scottchen, 2017-03-17 21:26:34]

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

[scottchen, 2017-03-17 21:26:59]

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:46: function
VirtualFocusRow(root, boundary, delegate) {
On 2017/03/16 21:31:54, hcarmona wrote:
> boundary is not used. Can we get rid of it and add later if we need it?
> 
> [Optional] You can change the next line to:
> cr.ui.FocusRow.call(this, root, /* boundary */ null, delegate) so that the
> meaning isn't lost.

Good idea.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:83: ironListTabIndex: {
On 2017/03/16 21:31:54, hcarmona wrote:
> This seems a bit redundant, can we use |tabindex| to avoid having two
properties
> that are the same?

They're actually not always the same since the template only does a one-way
binding on both attributes, and the mechanism actually make use of this fact.
For example, when a control within row-one is focused, row-one has tabIndex = -1
(so that it can't be focused), and ironListTabIndex = 0 indicating that the row
is "active", as in someone inside of it is focused.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:99: this.addItems_();
On 2017/03/16 21:31:54, hcarmona wrote:
> Should addItems_ be called before makeActive?

Either way is fine, row_.makeActive() and row_.addItem() both have checks
built-in such that it's safe to invoke in either order.

See: 
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...

(Btw, either one will make one more 1-op call per element, so both are O(2n) ~=
O(n).)

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:104: this.listen(this,
'dom-change', 'onDomChange_');
On 2017/03/16 21:31:54, hcarmona wrote:
> Can this just call addItems_ directly?

Yep!

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:107: });
On 2017/03/16 21:31:54, hcarmona wrote:
> I think you're missing a .bind(this) on your function

"this" is provided as the first element of the afterNextRender method signature
(line 93 above). see: https://github.com/Polymer/polymer/issues/4104

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:146: this.tabIndex = -1;
On 2017/03/16 21:31:54, hcarmona wrote:
> Why are we setting the tabindex to -1 here?
> Do we ever set it back to the original value?

It's to make the row itself back to an "unselectable" state, once it forwards
the focus to its children, so that you can't shift-tab back to the row itself.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.html
(right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.html:26:
<template is="dom-if" if="[[editable]]">
On 2017/03/16 21:31:54, hcarmona wrote:
> What happens if nothing is actionable? Where does focus go?

Discussed offline and found no case of a row that has absolutely no focusable
control. I've adjusted the code to fail with an assert() instead of null
pointer, so that it's more obvious the assumption was intentional.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.js
(right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.js:34:
lastFocused_: Object,
On 2017/03/16 21:31:54, hcarmona wrote:
> Does this need to notify when changed?

Don't think so, it's always only changed by the child, and I think the child
notifying on this attribute will allow other children to see the change already.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focus_row_behavior_test.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:12: <style></style>
On 2017/03/16 21:31:54, hcarmona wrote:
> Nit: style is optional, it can be removed when empty.

Done.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:49:
testElement.fire('focus');
On 2017/03/16 21:31:54, hcarmona wrote:
> I think events are async. This test might need to be changed to:
> 
> test('...', function(done) {
>   testElement.$.controll.addEventListener('focus', done);
>   testElement.focus();
> });
> 
> This will fail if done isn't called, so no need to assert anything :-)

This was a surprise to me too, but apparently event handlers run synchronously
after the event dispatch.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:53: test('will focus
a similar item that was last focused', function() {
On 2017/03/16 21:31:54, hcarmona wrote:
> This test can be updated in a similar manner to what I mentioned above

same as above

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:72: // iron-list is
responsible for firing 'focus' after taps, but is not used
On 2017/03/16 21:31:54, hcarmona wrote:
> Does this apply to the other tests above? Maybe move this documentation to the
> polymer element that you created.

This is only relevant to testing mouse taps.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:76: });
On 2017/03/16 21:31:54, hcarmona wrote:
> Lists can be empty, how should focus be handled in that case? Can we add a
test
> for that?

I think that's for iron-list to test - the FocusRowBehavior only morphs the
behavior after iron-list focuses a row.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(left):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:5:
suite('focusable-iron-list-item-behavior', function() {
On 2017/03/16 21:31:54, hcarmona wrote:
> Do we need to port over some of these tests?
> 
> click vs keyboard seems important

I have a new mouseFocused flag which makes sure mouse clicks doesn't focus the
row or the icon.
The rest of the tests regarding keyboard interaction is now relying on iron-list
to do the right thing, so I didn't port them over (and assumes that iron-list
has proper unit-testing).

https://codereview.chromium.org/2749513004/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_row.js:108: * @param {string|HTMLElement}
query The selector of the element from this
On 2017/03/16 21:31:54, hcarmona wrote:
> nit: rename |query| to |selector|

good idea, wasn't sure what to name it.

[commit-bot: I haz the power, 2017-03-17 21:27:45]

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

[scottchen, 2017-03-17 21:27:51]

The CQ bit was unchecked by scottchen@chromium.org

[scottchen, 2017-03-17 21:29:11]

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

[commit-bot: I haz the power, 2017-03-17 21:29:47]

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-03-17 23:22:43]

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

[commit-bot: I haz the power, 2017-03-17 23:22:44]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hcarmona, 2017-03-17 23:55:51]

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:83: ironListTabIndex: {
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > This seems a bit redundant, can we use |tabindex| to avoid having two
> properties
> > that are the same?
> 
> They're actually not always the same since the template only does a one-way
> binding on both attributes, and the mechanism actually make use of this fact.
> For example, when a control within row-one is focused, row-one has tabIndex =
-1
> (so that it can't be focused), and ironListTabIndex = 0 indicating that the
row
> is "active", as in someone inside of it is focused. 

SG, can we add a quick comment to explain that here?

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:99: this.addItems_();
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > Should addItems_ be called before makeActive?
> 
> Either way is fine, row_.makeActive() and row_.addItem() both have checks
> built-in such that it's safe to invoke in either order.
> 
> See: 
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...
> 
> (Btw, either one will make one more 1-op call per element, so both are O(2n)
~=
> O(n).)

Acknowledged.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:107: });
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > I think you're missing a .bind(this) on your function
> 
> "this" is provided as the first element of the afterNextRender method
signature
> (line 93 above). see: https://github.com/Polymer/polymer/issues/4104

Acknowledged.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:146: this.tabIndex = -1;
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > Why are we setting the tabindex to -1 here?
> > Do we ever set it back to the original value?
> 
> It's to make the row itself back to an "unselectable" state, once it forwards
> the focus to its children, so that you can't shift-tab back to the row itself.

What happens if you tab away, can it be focused again later?

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focus_row_behavior_test.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:49:
testElement.fire('focus');
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > I think events are async. This test might need to be changed to:
> > 
> > test('...', function(done) {
> >   testElement.$.controll.addEventListener('focus', done);
> >   testElement.focus();
> > });
> > 
> > This will fail if done isn't called, so no need to assert anything :-)
> 
> This was a surprise to me too, but apparently event handlers run synchronously
> after the event dispatch.

Acknowledged.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:72: // iron-list is
responsible for firing 'focus' after taps, but is not used
On 2017/03/17 21:26:59, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > Does this apply to the other tests above? Maybe move this documentation to
the
> > polymer element that you created.
> 
> This is only relevant to testing mouse taps.

Acknowledged.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focus_row_behavior_test.js:76: });
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > Lists can be empty, how should focus be handled in that case? Can we add a
> test
> > for that?
> 
> I think that's for iron-list to test - the FocusRowBehavior only morphs the
> behavior after iron-list focuses a row.

Acknowledged.

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(left):

https://codereview.chromium.org/2749513004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:5:
suite('focusable-iron-list-item-behavior', function() {
On 2017/03/17 21:26:58, scottchen wrote:
> On 2017/03/16 21:31:54, hcarmona wrote:
> > Do we need to port over some of these tests?
> > 
> > click vs keyboard seems important
> 
> I have a new mouseFocused flag which makes sure mouse clicks doesn't focus the
> row or the icon.
> The rest of the tests regarding keyboard interaction is now relying on
iron-list
> to do the right thing, so I didn't port them over (and assumes that iron-list
> has proper unit-testing).

Acknowledged.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:102: this.listen(this,
'dom-change', 'onDomChange_');
I think you forgot to change it here

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:157: onDomChange_:
function() {
You can also delete the function if you're not using it anymore

[Dan Beam, 2017-03-18 00:09:57]

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:6: * @param
{{lastFocused: Object}} listItem
why {lastFocused: Object} instead of just an Element?

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:11: this.listItem =
listItem;
can this be @private?

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:96:
this.row_.makeActive(this.ironListTabIndex == 0);
nit: this.ironListTabIndexChanged_();

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:142: else {
needs curlies for if

if (this.lastFocused) {

} else {

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.js
(right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.js:21: is:
'settings-startup-url-entry',
\n

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.js:22:
behaviors: [FocusRowBehavior],
\n

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:108: * @param {string|HTMLElement}
selector The selector of the element from
selectorOrElement

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:116: if (selector instanceof
HTMLElement)
typeof selector == 'string' is better (works across frames)

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:119: element =
this.root.querySelector(/** @type {string} */ (selector));
if (typeof selectorOrElement == 'string')
  element = this.root.querySelector(selectorOrElement);

[scottchen, 2017-03-18 01:05:47]

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:83: ironListTabIndex: {
On 2017/03/17 23:55:50, hcarmona wrote:
> On 2017/03/17 21:26:58, scottchen wrote:
> > On 2017/03/16 21:31:54, hcarmona wrote:
> > > This seems a bit redundant, can we use |tabindex| to avoid having two
> > properties
> > > that are the same?
> > 
> > They're actually not always the same since the template only does a one-way
> > binding on both attributes, and the mechanism actually make use of this
fact.
> > For example, when a control within row-one is focused, row-one has tabIndex
=
> -1
> > (so that it can't be focused), and ironListTabIndex = 0 indicating that the
> row
> > is "active", as in someone inside of it is focused. 
> 
> SG, can we add a quick comment to explain that here?

Done.

https://codereview.chromium.org/2749513004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focus_row_behavior.js:146: this.tabIndex = -1;
On 2017/03/17 23:55:50, hcarmona wrote:
> On 2017/03/17 21:26:58, scottchen wrote:
> > On 2017/03/16 21:31:54, hcarmona wrote:
> > > Why are we setting the tabindex to -1 here?
> > > Do we ever set it back to the original value?
> > 
> > It's to make the row itself back to an "unselectable" state, once it
forwards
> > the focus to its children, so that you can't shift-tab back to the row
itself.
> 
> What happens if you tab away, can it be focused again later?

Yeah, since now the child's tabindex would be 0.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:6: * @param
{{lastFocused: Object}} listItem
On 2017/03/18 00:09:56, Dan Beam wrote:
> why {lastFocused: Object} instead of just an Element?

I believe this will make sure that the element passed in has "lastFocused"
defined as a property.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:11: this.listItem =
listItem;
On 2017/03/18 00:09:56, Dan Beam wrote:
> can this be @private?

Done.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:96:
this.row_.makeActive(this.ironListTabIndex == 0);
On 2017/03/18 00:09:56, Dan Beam wrote:
> nit: this.ironListTabIndexChanged_();

Done.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:102: this.listen(this,
'dom-change', 'onDomChange_');
On 2017/03/17 23:55:51, hcarmona wrote:
> I think you forgot to change it here

Done.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:142: else {
On 2017/03/18 00:09:56, Dan Beam wrote:
> needs curlies for if
> 
> if (this.lastFocused) {
> 
> } else {


Style guide says "Omit curly braces for single-line if statements". Or does it
not apply if any of the if-else-statements in the chain has curlies?

And for future reference, what if it's:

if
 ...
else if
 ..
else if
 ..
else {
 ..
 ..
}

Do we then add curly for all the one-line if and else-ifs?

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:157: onDomChange_:
function() {
On 2017/03/17 23:55:51, hcarmona wrote:
> You can also delete the function if you're not using it anymore

Done.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_url_entry.js
(right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.js:21: is:
'settings-startup-url-entry',
On 2017/03/18 00:09:56, Dan Beam wrote:
> \n

That's weird, I had \n and `git cl format --js` removed it, but now I tried the
formatter again and it doesn't remove them anymore. 

Re-added.

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_url_entry.js:22:
behaviors: [FocusRowBehavior],
On 2017/03/18 00:09:56, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:108: * @param {string|HTMLElement}
selector The selector of the element from
On 2017/03/18 00:09:57, Dan Beam wrote:
> selectorOrElement

Done.

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:116: if (selector instanceof
HTMLElement)
On 2017/03/18 00:09:57, Dan Beam wrote:
> typeof selector == 'string' is better (works across frames)

Acknowledged.

https://codereview.chromium.org/2749513004/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:119: element =
this.root.querySelector(/** @type {string} */ (selector));
On 2017/03/18 00:09:57, Dan Beam wrote:
> if (typeof selectorOrElement == 'string')
>   element = this.root.querySelector(selectorOrElement);

Done.

[hcarmona, 2017-03-20 18:32:50]

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:142: else {
On 2017/03/18 01:05:47, scottchen wrote:
> On 2017/03/18 00:09:56, Dan Beam wrote:
> > needs curlies for if
> > 
> > if (this.lastFocused) {
> > 
> > } else {
> 
> 
> Style guide says "Omit curly braces for single-line if statements". Or does it
> not apply if any of the if-else-statements in the chain has curlies?
> 
> And for future reference, what if it's:
> 
> if
>  ...
> else if
>  ..
> else if
>  ..
> else {
>  ..
>  ..
> }
> 
> Do we then add curly for all the one-line if and else-ifs?

+1 to dbeam's comment. I don't think the coding standard is clear on this case.
But it looks a bit odd if only part of the if chain has curlies.

https://codereview.chromium.org/2749513004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:12: this.listItem =
listItem;
Privates end w/ an underscore: listItem_

[scottchen, 2017-03-20 21:42:08]

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:142: else {
On 2017/03/20 18:32:50, hcarmona wrote:
> On 2017/03/18 01:05:47, scottchen wrote:
> > On 2017/03/18 00:09:56, Dan Beam wrote:
> > > needs curlies for if
> > > 
> > > if (this.lastFocused) {
> > > 
> > > } else {
> > 
> > 
> > Style guide says "Omit curly braces for single-line if statements". Or does
it
> > not apply if any of the if-else-statements in the chain has curlies?
> > 
> > And for future reference, what if it's:
> > 
> > if
> >  ...
> > else if
> >  ..
> > else if
> >  ..
> > else {
> >  ..
> >  ..
> > }
> > 
> > Do we then add curly for all the one-line if and else-ifs?
> 
> +1 to dbeam's comment. I don't think the coding standard is clear on this
case.
> But it looks a bit odd if only part of the if chain has curlies.

Acknowledged.

https://codereview.chromium.org/2749513004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:12: this.listItem =
listItem;
On 2017/03/20 18:32:50, hcarmona wrote:
> Privates end w/ an underscore: listItem_

Done.

[Dan Beam, 2017-03-21 07:11:53]

lgtm

https://codereview.chromium.org/2749513004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:134:
controls[i].getAttribute('type'),
can we make this focus-type instead of just type?

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:119: element = selectorOrElement;
why can't you just munge the argument?  because it's an argument and not
writeable?

[scottchen, 2017-03-21 20:10:34]

https://codereview.chromium.org/2749513004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/focus_row_behavior.js (right):

https://codereview.chromium.org/2749513004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/focus_row_behavior.js:134:
controls[i].getAttribute('type'),
On 2017/03/21 07:11:52, Dan Beam wrote:
> can we make this focus-type instead of just type?

Done.

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:119: element = selectorOrElement;
On 2017/03/21 07:11:52, Dan Beam wrote:
> why can't you just munge the argument?  because it's an argument and not
> writeable?

Could you give me an example of how you'd do the munging in this case?

[Dan Beam, 2017-03-21 20:51:56]

lgtm

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/2749513004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/focus_row.js:119: element = selectorOrElement;
On 2017/03/21 20:10:33, scottchen wrote:
> On 2017/03/21 07:11:52, Dan Beam wrote:
> > why can't you just munge the argument?  because it's an argument and not
> > writeable?
> 
> Could you give me an example of how you'd do the munging in this case?

I didn't see that there's a bunch of uses of |element| after this (otherwise I
would've just told you to overwrite selectorOrElement), but this is mildly
simpler if it compiles.

  if (typeof selectorOrElement == 'string')
    selectorOrElement = this.root.querySelector(selectorOrElement);

  if (!selectorOrElement)
    return false;

  var element = assertInstanceOf(selectorOrElement, Element);

[hcarmona, 2017-03-21 22:55:50]

LGTM

[scottchen, 2017-03-22 03:57:03]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-03-22 03:57:40]

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-03-22 04:39:39]

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

[commit-bot: I haz the power, 2017-03-22 04:39:40]

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

[scottchen, 2017-03-22 17:54:03]

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

[commit-bot: I haz the power, 2017-03-22 17:54:31]

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-03-22 18:56:30]

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

[commit-bot: I haz the power, 2017-03-22 18:56:31]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-03-22 21:13:39]

Patchset #11 (id:200001) has been deleted

[scottchen, 2017-03-22 23:17:56]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-22 23:17:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2749513004/#ps180001
(title: "fix test")

[commit-bot: I haz the power, 2017-03-22 23:18:43]

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-03-22 23:29:37]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490224676544490,
"parent_rev": "226e88f271fb6fc8a47110d440c9e12662edec95", "commit_rev":
"12e779b85deea4e5752fc02631aa9c3718aeb63e"}

[commit-bot: I haz the power, 2017-03-22 23:30:17]

Description was changed from

==========
MD Settings: adjust iron-list focus row behaviors.

In an effort to bring iron-list focus behaviors closer to other webUIs, this CL
extracts code relating to "cr.ui.FocusRow" from MD history's <history-item>, and
encapsulates it in a new FocusRowBehavior to be reused by other iron-list row
custom elements.

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

to

==========
MD Settings: adjust iron-list focus row behaviors.

In an effort to bring iron-list focus behaviors closer to other webUIs, this CL
extracts code relating to "cr.ui.FocusRow" from MD history's <history-item>, and
encapsulates it in a new FocusRowBehavior to be reused by other iron-list row
custom elements.

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

Review-Url: https://codereview.chromium.org/2749513004
Cr-Commit-Position: refs/heads/master@{#458930}
Committed:
https://chromium.googlesource.com/chromium/src/+/12e779b85deea4e5752fc02631aa...
==========

[commit-bot: I haz the power, 2017-03-22 23:30:18]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/12e779b85deea4e5752fc02631aa...