MD Settings: fix unnecessary focus on list items.

MD Settings: fix unnecessary focus on list items.

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

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

[scottchen, 2017-03-10 01:55:32]

Description was changed from

==========
MD Settings: fix unnecessary focus on list items.

BUG=699403
==========

to

==========
MD Settings: fix unnecessary focus on list items.

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

[scottchen, 2017-03-10 01:57:03]

scottchen@chromium.org changed reviewers:
+ dpapad@chromium.org

[scottchen, 2017-03-10 03:11:13]

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

[commit-bot: I haz the power, 2017-03-10 03:11:39]

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-10 03:34:39]

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

[commit-bot: I haz the power, 2017-03-10 03:34:39]

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

[scottchen, 2017-03-10 19:09:49]

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

[commit-bot: I haz the power, 2017-03-10 19:10: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-03-10 20:13:57]

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

[commit-bot: I haz the power, 2017-03-10 20:13:58]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-03-11 00:04:47]

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/focusable_iron_list_item_behavior.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:31: *
Unflag when moving away via keyboard (e.g. tabbing onto its children).
@param missing.

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:38: if
(event.key == "Enter")
I think that the convention is to always use single quotes in JS, double quotes
in HTML.

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:47:
document.body.addEventListener('keydown', function() {
This listener is leaked (it will be active for any subsequent tests). We should
remove it, or probably easier to wrap the element you are testing with a <div>,
add the listener on the <div> instead. Then, when clearBody() is called the
<div> itself will be removed from the DOM, and no listeners will be leaked.

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:53:
testElement.async(function() {
Tried this locally. It does not seem that the async() call is needed. Firing an
event synchronously fires the listener.

Also how about simplifying this test as follows

document.body.addEventListener('keydown', function() {
  assertNotReached('keydown event propagation should have stopped');
});
MockInteractions.keyEventOn(testElement, 'keydown', 13, undefined, 'Enter');

[Dan Beam, 2017-03-11 00:06:10]

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

[Dan Beam, 2017-03-11 00:06:11]

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/focusable_iron_list_item_behavior.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:38: if
(event.key == "Enter")
On 2017/03/11 00:04:47, dpapad wrote:
> I think that the convention is to always use single quotes in JS, double
quotes
> in HTML.

fwiw: `git cl format --js`fixes this, btw

[Dan Beam, 2017-03-11 00:06:20]

Description was changed from

==========
MD Settings: fix unnecessary focus on list items.

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

to

==========
MD Settings: fix unnecessary focus on list items.

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

[Dan Beam, 2017-03-11 00:06:20]

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

[scottchen, 2017-03-13 19:02:05]

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/focusable_iron_list_item_behavior.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:38: if
(event.key == "Enter")
On 2017/03/11 00:06:10, Dan Beam wrote:
> On 2017/03/11 00:04:47, dpapad wrote:
> > I think that the convention is to always use single quotes in JS, double
> quotes
> > in HTML.
> 
> fwiw: `git cl format --js`fixes this, btw

I made this patch after running `git cl format --js`, so I don't think that
command changes quotes.
Fix is done.

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:47:
document.body.addEventListener('keydown', function() {
On 2017/03/11 00:04:47, dpapad wrote:
> This listener is leaked (it will be active for any subsequent tests). We
should
> remove it, or probably easier to wrap the element you are testing with a
<div>,
> add the listener on the <div> instead. Then, when clearBody() is called the
> <div> itself will be removed from the DOM, and no listeners will be leaked.

Done.

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:53:
testElement.async(function() {
On 2017/03/11 00:04:47, dpapad wrote:
> Tried this locally. It does not seem that the async() call is needed. Firing
an
> event synchronously fires the listener.
> 
> Also how about simplifying this test as follows
> 
> document.body.addEventListener('keydown', function() {
>   assertNotReached('keydown event propagation should have stopped');
> });
> MockInteractions.keyEventOn(testElement, 'keydown', 13, undefined, 'Enter');

There were some weirdness with assertNotReached throwing a 'testIsDone' warning,
but I have simplified and removed the async.

[dpapad, 2017-03-13 19:30:37]

LGTM with nits.

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:53:
testElement.async(function() {
On 2017/03/13 at 19:02:04, scottchen wrote:
> On 2017/03/11 00:04:47, dpapad wrote:
> > Tried this locally. It does not seem that the async() call is needed. Firing
an
> > event synchronously fires the listener.
> > 
> > Also how about simplifying this test as follows
> > 
> > document.body.addEventListener('keydown', function() {
> >   assertNotReached('keydown event propagation should have stopped');
> > });
> > MockInteractions.keyEventOn(testElement, 'keydown', 13, undefined, 'Enter');
> 
> There were some weirdness with assertNotReached throwing a 'testIsDone'
warning, but I have simplified and removed the async.

FWIW, I investigated a bit the weirdness you mentioned. Findings
 - There is a naming collision for top level assertNotReached(), see
https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=55 and
https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=1717.
 - In some cases (uncaught async error) mocha.js ends up firing the 'end' event
twice, which causes the mocha_adapter.js to call testDone() twice at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/mocha_adapter.js?...,
which causes a warning to show up in the output.

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

https://codereview.chromium.org/2745653003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:32: *
@param {!Event} event the key down event.
s/the/The

or just
@param {!Event} event
I don't think the comment adds value.

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

https://codereview.chromium.org/2745653003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:58: //
Test other keys still works
"Test other keys still work."

[scottchen, 2017-03-14 19:58:10]

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js
(right):

https://codereview.chromium.org/2745653003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:53:
testElement.async(function() {
On 2017/03/13 19:30:36, dpapad wrote:
> On 2017/03/13 at 19:02:04, scottchen wrote:
> > On 2017/03/11 00:04:47, dpapad wrote:
> > > Tried this locally. It does not seem that the async() call is needed.
Firing
> an
> > > event synchronously fires the listener.
> > > 
> > > Also how about simplifying this test as follows
> > > 
> > > document.body.addEventListener('keydown', function() {
> > >   assertNotReached('keydown event propagation should have stopped');
> > > });
> > > MockInteractions.keyEventOn(testElement, 'keydown', 13, undefined,
'Enter');
> > 
> > There were some weirdness with assertNotReached throwing a 'testIsDone'
> warning, but I have simplified and removed the async.
> 
> FWIW, I investigated a bit the weirdness you mentioned. Findings
>  - There is a naming collision for top level assertNotReached(), see
> https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=55 and
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=1717.
>  - In some cases (uncaught async error) mocha.js ends up firing the 'end'
event
> twice, which causes the mocha_adapter.js to call testDone() twice at
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/mocha_adapter.js?...,
> which causes a warning to show up in the output.

The line at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=1717
seems to be exporting "expectNotReached", not "assertNotReached". Did you mean
to paste
https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=1699 ?

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

https://codereview.chromium.org/2745653003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:32: *
@param {!Event} event the key down event.
On 2017/03/13 19:30:37, dpapad wrote:
> s/the/The
> 
> or just
> @param {!Event} event
> I don't think the comment adds value.

Done.

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

https://codereview.chromium.org/2745653003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:58: //
Test other keys still works
On 2017/03/13 19:30:37, dpapad wrote:
> "Test other keys still work."

Done.

[scottchen, 2017-03-14 19:58:15]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-14 19:58:16]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2745653003/#ps80001
(title: "fixes based on comments")

[commit-bot: I haz the power, 2017-03-14 19:59:04]

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-14 22:40:39]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scottchen, 2017-03-14 23:04:15]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:05:22]

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-15 00:44:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489532655627190,
"parent_rev": "0e9d619f88ffabc3c237cbdf75603df8af9925e6", "commit_rev":
"b520415c483d14ee79d8fb82accc63aedd78b755"}

[commit-bot: I haz the power, 2017-03-15 00:45:32]

Description was changed from

==========
MD Settings: fix unnecessary focus on list items.

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

to

==========
MD Settings: fix unnecessary focus on list items.

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

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

[commit-bot: I haz the power, 2017-03-15 00:45:36]

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