MD Settings Fix associated-control for bluetooth, people, device subpage

MD Settings Fix associated-control for bluetooth, people, device subpage

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

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

[stevenjb, 2017-02-28 23:33:54]

Description was changed from

==========
MD Settings Fix associated-control for bluetooth, people, device subpage

BUG=695195
==========

to

==========
MD Settings Fix associated-control for bluetooth, people, device subpage

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

[stevenjb, 2017-02-28 23:34:55]

stevenjb@chromium.org changed reviewers:
+ dpapad@chromium.org, jdufault@chromium.org, michaelpg@chromium.org

[jdufault, 2017-02-28 23:41:00]

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/device_page.html:105: <template
is="dom-if" route-path="/stylus">
Does navigating directly to the stylus subpage work correctly? Note that
hasStylus_ can go from false->true if the page load is happening from a Chrome
crash recovery.

[dpapad, 2017-02-28 23:56:12]

Can you verify that searching for 'a' does not result in any errors in the
console?

Searching for 'a' basically finds a "hit" in every single settings-subpage, and
therefore if any associated-controls are missing, it will result in an error.

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:330: <template
is="dom-if" route-path="/lockScreen" no-search>
Unless you remove no-search here, the associated-control you are adding will
have no effect (the search algorithm will always skip this subtree).

[stevenjb, 2017-03-01 00:03:16]

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/device_page.html:105: <template
is="dom-if" route-path="/stylus">
On 2017/02/28 23:41:00, jdufault wrote:
> Does navigating directly to the stylus subpage work correctly? Note that
> hasStylus_ can go from false->true if the page load is happening from a Chrome
> crash recovery.

Yes, the stylus page is fine. The problem is that checkPointerSubpage() will
navigate away from the subpage if hasMouse and hasTouchpad are false.

Arguably we may wish to modify that logic to also check hasStylus, in which case
we will need to set hasStylus ot be ininitialized also and just hide the stylus
section instead of using a dom-if, since bad things happen when dom-if
properties are undefined :(

[stevenjb, 2017-03-01 00:10:33]

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/device_page.html:105: <template
is="dom-if" route-path="/stylus">
On 2017/03/01 00:03:16, stevenjb wrote:
> On 2017/02/28 23:41:00, jdufault wrote:
> > Does navigating directly to the stylus subpage work correctly? Note that
> > hasStylus_ can go from false->true if the page load is happening from a
Chrome
> > crash recovery.
> 
> Yes, the stylus page is fine. The problem is that checkPointerSubpage() will
> navigate away from the subpage if hasMouse and hasTouchpad are false.
> 
> Arguably we may wish to modify that logic to also check hasStylus, in which
case
> we will need to set hasStylus ot be ininitialized also and just hide the
stylus
> section instead of using a dom-if, since bad things happen when dom-if
> properties are undefined :(

I just realized I answered the wrong question :)

No, this does not work correctly. Investigating.

[stevenjb, 2017-03-01 00:10:50]

On 2017/02/28 23:56:12, dpapad wrote:
> Can you verify that searching for 'a' does not result in any errors in the
> console?
> 
> Searching for 'a' basically finds a "hit" in every single settings-subpage,
and
> therefore if any associated-controls are missing, it will result in an error.
> 
>
https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/people_page/people_page.html (right):
> 
>
https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/people_page/people_page.html:330: <template
> is="dom-if" route-path="/lockScreen" no-search>
> Unless you remove no-search here, the associated-control you are adding will
> have no effect (the search algorithm will always skip this subtree).

I tested with 'a' through 'z' :)

[stevenjb, 2017-03-01 00:20:31]

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/people_page.html:330: <template
is="dom-if" route-path="/lockScreen" no-search>
On 2017/02/28 23:56:12, dpapad wrote:
> Unless you remove no-search here, the associated-control you are adding will
> have no effect (the search algorithm will always skip this subtree).

Ah, I didn't see that. No reason I can think of for this to be no-search.
Removed.

[stevenjb, 2017-03-01 00:31:37]

PTAL

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/device_page.html:105: <template
is="dom-if" route-path="/stylus">
On 2017/03/01 00:10:33, stevenjb wrote:
> On 2017/03/01 00:03:16, stevenjb wrote:
> > On 2017/02/28 23:41:00, jdufault wrote:
> > > Does navigating directly to the stylus subpage work correctly? Note that
> > > hasStylus_ can go from false->true if the page load is happening from a
> Chrome
> > > crash recovery.
> > 
> > Yes, the stylus page is fine. The problem is that checkPointerSubpage() will
> > navigate away from the subpage if hasMouse and hasTouchpad are false.
> > 
> > Arguably we may wish to modify that logic to also check hasStylus, in which
> case
> > we will need to set hasStylus ot be ininitialized also and just hide the
> stylus
> > section instead of using a dom-if, since bad things happen when dom-if
> > properties are undefined :(
> 
> I just realized I answered the wrong question :)
> 
> No, this does not work correctly. Investigating.

OK, I looked at this some more and understand why this is a problem. Basically
if we want to search the stylus page (which I would say we do, it has terms like
'Note-taking app'), and be able to navigate to the page directly, we need to
hide the section instead of dom-if it. I think that is reasonable here.

There is one quirk with searching hidden subpages - the search 'succeeds', but
nothing is highlighted. I think we can probably live with that? This is not
going to be the only such example I think.

[dpapad, 2017-03-01 00:34:47]

On 2017/03/01 at 00:31:37, stevenjb wrote:
> PTAL
> 
>
https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/device_page/device_page.html (right):
> 
>
https://codereview.chromium.org/2715403005/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/device_page/device_page.html:105: <template
is="dom-if" route-path="/stylus">
> On 2017/03/01 00:10:33, stevenjb wrote:
> > On 2017/03/01 00:03:16, stevenjb wrote:
> > > On 2017/02/28 23:41:00, jdufault wrote:
> > > > Does navigating directly to the stylus subpage work correctly? Note that
> > > > hasStylus_ can go from false->true if the page load is happening from a
> > Chrome
> > > > crash recovery.
> > > 
> > > Yes, the stylus page is fine. The problem is that checkPointerSubpage()
will
> > > navigate away from the subpage if hasMouse and hasTouchpad are false.
> > > 
> > > Arguably we may wish to modify that logic to also check hasStylus, in
which
> > case
> > > we will need to set hasStylus ot be ininitialized also and just hide the
> > stylus
> > > section instead of using a dom-if, since bad things happen when dom-if
> > > properties are undefined :(
> > 
> > I just realized I answered the wrong question :)
> > 
> > No, this does not work correctly. Investigating.
> 
> OK, I looked at this some more and understand why this is a problem. Basically
if we want to search the stylus page (which I would say we do, it has terms like
'Note-taking app'), and be able to navigate to the page directly, we need to
hide the section instead of dom-if it. I think that is reasonable here.
> 
> There is one quirk with searching hidden subpages - the search 'succeeds', but
nothing is highlighted. I think we can probably live with that? This is not
going to be the only such example I think.

Steven, anything that is hidden is supposed to be excluded from search hits. The
relevant code is at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search....
Is your hidden element hidden in a way that is not caught by that condition?

[stevenjb, 2017-03-01 00:55:52]

Ah. It looks like it is the subpage itself that is triggering the result.

We need to set the settings-stylus subpage to
'no-search="[[!hasStylus_]]"'. Kind of a pain, but I can't think of any
other examples where we have a conditional subpage like this.


On Tue, Feb 28, 2017 at 5:34 PM, dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2017/03/01 at 00:31:37, stevenjb wrote:
> > PTAL
> >
> >
> https://codereview.chromium.org/2715403005/diff/1/chrome/bro
> wser/resources/settings/device_page/device_page.html
> > File chrome/browser/resources/settings/device_page/device_page.html
> (right):
> >
> >
> https://codereview.chromium.org/2715403005/diff/1/chrome/bro
> wser/resources/settings/device_page/device_page.html#newcode105
> > chrome/browser/resources/settings/device_page/device_page.html:105:
> <template
> is="dom-if" route-path="/stylus">
> > On 2017/03/01 00:10:33, stevenjb wrote:
> > > On 2017/03/01 00:03:16, stevenjb wrote:
> > > > On 2017/02/28 23:41:00, jdufault wrote:
> > > > > Does navigating directly to the stylus subpage work correctly?
> Note that
> > > > > hasStylus_ can go from false->true if the page load is happening
> from a
> > > Chrome
> > > > > crash recovery.
> > > >
> > > > Yes, the stylus page is fine. The problem is that
> checkPointerSubpage()
> will
> > > > navigate away from the subpage if hasMouse and hasTouchpad are false.
> > > >
> > > > Arguably we may wish to modify that logic to also check hasStylus, in
> which
> > > case
> > > > we will need to set hasStylus ot be ininitialized also and just hide
> the
> > > stylus
> > > > section instead of using a dom-if, since bad things happen when
> dom-if
> > > > properties are undefined :(
> > >
> > > I just realized I answered the wrong question :)
> > >
> > > No, this does not work correctly. Investigating.
> >
> > OK, I looked at this some more and understand why this is a problem.
> Basically
> if we want to search the stylus page (which I would say we do, it has
> terms like
> 'Note-taking app'), and be able to navigate to the page directly, we need
> to
> hide the section instead of dom-if it. I think that is reasonable here.
> >
> > There is one quirk with searching hidden subpages - the search
> 'succeeds', but
> nothing is highlighted. I think we can probably live with that? This is not
> going to be the only such example I think.
>
> Steven, anything that is hidden is supposed to be excluded from search
> hits. The
> relevant code is at
> https://cs.chromium.org/chromium/src/chrome/browser/resource
> s/settings/search_settings.js?q=search_settings.js&sq=
> package:chromium&dr&l=138-140.
> Is your hidden element hidden in a way that is not caught by that
> condition?
>
> https://codereview.chromium.org/2715403005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2017-03-01 00:56:17]

ptal

[stevenjb, 2017-03-01 01:13:56]

Since the lock screen (quick unlock) subpages show modal dialogs
unconditionally, rendering them for search can be problematic. Making them both
no-search for now.

PTAL

[dpapad, 2017-03-01 02:08:05]

LGTM with question.

https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:39: <div
id="stylusRow" class="settings-box" on-tap="onStylusTap_"
Is this change from dom-if to hidden necessary?

[dpapad, 2017-03-01 02:09:31]

On 2017/03/01 at 02:08:05, dpapad wrote:
> LGTM with question.
> 
>
https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/device_page/device_page.html (right):
> 
>
https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/device_page/device_page.html:39: <div
id="stylusRow" class="settings-box" on-tap="onStylusTap_"
> Is this change from dom-if to hidden necessary?

Also, is this CL also addressing crbug.com/696422?

[stevenjb, 2017-03-01 16:43:25]

On 2017/03/01 02:09:31, dpapad wrote:
> On 2017/03/01 at 02:08:05, dpapad wrote:
> > LGTM with question.
> > 
> >
>
https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/device_page/device_page.html (right):
> > 
> >
>
https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/device_page/device_page.html:39: <div
> id="stylusRow" class="settings-box" on-tap="onStylusTap_"
> > Is this change from dom-if to hidden necessary?
> 
> Also, is this CL also addressing crbug.com/696422?

Yes it will. Marking crbug.com/69642 as a dupe.

[stevenjb, 2017-03-01 16:49:57]

https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2715403005/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:39: <div
id="stylusRow" class="settings-box" on-tap="onStylusTap_"
On 2017/03/01 02:08:05, dpapad wrote:
> Is this change from dom-if to hidden necessary?

Actually, no. I thought it was before I thought to do
no-search$="[[!hasStylus_]], but with that change this is unnecessary. Reverted.

[michaelpg, 2017-03-02 00:00:21]

lgtm too

[stevenjb, 2017-03-02 00:10:06]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-02 00:10:07]

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/2715403005/#ps80001
(title: "Restore dom-if")

[commit-bot: I haz the power, 2017-03-02 00:10:55]

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-02 01:47:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488413406731220,
"parent_rev": "49957feee8c5ea46bc551e669e370164b6852702", "commit_rev":
"f2de54cd096549323fd45c469f62831c5fe90f6f"}

[commit-bot: I haz the power, 2017-03-02 01:47:39]

Description was changed from

==========
MD Settings Fix associated-control for bluetooth, people, device subpage

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

to

==========
MD Settings Fix associated-control for bluetooth, people, device subpage

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

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

[commit-bot: I haz the power, 2017-03-02 01:47:41]

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