MD Settings: adjust focus-outline behaviors on search engine iron-list.

MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.

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

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

[scottchen, 2017-02-16 21:55:46]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list

BUG=684152
==========

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list

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

[scottchen, 2017-02-16 21:57:45]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list

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

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list

Goal: When user clicks on an item, there should be no focus outline.

Caveat #1: We can't simply just add .no-outline to everything the mouse
           clicks - in case the user clicks on something that was
           focused by keyboard, the click would appear to unexpectedly
           unfocus the item.
Caveat #2: we can't just do "document.activeElement == this" (as we did
           for action-links), because document.activeElement only track
           interactive elements (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
     When user uses keyboard to traverse through the iron-list, key-down is
     fired on the previously focused item, then key-up is fired on the newly
     focused item. We use this behavior here to determine if an item is
     already in focus via keyboard. If it were, we don't add .no-outline.
==========

[scottchen, 2017-02-16 21:57:46]

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

[scottchen, 2017-02-16 22:04:52]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list

Goal: When user clicks on an item, there should be no focus outline.

Caveat #1: We can't simply just add .no-outline to everything the mouse
           clicks - in case the user clicks on something that was
           focused by keyboard, the click would appear to unexpectedly
           unfocus the item.
Caveat #2: we can't just do "document.activeElement == this" (as we did
           for action-links), because document.activeElement only track
           interactive elements (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
     When user uses keyboard to traverse through the iron-list, key-down is
     fired on the previously focused item, then key-up is fired on the newly
     focused item. We use this behavior here to determine if an item is
     already in focus via keyboard. If it were, we don't add .no-outline.
==========

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.
==========

[scottchen, 2017-02-16 22:06:16]

Feel free to start reviewing and leaving comments, but I'm going to try to find
a way to extract this into a behavior that's re-usable for any row items inside
the iron-list.

[hcarmona, 2017-02-16 22:33:06]

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45:
this.addEventListener('keyup', function(e) {
can we use listeners instead?

listeners: {
  'keyup': 'onKeyUp_',
  'keydown': 'onKeyDown_',
  'mousedown': 'onMouseDown_',
  'blur': 'onBlur_',
},

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:59:
if(!this.focusedByKey_)
What are you trying to prevent here? Is this to make sure the focus ring doesn't
go away if the user tabbed here then clicked?

[Dan Beam, 2017-02-16 22:52:48]

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45:
this.addEventListener('keyup', function(e) {
On 2017/02/16 22:33:06, hcarmona wrote:
> can we use listeners instead?
> 
> listeners: {
>   'keyup': 'onKeyUp_',
>   'keydown': 'onKeyDown_',
>   'mousedown': 'onMouseDown_',
>   'blur': 'onBlur_',
> },

+1

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:59:
if(!this.focusedByKey_)
On 2017/02/16 22:33:06, hcarmona wrote:
> What are you trying to prevent here? Is this to make sure the focus ring
doesn't
> go away if the user tabbed here then clicked?

see: action_link.js

[scottchen, 2017-02-16 22:53:28]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.
==========

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-02-16 23:01:54]

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45:
this.addEventListener('keyup', function(e) {
On 2017/02/16 22:52:48, Dan Beam wrote:
> On 2017/02/16 22:33:06, hcarmona wrote:
> > can we use listeners instead?
> > 
> > listeners: {
> >   'keyup': 'onKeyUp_',
> >   'keydown': 'onKeyDown_',
> >   'mousedown': 'onMouseDown_',
> >   'blur': 'onBlur_',
> > },
> 
> +1

Acknowledged, will try that on the new behavior I just uploaded with the new
patch.

https://codereview.chromium.org/2700863002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:59:
if(!this.focusedByKey_)
On 2017/02/16 22:52:48, Dan Beam wrote:
> On 2017/02/16 22:33:06, hcarmona wrote:
> > What are you trying to prevent here? Is this to make sure the focus ring
> doesn't
> > go away if the user tabbed here then clicked?
> 
> see: action_link.js

Yeah hcarmona@ it's to prevent exactly that.
dbeam@ I have a brief explanation in the CL description why we can't do it the
same way as action-link.js; I looked at it and investigated for a while already
(see caveat #2).

As a general note, is there a better place to put additional explanation of the
CL's code, besides the CL description (is it better to send it with the first
publish email?) I'm wondering because I've experienced a couple times where the
notes in the CL description seems to be not seen by the reviewers.

[scottchen, 2017-02-16 23:23:58]

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

[commit-bot: I haz the power, 2017-02-16 23:25:58]

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-02-17 01:38:01]

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

[commit-bot: I haz the power, 2017-02-17 01:38:02]

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

[hcarmona, 2017-02-17 23:35:37]

Looks good, is it possible to add some tests for the behavior to ensure that our
expected use cases are covered?

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:7:
<link rel="import" href="../focusable_iron_list_item_behavior.html">
You also need to add this dependency to compiled_resources2.gyp to fix the
closure compile error.

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:38:

nit: remove the \n

[scottchen, 2017-02-22 07:07:06]

I added some tests - however, I wasn't able to test the exact scenarios of when
the focus ring show/doesn't show up; After trying a bunch of experiments, it
seems that you cannot trigger the visual focus outline with javascript on things
that by default aren't focusable, even with tabindex=0. 
Given that, the best possible tests I think is to make sure the no-outline class
is applied and removed correctly, based on the events that are being listened
to.

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:7:
<link rel="import" href="../focusable_iron_list_item_behavior.html">
On 2017/02/17 23:35:37, hcarmona wrote:
> You also need to add this dependency to compiled_resources2.gyp to fix the
> closure compile error.

Done.

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:38:

On 2017/02/17 23:35:37, hcarmona wrote:
> nit: remove the \n

Done.

[scottchen, 2017-02-22 07:07:17]

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

[commit-bot: I haz the power, 2017-02-22 07:08:12]

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-02-22 08:57:50]

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

[commit-bot: I haz the power, 2017-02-22 08:57:51]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-22 16:42:30]

On 2017/02/22 07:07:06, scottchen wrote:
> I added some tests - however, I wasn't able to test the exact scenarios of
when
> the focus ring show/doesn't show up; After trying a bunch of experiments, it
> seems that you cannot trigger the visual focus outline with javascript on
things
> that by default aren't focusable, even with tabindex=0. 
> Given that, the best possible tests I think is to make sure the no-outline
class
> is applied and removed correctly, based on the events that are being listened
> to.

can you explain what you're talking about here?  you want to simulate a focus
style but without setting focus?

[scottchen, 2017-02-22 18:20:41]

On 2017/02/22 16:42:30, Dan Beam wrote:
> On 2017/02/22 07:07:06, scottchen wrote:
> > I added some tests - however, I wasn't able to test the exact scenarios of
> when
> > the focus ring show/doesn't show up; After trying a bunch of experiments, it
> > seems that you cannot trigger the visual focus outline with javascript on
> things
> > that by default aren't focusable, even with tabindex=0. 
> > Given that, the best possible tests I think is to make sure the no-outline
> class
> > is applied and removed correctly, based on the events that are being
listened
> > to.
> 
> can you explain what you're talking about here?  you want to simulate a focus
> style but without setting focus?

I wanted to explain why the unit tests I added only test the ".no-outline" class
and not the actual focus states - 
In the unit test, I understand that ideally we want to test exactly when the
focus outline will show up/wont show up, since that's the most direct thing the
users will see. 
I wanted to do that by testing either window.getComputedStyle(ele)['outline'],
or ele.getAttribute('focused'), but it seems that it is impossible to simulate
those results from javascript on a element (like paper-icon-button or div) that
is not focusable by default (i.e. not an input or anchor etc). So the best tests
I can think of are only around the .no-outline class.

[scottchen, 2017-02-22 22:58:31]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.

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

[Dan Beam, 2017-02-22 23:25:46]

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

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:29:
if(!this.$$('[focused]'))
please stop doing this

  if(
    ^
and do this instead.

  if (!this.$$('[focused]'))
    ^

please read the style guide[s]:
https://google.github.io/styleguide/jsguide.html
https://google.github.io/styleguide/javascriptguide.xml

our code also never does this. git cl format --js should also help.

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:46:
};
no ;

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:1332:
'FocusableIronListItemBehavior', function() {
can you change this to add new information or be less long?

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

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:17:
<iron-list items=[[items]]>
why do you actually need to test inside of an iron-list?  the code does not seem
to depend on these details.  can you just simulated [focused]?

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:47:
return ['apple', 'bannana', 'cucumber', 'doughnut'];
in the past we've had issues with iron-lists in tests with >3 items

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:67:
ironList.fire('iron-resize'); // Bug with iron-list.
notifyResize()

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:70:
ironListItems = ironList.querySelectorAll('test-element-item');
i would not recommend accessing the physical items of an iron-list

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:75:
var item_1 = ironListItems[0];
don't use _ in js var names (jsVarsLikeThis)

[scottchen, 2017-02-23 00:31:50]

I changed the test to not be coupled with iron-list and tests the focus states
more directly now.

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

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:29:
if(!this.$$('[focused]'))
On 2017/02/22 23:25:46, Dan Beam wrote:
> please stop doing this
> 
>   if(
>     ^
> and do this instead.
> 
>   if (!this.$$('[focused]'))
>     ^
> 
> please read the style guide[s]:
> https://google.github.io/styleguide/jsguide.html
> https://google.github.io/styleguide/javascriptguide.xml
> 
> our code also never does this. git cl format --js should also help.

Done.

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
(right):

https://codereview.chromium.org/2700863002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:46:
};
On 2017/02/22 23:25:46, Dan Beam wrote:
> no ;

Done.

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:1332:
'FocusableIronListItemBehavior', function() {
On 2017/02/22 23:25:46, Dan Beam wrote:
> can you change this to add new information or be less long?

Done.

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

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:17:
<iron-list items=[[items]]>
On 2017/02/22 23:25:46, Dan Beam wrote:
> why do you actually need to test inside of an iron-list?  the code does not
seem
> to depend on these details.  can you just simulated [focused]?

Done.

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:47:
return ['apple', 'bannana', 'cucumber', 'doughnut'];
On 2017/02/22 23:25:46, Dan Beam wrote:
> in the past we've had issues with iron-lists in tests with >3 items

Changing test to not depend on iron-list, this code will be removed.

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:67:
ironList.fire('iron-resize'); // Bug with iron-list.
On 2017/02/22 23:25:46, Dan Beam wrote:
> notifyResize()

Changing test to not depend on iron-list, this code will be removed.

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:70:
ironListItems = ironList.querySelectorAll('test-element-item');
On 2017/02/22 23:25:46, Dan Beam wrote:
> i would not recommend accessing the physical items of an iron-list

Changing test to not depend on iron-list, this code will be removed.

https://codereview.chromium.org/2700863002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/focusable_iron_list_item_behavior_test.js:75:
var item_1 = ironListItems[0];
On 2017/02/22 23:25:46, Dan Beam wrote:
> don't use _ in js var names (jsVarsLikeThis)

Acknowledged.

[hcarmona, 2017-02-23 01:02:41]

LGTM w/ some nits

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

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:9:
focusedByKey_: {type: Boolean, value: false},
Please change to

focusedByKey_: Boolean,

false is the default value for a boolean

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:30:
'engine.canBeEdited,' +
nit: old indent was more readable

[scottchen, 2017-02-23 01:19:13]

Thanks hcarmona! This one was a bit tricky so I'll also wait for dbeam's LG.

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

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/focusable_iron_list_item_behavior.js:9:
focusedByKey_: {type: Boolean, value: false},
On 2017/02/23 01:02:41, hcarmona wrote:
> Please change to
> 
> focusedByKey_: Boolean,
> 
> false is the default value for a boolean

Done.

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2700863002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:30:
'engine.canBeEdited,' +
On 2017/02/23 01:02:41, hcarmona wrote:
> nit: old indent was more readable

This is done by `git cl format --js`, and I think dbeam's rule of thumb is to
just leave however the formatter did it.

[Dan Beam, 2017-02-23 06:29:54]

lgtm but i do wish there was a way to share .no-outline more easily.  maybe just
use .style directly?

[scottchen, 2017-02-23 18:55:49]

On 2017/02/23 06:29:54, Dan Beam wrote:
> lgtm but i do wish there was a way to share .no-outline more easily.  maybe
just
> use .style directly?

Thanks, will land it now. 
Regarding your last comment, did you mean HTMLElement.style via javascript? I
could look into that.

[scottchen, 2017-02-23 18:55:58]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-02-23 18:55:58]

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

[commit-bot: I haz the power, 2017-02-23 18:56:23]

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

[Dan Beam, 2017-02-23 18:59:47]

On 2017/02/23 18:55:49, scottchen wrote:
> On 2017/02/23 06:29:54, Dan Beam wrote:
> > lgtm but i do wish there was a way to share .no-outline more easily.  maybe
> just
> > use .style directly?
> 
> Thanks, will land it now. 
> Regarding your last comment, did you mean HTMLElement.style via javascript? I
> could look into that.

yes

[commit-bot: I haz the power, 2017-02-23 20:13:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487876158100240,
"parent_rev": "48a2140e0358fadd7a80621d93903ecd5b67d0f3", "commit_rev":
"f3f17d64c61c095f5d7542bb74e2e7137f339b14"}

[commit-bot: I haz the power, 2017-02-23 20:14:20]

Description was changed from

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.

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

to

==========
MD Settings: adjust focus-outline behaviors on search engine iron-list.

Goal: 
Based on offline discussion with dbeam@, the result should be:
  - mouse clicking anywhere should behave as user were clicking on dead-space.
  - pressing tab after clicking on an item would go into it's child icon-button 
    (because on web pages when you click somewhere and clicks tab it would go 
    to the "next" focusable thing, next as in right-and-down in pixels)
  - clicking on an item would not show focus-line, but when you press up and 
    down it'll then show the focus-line on the new items you're focusing.
  - clicking on a row thats focused by keyboard should not unfocus the row.

Caveat #1: 
  We can't simply just add .no-outline to everything the mouse clicks - in case 
  the user clicks on something that was focused by keyboard, the click would 
  appear to unexpectedly unfocus the item.

Caveat #2: 
  We can't just do "document.activeElement == this" (as we did for 
  action-links), because document.activeElement only track interactive elements 
  (inputs, buttons etc.) and not divs etc.

Explaining the solution in the CL:
  When user uses keyboard to traverse through the iron-list, key-down is fired 
  on the previously focused item, then key-up is fired on the newly focused 
  item. We use this behavior here to determine if an item is already in focus 
  via keyboard. If it were, we don't add .no-outline.

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

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

[commit-bot: I haz the power, 2017-02-23 20:14:21]

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