[MD settings] advanced toggle; add scrollWhenReady

[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3bb85be6e84ab31fef20f1d1436dbc70c78091d5
Cr-Commit-Position: refs/heads/master@{#400259}

[dschuyler, 2016-06-10 00:34:07]

Description was changed from

==========
[MD settings] up/down arrow icon in advanced toggle

BUG=
==========

to

==========
[MD settings] up/down arrow icon in advanced toggle

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

[dschuyler, 2016-06-10 00:36:33]

Description was changed from

==========
[MD settings] up/down arrow icon in advanced toggle

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

to

==========
[MD settings] up/down arrow icon in advanced toggle

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

[dschuyler, 2016-06-10 21:26:07]

Patchset #2 (id:20001) has been deleted

[dschuyler, 2016-06-10 21:26:17]

Patchset #2 (id:40001) has been deleted

[dschuyler, 2016-06-10 21:33:58]

Description was changed from

==========
[MD settings] up/down arrow icon in advanced toggle

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

to

==========
[MD settings] up/down arrow icon in advanced toggle

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-06-10 21:33:59]

dschuyler@chromium.org changed reviewers:
+ michaelpg@chromium.org

[dschuyler, 2016-06-10 21:34:13]

Description was changed from

==========
[MD settings] up/down arrow icon in advanced toggle

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-06-10 21:35:54]

I looked at separating these, but most of it
is related to scrolling. The changes for the
advanced toggle are rather minor.

[michaelpg, 2016-06-13 21:11:22]

TL;DR: I don't think <settings-main> should have the same behavior its children
implement.

If we do want to make significant structural changes, the CL title should
reflect that.

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

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.html:46:
<iron-icon icon="[[arrowState_(showAdvancedPage_)]]"></iron-icon>
import:
chrome://resources/cr_elements/icons.html
/icons.html

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

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:12: behaviors:
[RoutableBehavior],
This should not be a RoutableBehavior, which controls expanding and collapsing
settings-sections and scrolling to settings-sections. (It's not the best name.
It includes MainPageBehavior, which is also not a great name: the "main pages"
are basic, advanced, and about, not <settings-main> (which we never call a
"page").)

If you just want common "scrollWhenReady" behavior, you should do that
separately or factor it out of MainPageBehavior. Looks like it can just be
static, btw.

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:28:
this.scroller = (this.parentNode.$ && this.parentNode.$.mainContainer) ||
This is a symptom of using this behavior for heterogeneous elements.
<settings-main> doesn't need this anyway, it owns the scrolling container.

[dschuyler, 2016-06-13 21:44:50]

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

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.html:46:
<iron-icon icon="[[arrowState_(showAdvancedPage_)]]"></iron-icon>
On 2016/06/13 21:11:22, michaelpg wrote:
> import:
> chrome://resources/cr_elements/icons.html
> /icons.html

Done.

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

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:12: behaviors:
[RoutableBehavior],
On 2016/06/13 21:11:22, michaelpg wrote:
> This should not be a RoutableBehavior, which controls expanding and collapsing
> settings-sections and scrolling to settings-sections. (It's not the best name.
> It includes MainPageBehavior, which is also not a great name: the "main pages"
> are basic, advanced, and about, not <settings-main> (which we never call a
> "page").)
> 
> If you just want common "scrollWhenReady" behavior, you should do that
> separately or factor it out of MainPageBehavior. Looks like it can just be
> static, btw.

Done.

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:28:
this.scroller = (this.parentNode.$ && this.parentNode.$.mainContainer) ||
On 2016/06/13 21:11:22, michaelpg wrote:
> This is a symptom of using this behavior for heterogeneous elements.
> <settings-main> doesn't need this anyway, it owns the scrolling container.

Done.

[dschuyler, 2016-06-13 23:50:24]

Description was changed from

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-06-15 23:16:51]

On 2016/06/13 21:44:50, dschuyler wrote:
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_main/settings_main.html
(right):
> 
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_main/settings_main.html:46:
> <iron-icon icon="[[arrowState_(showAdvancedPage_)]]"></iron-icon>
> On 2016/06/13 21:11:22, michaelpg wrote:
> > import:
> > chrome://resources/cr_elements/icons.html
> > /icons.html
> 
> Done.
> 
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_main/settings_main.js (right):
> 
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_main/settings_main.js:12:
behaviors:
> [RoutableBehavior],
> On 2016/06/13 21:11:22, michaelpg wrote:
> > This should not be a RoutableBehavior, which controls expanding and
collapsing
> > settings-sections and scrolling to settings-sections. (It's not the best
name.
> > It includes MainPageBehavior, which is also not a great name: the "main
pages"
> > are basic, advanced, and about, not <settings-main> (which we never call a
> > "page").)
> > 
> > If you just want common "scrollWhenReady" behavior, you should do that
> > separately or factor it out of MainPageBehavior. Looks like it can just be
> > static, btw.
> 
> Done.
> 
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> (right):
> 
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_page/main_page_behavior.js:28:
> this.scroller = (this.parentNode.$ && this.parentNode.$.mainContainer) ||
> On 2016/06/13 21:11:22, michaelpg wrote:
> > This is a symptom of using this behavior for heterogeneous elements.
> > <settings-main> doesn't need this anyway, it owns the scrolling container.
> 
> Done.

How is this looking? Could it land while further work is done on the
animations?

[michaelpg, 2016-06-16 03:45:30]

On 2016/06/15 23:16:51, dschuyler wrote:
> On 2016/06/13 21:44:50, dschuyler wrote:
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/settings_main/settings_main.html
> (right):
> > 
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/settings_main/settings_main.html:46:
> > <iron-icon icon="[[arrowState_(showAdvancedPage_)]]"></iron-icon>
> > On 2016/06/13 21:11:22, michaelpg wrote:
> > > import:
> > > chrome://resources/cr_elements/icons.html
> > > /icons.html
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/settings_main/settings_main.js
(right):
> > 
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/settings_main/settings_main.js:12:
> behaviors:
> > [RoutableBehavior],
> > On 2016/06/13 21:11:22, michaelpg wrote:
> > > This should not be a RoutableBehavior, which controls expanding and
> collapsing
> > > settings-sections and scrolling to settings-sections. (It's not the best
> name.
> > > It includes MainPageBehavior, which is also not a great name: the "main
> pages"
> > > are basic, advanced, and about, not <settings-main> (which we never call a
> > > "page").)
> > > 
> > > If you just want common "scrollWhenReady" behavior, you should do that
> > > separately or factor it out of MainPageBehavior. Looks like it can just be
> > > static, btw.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2054013002/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/settings_page/main_page_behavior.js:28:
> > this.scroller = (this.parentNode.$ && this.parentNode.$.mainContainer) ||
> > On 2016/06/13 21:11:22, michaelpg wrote:
> > > This is a symptom of using this behavior for heterogeneous elements.
> > > <settings-main> doesn't need this anyway, it owns the scrolling container.
> > 
> > Done.
> 
> How is this looking? Could it land while further work is done on the
> animations?

yes please :-)

[michaelpg, 2016-06-16 03:45:59]

lgtm! (w/ nits)

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:42: <div
id="toggleSpacer">
nit: </div> on same line

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

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:69: return
this.shadowRoot.querySelector('settings-advanced-page');
this.$$('...')

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:72: return
this.shadowRoot.querySelector('#toggleContainer');
this.$$('...')

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:41: /**
@type {?Element} The scrolling container. Elements must set this. */
remove "Element must set this."

[dschuyler, 2016-06-16 20:14:21]

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.html:42: <div
id="toggleSpacer">
On 2016/06/16 03:45:59, michaelpg wrote:
> nit: </div> on same line

Done.

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

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:69: return
this.shadowRoot.querySelector('settings-advanced-page');
On 2016/06/16 03:45:59, michaelpg wrote:
> this.$$('...')

Done.

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:72: return
this.shadowRoot.querySelector('#toggleContainer');
On 2016/06/16 03:45:59, michaelpg wrote:
> this.$$('...')

Done.

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2054013002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:41: /**
@type {?Element} The scrolling container. Elements must set this. */
On 2016/06/16 03:45:59, michaelpg wrote:
> remove "Element must set this."

Done.

[dschuyler, 2016-06-16 20:25:37]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-06-16 20:25:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2054013002/#ps120001
(title: "review nits")

[commit-bot: I haz the power, 2016-06-16 20:26:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054013002/120001

[commit-bot: I haz the power, 2016-06-16 21:29:35]

Description was changed from

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-16 21:29:36]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-06-16 21:31:02]

Description was changed from

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the 
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] advanced toggle; add scrollWhenReady

This CL makes change in two subjects. The first is the
advanced page toggle shown in the main content area of
the settings page. The second is adding the
scrollWhenReady handler to the main settings behavior
that handles scrolling an element into view once the
page is ready for that to happen.

BUG=593989, 614566, 618755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3bb85be6e84ab31fef20f1d1436dbc70c78091d5
Cr-Commit-Position: refs/heads/master@{#400259}
==========

[commit-bot: I haz the power, 2016-06-16 21:31:03]

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