[MD settings] end of page padding in content area

[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/68a872b1209e48315f408d5d1693d2636df299b1
Cr-Commit-Position: refs/heads/master@{#407581}

[dschuyler, 2016-06-22 21:23:40]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
==========

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

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

[dschuyler, 2016-06-22 21:25:44]

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

[dschuyler, 2016-06-22 21:25:44]

The need for that height calculation was
rather narrow, so I made it a local member
rather than a member on Element.prototype.

[Dan Beam, 2016-06-23 02:48:21]

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

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:59: <div
id='endOfPageSpace'></div>
' => "

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:59: <div
id='endOfPageSpace'></div>
nit: id="overscroll"

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

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:66:
this.currentRoute = {
why is this necessary?

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:136:
lastSettingsElementHeight_: function() {
can calcHeight_ just live inside here instead as a pure function, i.e.

function calcHeight() {
}

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:139:
this.showAboutPage_)
all these conditionals are more than 1 line in the conditional or the body, so
they should all have curlies

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:144: if
(this.$$('#toggleContainer'))
don't query this twice, i.e.

var toggleContainer = this.$$('#toggleContainer');
if (toggleContainer)
  return calcHeight(toggleContainer);

[dschuyler, 2016-06-23 18:22:01]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

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

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu. Also includes a fix to set the route properly
when going to basic or advanced pages (without going to a section within
the page).

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

[dschuyler, 2016-06-23 18:33:10]

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

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:59: <div
id='endOfPageSpace'></div>
On 2016/06/23 02:48:21, Dan Beam wrote:
> ' => "

Done.

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:59: <div
id='endOfPageSpace'></div>
On 2016/06/23 02:48:21, Dan Beam wrote:
> nit: id="overscroll"

Done.

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

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:66:
this.currentRoute = {
On 2016/06/23 02:48:21, Dan Beam wrote:
> why is this necessary?

The route was not being correctly set.
This could be moved to a separate CL.
I'll update the CL notes to mention that
this is here.

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:136:
lastSettingsElementHeight_: function() {
On 2016/06/23 02:48:21, Dan Beam wrote:
> can calcHeight_ just live inside here instead as a pure function, i.e.
> 
> function calcHeight() {
> }

Done.

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:139:
this.showAboutPage_)
On 2016/06/23 02:48:21, Dan Beam wrote:
> all these conditionals are more than 1 line in the conditional or the body, so
> they should all have curlies

Done.

https://codereview.chromium.org/2090753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.js:144: if
(this.$$('#toggleContainer'))
On 2016/06/23 02:48:21, Dan Beam wrote:
> don't query this twice, i.e.
> 
> var toggleContainer = this.$$('#toggleContainer');
> if (toggleContainer)
>   return calcHeight(toggleContainer);

Done.

[Dan Beam, 2016-06-30 06:45:54]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:70: url: '',
can you just omit URL?

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:135:
Polymer.dom.flush();
why is this required?

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:137:
this.showAboutPage_) {
can this be done earlier?

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:143:
'[section=reset]'));
why the reset section?

[michaelpg, 2016-06-30 17:13:21]

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

[michaelpg, 2016-06-30 17:13:22]

Who decided we should do this? It's not how the web works. It would be nice if
we had another way of highlighting the current section rather than effectively
hiding the rest of the page (eg for the last section).

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:66:
this.currentRoute = {
can this be submitted separately?

[michaelpg, 2016-06-30 18:17:53]

On 2016/06/30 17:13:22, michaelpg wrote:
> Who decided we should do this? It's not how the web works. It would be nice if
> we had another way of highlighting the current section rather than effectively
> hiding the rest of the page (eg for the last section).

Dave pointed me to this doc which asks for this behavior as a V2 improvement:

https://docs.google.com/document/d/1rQ5lc9-cjb8ZCue4AZfMW6QSiJhs6ygZes9MuwU9g...

so I don't wanna question that decision.

> 
>
https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_main/settings_main.js (right):
> 
>
https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_main/settings_main.js:66:
> this.currentRoute = {
> can this be submitted separately?

^ would still be nice in case something needs reverting

[dschuyler, 2016-06-30 23:52:18]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:70: url: '',
On 2016/06/30 06:45:54, Dan Beam wrote:
> can you just omit URL?

We could change the SettingsRoute typedef to not require
the url. My hope is that requiring the URL
will help others specify routes correctly in
settings_router.js. Setting the url here seems
like a small price to have help getting the 
routes correct. WDYT

[Dan Beam, 2016-06-30 23:59:15]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:70: url: '',
On 2016/06/30 23:52:18, dschuyler wrote:
> On 2016/06/30 06:45:54, Dan Beam wrote:
> > can you just omit URL?
> 
> We could change the SettingsRoute typedef to not require
> the url.

how are all the other assignments to currentRoute compiling?
https://cs.chromium.org/search/?q=%22currentroute+%3D%22&sq=package:chromium&...

> My hope is that requiring the URL
> will help others specify routes correctly in
> settings_router.js. Setting the url here seems
> like a small price to have help getting the 
> routes correct. WDYT

forcing folks to copy 1 more key that's partially used doesn't seem like an
improvement.

1) the URL is set based on this code, which doesn't care about the "url" key:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

2) the URLs you're specifying are wrong, as far as I can tell:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

so how does using url "help getting the routes correct"?

if you really want to make this better, we should make named routes, i.e.

Routes = {
  BASIC: {
    url: '/',
    ...
  },
  ADVANCED: {
    url: '/advanced',
    ...
  },
},

routes_: [Routes.BASIC, Routes.ADVANCED, ...],

and then from somewhere that needs to navigate:

this.currentRoute = Routes.BASIC;

[dschuyler, 2016-07-01 19:21:22]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:66:
this.currentRoute = {
On 2016/06/30 17:13:22, michaelpg wrote:
> can this be submitted separately?

Done.

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:70: url: '',
On 2016/06/30 06:45:54, Dan Beam wrote:
> can you just omit URL?

Done.

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:70: url: '',
On 2016/06/30 23:59:15, Dan Beam wrote:
> On 2016/06/30 23:52:18, dschuyler wrote:
> > On 2016/06/30 06:45:54, Dan Beam wrote:
> > > can you just omit URL?
> > 
> > We could change the SettingsRoute typedef to not require
> > the url.
> 
> how are all the other assignments to currentRoute compiling?
>
https://cs.chromium.org/search/?q=%22currentroute+%3D%22&sq=package:chromium&...
> 
> > My hope is that requiring the URL
> > will help others specify routes correctly in
> > settings_router.js. Setting the url here seems
> > like a small price to have help getting the 
> > routes correct. WDYT
> 
> forcing folks to copy 1 more key that's partially used doesn't seem like an
> improvement.
> 
> 1) the URL is set based on this code, which doesn't care about the "url" key:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> 
> 2) the URLs you're specifying are wrong, as far as I can tell:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> 
> so how does using url "help getting the routes correct"?
> 
> if you really want to make this better, we should make named routes, i.e.
> 
> Routes = {
>   BASIC: {
>     url: '/',
>     ...
>   },
>   ADVANCED: {
>     url: '/advanced',
>     ...
>   },
> },
> 
> routes_: [Routes.BASIC, Routes.ADVANCED, ...],
> 
> and then from somewhere that needs to navigate:
> 
> this.currentRoute = Routes.BASIC;

Acknowledged.

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:135:
Polymer.dom.flush();
On 2016/06/30 06:45:54, Dan Beam wrote:
> why is this required?

I moved it up to where this is called from
and commented it. I'm using the flush to 
apply the dom-if prior to calculating sizes.

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:137:
this.showAboutPage_) {
On 2016/06/30 06:45:54, Dan Beam wrote:
> can this be done earlier?

Done.

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:143:
'[section=reset]'));
On 2016/06/30 06:45:54, Dan Beam wrote:
> why the reset section?

Changed to fetching last settings-section.

[Dan Beam, 2016-07-02 00:17:31]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:135:
Polymer.dom.flush();
On 2016/07/01 19:21:21, dschuyler wrote:
> On 2016/06/30 06:45:54, Dan Beam wrote:
> > why is this required?
> 
> I moved it up to where this is called from
> and commented it. I'm using the flush to 
> apply the dom-if prior to calculating sizes.

can't we just use async() to act after this instead?

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

https://codereview.chromium.org/2090753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:123:
Polymer.dom.flush();
can't we just do:

this.async(function() {
  this.$.overscroll.style.paddingBottom = ...;
});

[dschuyler, 2016-07-02 00:23:55]

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

https://codereview.chromium.org/2090753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:135:
Polymer.dom.flush();
On 2016/07/02 00:17:31, Dan Beam wrote:
> On 2016/07/01 19:21:21, dschuyler wrote:
> > On 2016/06/30 06:45:54, Dan Beam wrote:
> > > why is this required?
> > 
> > I moved it up to where this is called from
> > and commented it. I'm using the flush to 
> > apply the dom-if prior to calculating sizes.
> 
> can't we just use async() to act after this instead?

Done.

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

https://codereview.chromium.org/2090753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:123:
Polymer.dom.flush();
On 2016/07/02 00:17:31, Dan Beam wrote:
> can't we just do:
> 
> this.async(function() {
>   this.$.overscroll.style.paddingBottom = ...;
> });

Done.

[Dan Beam, 2016-07-06 15:25:37]

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

https://codereview.chromium.org/2090753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:122: /* Wait
for the dom-if changes prior to calculating the overflow padding. */
// Use a 1-line comment for implementation comments.

https://google.github.io/styleguide/javascriptguide.xml#Comments

https://codereview.chromium.org/2090753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:139:
this.showAboutPage_) {
what is this logic trying to derive?

[dschuyler, 2016-07-06 21:32:19]

Patchset #5 (id:80001) has been deleted

[dschuyler, 2016-07-06 21:43:56]

Patchset #5 (id:100001) has been deleted

[dschuyler, 2016-07-06 21:45:05]

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

https://codereview.chromium.org/2090753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:122: /* Wait
for the dom-if changes prior to calculating the overflow padding. */
On 2016/07/06 15:25:36, Dan Beam wrote:
> // Use a 1-line comment for implementation comments.
> 
> https://google.github.io/styleguide/javascriptguide.xml#Comments

Done.

https://codereview.chromium.org/2090753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:139:
this.showAboutPage_) {
On 2016/07/06 15:25:36, Dan Beam wrote:
> what is this logic trying to derive?

I renamed and reworked this a bit to make it
clearer (and work better).

Done.

[michaelpg, 2016-07-07 20:09:38]

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

https://codereview.chromium.org/2090753002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:129:
this.$.overscroll.style.paddingBottom = this.overscrollHeight_() + 'px';
should this be floored at 0?

https://codereview.chromium.org/2090753002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:162: var
lastSection = sections[sections.length - 1];
seems overly complicated:

this.$$('settings-advanced-page').$$('settings-section:last-of-type')

[dschuyler, 2016-07-20 20:42:40]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu. Also includes a fix to set the route properly
when going to basic or advanced pages (without going to a section within
the page).

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

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu. Also includes a fix to set the route properly
when going to basic or advanced pages (without going to a section within
the page).

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-07-20 20:42:43]

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

[commit-bot: I haz the power, 2016-07-20 20:43:05]

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-07-20 20:43:06]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[dschuyler, 2016-07-20 20:44:08]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu. Also includes a fix to set the route properly
when going to basic or advanced pages (without going to a section within
the page).

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-07-20 20:44:20]

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

https://codereview.chromium.org/2090753002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:129:
this.$.overscroll.style.paddingBottom = this.overscrollHeight_() + 'px';
On 2016/07/07 20:09:38, michaelpg wrote:
> should this be floored at 0?

I added an assert in calcHeight instead.

https://codereview.chromium.org/2090753002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:162: var
lastSection = sections[sections.length - 1];
On 2016/07/07 20:09:38, michaelpg wrote:
> seems overly complicated:
> 
> this.$$('settings-advanced-page').$$('settings-section:last-of-type')

Done.

[Dan Beam, 2016-07-20 21:22:40]

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

https://codereview.chromium.org/2090753002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:166: function
calcHeight(scrollHeight, element) {
nit: missing a @param, BUT, can we just get this value when we need it?

var calcHeight = function(element) {
  var height = this.parentNode.scrollHeight - ...;
}.bind(this);

if we do end up needing a variable, should the name "availableHeight" be more
explanatory?

[Dan Beam, 2016-07-20 21:23:33]

otherwise lgtm

[commit-bot: I haz the power, 2016-07-20 21:46:24]

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

[commit-bot: I haz the power, 2016-07-20 21:46:24]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-07-21 00:13:34]

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

[michaelpg, 2016-07-21 00:14:06]

nlgtm until you make the "somewhat" dynamic padding less "somewhat", and more
dynamic :-)

1. chrome://md-settings
2. click Advanced

there's now almost 100% height worth of padding-bottom

3. click Advanced again to close

there's still 100% worth of padding-bottom, and now the toggle button is
literally all I see: http://i.imgur.com/xDRDX8s.png

[commit-bot: I haz the power, 2016-07-21 00:14:22]

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

[dschuyler, 2016-07-21 00:14:23]

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

https://codereview.chromium.org/2090753002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:166: function
calcHeight(scrollHeight, element) {
On 2016/07/20 21:22:39, Dan Beam wrote:
> nit: missing a @param, BUT, can we just get this value when we need it?
> 
> var calcHeight = function(element) {
>   var height = this.parentNode.scrollHeight - ...;
> }.bind(this);
> 
> if we do end up needing a variable, should the name "availableHeight" be more
> explanatory?

That is nicer, thanks.

Done.

[commit-bot: I haz the power, 2016-07-21 00:14:24]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[michaelpg, 2016-07-21 00:15:21]

On 2016/07/21 00:14:06, michaelpg wrote:
> nlgtm until you make the "somewhat" dynamic padding less "somewhat", and more
> dynamic :-)
> 
> 1. chrome://md-settings
> 2. click Advanced
> 
> there's now almost 100% height worth of padding-bottom
> 
> 3. click Advanced again to close
> 
> there's still 100% worth of padding-bottom, and now the toggle button is
> literally all I see: http://i.imgur.com/xDRDX8s.png

Gosh darnit Rietveld. I'm not even going to try to remember the incantation,
just don't land this until it works a bit better.

[Dan Beam, 2016-07-21 00:29:39]

so, btw, are we gonna remove the overscroll if the user scrolls up?

[commit-bot: I haz the power, 2016-07-21 00:55:10]

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

[commit-bot: I haz the power, 2016-07-21 00:55:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2016-07-21 04:28:13]

On 2016/07/21 00:29:39, Dan Beam wrote:
> so, btw, are we gonna remove the overscroll if the user scrolls up?

eek, changing the length of the scrollbar as you scroll? TBF, not doing so is
also "eek", but see #9 :-)

[dschuyler, 2016-07-21 18:33:45]

On 2016/07/21 04:28:13, michaelpg wrote:
> On 2016/07/21 00:29:39, Dan Beam wrote:
> > so, btw, are we gonna remove the overscroll if the user scrolls up?
> 
> eek, changing the length of the scrollbar as you scroll? TBF, not doing so is
> also "eek", but see #9 :-)

I'd like to keep this CL focused on adding the padding. The scrolling
and more dynamic padding behavior will likely be an a large enough
topic on it's own.

[Dan Beam, 2016-07-21 19:08:38]

aight still lgtm then

[michaelpg, 2016-07-21 21:21:48]

On 2016/07/21 19:08:38, Dan Beam wrote:
> aight still lgtm then

I'm still not a fan of the regression I outlined in #32. But it's not as severe
now that we don't auto-scroll when closing Advanced.

AFAICT, this CL does 3 things. Please point out where I'm mistaken or not
understanding the intent.

1) After any navigation to the Basic page (aside from the initial page load),
adds enough padding that the Advanced toggle button can be scrolled to the top
of the page.
2) After any navigation to the Advanced page (aside from the initial page load),
adds enough padding that the bottom-most section can be scrolled to top of the
page.
3) After any other navigation, clear the padding.

I believe 1) is a bug this CL introduces. I also don't understand the logic
behind 2) -- if the padding isn't based on the section, why not just set the
padding on page load? It seems like you only want to change it when the window
resizes, or clear it when a sub-page or About is opened.

[dschuyler, 2016-07-23 00:50:56]

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

[commit-bot: I haz the power, 2016-07-23 00:51:16]

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-07-23 00:51:18]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[dschuyler, 2016-07-23 00:57:47]

On 2016/07/21 21:21:48, michaelpg wrote:
> On 2016/07/21 19:08:38, Dan Beam wrote:
> > aight still lgtm then
> 
> I'm still not a fan of the regression I outlined in #32. But it's not as
severe
> now that we don't auto-scroll when closing Advanced.
> 
> AFAICT, this CL does 3 things. Please point out where I'm mistaken or not
> understanding the intent.
> 
> 1) After any navigation to the Basic page (aside from the initial page load),
> adds enough padding that the Advanced toggle button can be scrolled to the top
> of the page.
Okay, I've changed it now to keep the last section of the basic page on the
screen
so that the window will not just have the advanced toggle control.

> 2) After any navigation to the Advanced page (aside from the initial page
load),
> adds enough padding that the bottom-most section can be scrolled to top of the
> page.
True.

> 3) After any other navigation, clear the padding.
This CL doesn't intend to do that (it doesn't).

> 
> I believe 1) is a bug this CL introduces. I also don't understand the logic
> behind 2) -- if the padding isn't based on the section, why not just set the
> padding on page load? It seems like you only want to change it when the window
> resizes, or clear it when a sub-page or About is opened.
I agree that it would be much simpler to put padding at the end of the page and
leave it there. I trust tbuckley@ and he made the call on the current design.
Though maybe his opinion could be swayed to something simpler.

[dschuyler, 2016-07-23 01:01:11]

On 2016/07/23 00:57:47, dschuyler wrote:
> On 2016/07/21 21:21:48, michaelpg wrote:
> > On 2016/07/21 19:08:38, Dan Beam wrote:
> > > aight still lgtm then
> > 
> > I'm still not a fan of the regression I outlined in #32. But it's not as
> severe
> > now that we don't auto-scroll when closing Advanced.
> > 
> > AFAICT, this CL does 3 things. Please point out where I'm mistaken or not
> > understanding the intent.
> > 
> > 1) After any navigation to the Basic page (aside from the initial page
load),
> > adds enough padding that the Advanced toggle button can be scrolled to the
top
> > of the page.
> Okay, I've changed it now to keep the last section of the basic page on the
> screen
> so that the window will not just have the advanced toggle control.
> 
> > 2) After any navigation to the Advanced page (aside from the initial page
> load),
> > adds enough padding that the bottom-most section can be scrolled to top of
the
> > page.
> True.
> 
> > 3) After any other navigation, clear the padding.
> This CL doesn't intend to do that (it doesn't).
Clarification: it does clear it when going to a sub-page or About page, but I 
wouldn't say 'any other navigation' (it doesn't clear it on browser back, for 
example).
> 
> > 
> > I believe 1) is a bug this CL introduces. I also don't understand the logic
> > behind 2) -- if the padding isn't based on the section, why not just set the
> > padding on page load? It seems like you only want to change it when the
window
> > resizes, or clear it when a sub-page or About is opened.
> I agree that it would be much simpler to put padding at the end of the page
and
> leave it there. I trust tbuckley@ and he made the call on the current design.
> Though maybe his opinion could be swayed to something simpler.

[commit-bot: I haz the power, 2016-07-23 01:24:08]

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

[commit-bot: I haz the power, 2016-07-23 01:24:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2016-07-25 16:56:40]

lgtm

[dschuyler, 2016-07-25 18:29:29]

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

[commit-bot: I haz the power, 2016-07-25 18:30:10]

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-07-25 18:30:13]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[dschuyler, 2016-07-25 19:26:04]

Fix for unit testing.

[commit-bot: I haz the power, 2016-07-25 20:03:11]

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

[commit-bot: I haz the power, 2016-07-25 20:03:12]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-07-25 21:19:22]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-07-25 21:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2090753002/#ps200001
(title: "unit test fix")

[commit-bot: I haz the power, 2016-07-25 21:20:08]

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-07-25 21:20:13]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-25 21:25:05]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-25 21:25:06]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-07-25 21:26:38]

Description was changed from

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] end of page padding in content area

This CL adds (somewhat) dynamic padding to the end of the MD settings
so that the last item may scroll to the top of the page when selected
in the side nav menu.

BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/68a872b1209e48315f408d5d1693d2636df299b1
Cr-Commit-Position: refs/heads/master@{#407581}
==========

[commit-bot: I haz the power, 2016-07-25 21:26:39]

Patchset 9 (id:??) landed as
https://crrev.com/68a872b1209e48315f408d5d1693d2636df299b1
Cr-Commit-Position: refs/heads/master@{#407581}