MD Settings: Skip hidden nodes when searching.

MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

Nodes that are hidden via a different CSS class, for example <iron-pages> CSS rule
:host > ::content > :not(.iron-selected) { display: none !important; }

are still being searched, and need to be addressed in follow up.

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

Committed: https://crrev.com/2e78a5383cf4651004dace4e7f708eba0c30753f
Cr-Commit-Position: refs/heads/master@{#410915}

[dpapad, 2016-08-09 01:34:58]

Description was changed from

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
"display: none" are not applicable to the user's current state and should be
skipped.

BUG=608535
==========

to

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
"display: none" are not applicable to the user's current state and should be
skipped.

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

[dpapad, 2016-08-09 18:07:21]

Description was changed from

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
"display: none" are not applicable to the user's current state and should be
skipped.

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

to

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

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

[dpapad, 2016-08-09 18:09:13]

Description was changed from

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

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

to

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

Nodes that are hidden via a different CSS class, for example <iron-pages> CSS
rule
:host > ::content > :not(.iron-selected) { display: none !important; }

are still being searched, and need to be addressed in follow up.

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

[dpapad, 2016-08-09 18:09:21]

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

[commit-bot: I haz the power, 2016-08-09 18:09:44]

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-08-09 19:49:34]

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

[commit-bot: I haz the power, 2016-08-09 19:49:35]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-08-09 21:05:22]

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

[Dan Beam, 2016-08-09 22:01:38]

generally lgtm but i really don't want to start doing this ternary thing

i don't really see how

thing && thing.doSomething()

or

value ? a() : b()

are that useful to let in our codebase

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_settings.js:141: node.style.display ==
'none'))) {
nit: maybe something like

if (IGNORED_ELEMENTS.has(node.nodeName))
  return;

if (node instanceof HTMLElement) {
  let element = /** @type {HTMLElement} */(node);
  if (element.hasAttribute(SKIP_SEARCH_CSS_ATTRIBUTE) ||
      element.hasAttribute('hidden') ||
      element.style.display == 'none') {
    return;
  }
}

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_settings.js:347: visible ?
haven't we already talked about these?

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:45:
:host(.hidden-for-search) {
can we maybe make this a property and use reflectToAttribute: true?

[dpapad, 2016-08-09 23:03:02]

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_settings.js:141: node.style.display ==
'none'))) {
On 2016/08/09 at 22:01:38, Dan Beam wrote:
> nit: maybe something like
> 
> if (IGNORED_ELEMENTS.has(node.nodeName))
>   return;
> 
> if (node instanceof HTMLElement) {
>   let element = /** @type {HTMLElement} */(node);
>   if (element.hasAttribute(SKIP_SEARCH_CSS_ATTRIBUTE) ||
>       element.hasAttribute('hidden') ||
>       element.style.display == 'none') {
>     return;
>   }
> }

Done. Except that I am not using 'let'. I have nothing against let, it just
seems odd to sprinkle a single 'let' keyword inside a file that is only using
'var' everywhere else.

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_settings.js:347: visible ?
On 2016/08/09 at 22:01:38, Dan Beam wrote:
> haven't we already talked about these?

Done. classList#toggle() accepts a second parameter, which is just what I need
here, so removed ternary operator.

We have talked about this once. Suggesting to take the time to modify the JS
styleguide to explicitly mention this, if you strongly dislike such usage of the
ternary operator.

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:45:
:host(.hidden-for-search) {
On 2016/08/09 at 22:01:38, Dan Beam wrote:
> can we maybe make this a property and use reflectToAttribute: true?

What would be the benefit? Current approach is native and seems to require less
work.

[Dan Beam, 2016-08-09 23:05:01]

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:45:
:host(.hidden-for-search) {
On 2016/08/09 23:03:02, dpapad wrote:
> On 2016/08/09 at 22:01:38, Dan Beam wrote:
> > can we maybe make this a property and use reflectToAttribute: true?
> 
> What would be the benefit? Current approach is native and seems to require
less
> work.

classes set from external code are overly tightly coupled

[Dan Beam, 2016-08-09 23:05:38]

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_settings.js:347: visible ?
On 2016/08/09 23:03:02, dpapad wrote:
> On 2016/08/09 at 22:01:38, Dan Beam wrote:
> > haven't we already talked about these?
> 
> Done. classList#toggle() accepts a second parameter, which is just what I need
> here, so removed ternary operator.
> 
> We have talked about this once. Suggesting to take the time to modify the JS
> styleguide to explicitly mention this, if you strongly dislike such usage of
the
> ternary operator.

it's already covered by "BE CONSISTENT"

[dpapad, 2016-08-09 23:30:29]

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_section.html
(right):

https://codereview.chromium.org/2223083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_section.html:45:
:host(.hidden-for-search) {
On 2016/08/09 at 23:05:01, Dan Beam wrote:
> On 2016/08/09 23:03:02, dpapad wrote:
> > On 2016/08/09 at 22:01:38, Dan Beam wrote:
> > > can we maybe make this a property and use reflectToAttribute: true?
> > 
> > What would be the benefit? Current approach is native and seems to require
less
> > work.
> 
> classes set from external code are overly tightly coupled

Changed to attribute.

[Dan Beam, 2016-08-09 23:32:45]

still lgtm

[dpapad, 2016-08-10 00:34:47]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-08-10 00:35:24]

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-08-10 01:46:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-10 01:49:33]

Description was changed from

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

Nodes that are hidden via a different CSS class, for example <iron-pages> CSS
rule
:host > ::content > :not(.iron-selected) { display: none !important; }

are still being searched, and need to be addressed in follow up.

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

to

==========
MD Settings: Skip hidden nodes when searching.

Nodes that are hidden, either with the "hidden" attribute, or via an explicit
style "display: none" are not applicable to the user's current state and should
be skipped.

Nodes that are hidden via a different CSS class, for example <iron-pages> CSS
rule
:host > ::content > :not(.iron-selected) { display: none !important; }

are still being searched, and need to be addressed in follow up.

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

Committed: https://crrev.com/2e78a5383cf4651004dace4e7f708eba0c30753f
Cr-Commit-Position: refs/heads/master@{#410915}
==========

[commit-bot: I haz the power, 2016-08-10 01:49:35]

Patchset 3 (id:??) landed as
https://crrev.com/2e78a5383cf4651004dace4e7f708eba0c30753f
Cr-Commit-Position: refs/heads/master@{#410915}