MD Settings: Set overscroll before attempting to scroll to section

MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4a12c6d364a12f51c18854c0b16bae824a55c88e
Cr-Commit-Position: refs/heads/master@{#422569}

[michaelpg, 2016-09-07 03:09:55]

Description was changed from

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This works in my testing, but there are so many permutations to check that it's
possible this breaks some other behavior. We shouldn't speculatively keep the
setTimeout in (calling call setOverscroll_ twice, sync and async). But if it
turns out this CL causes problems, that will be a likely suspect.

BUG=644583
R=dschuyler@chromium.org
==========

to

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This works in my testing, but there are so many permutations to check that it's
possible this breaks some other behavior. We shouldn't speculatively keep the
setTimeout in (calling call setOverscroll_ twice, sync and async). But if it
turns out this CL causes problems, that will be a likely suspect.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-09-07 03:12:01]

PTAL...

[dschuyler, 2016-09-07 20:33:32]

On 2016/09/07 03:12:01, michaelpg wrote:
> PTAL...

Thanks, I tried out the CL.

I noticed that going to the chrome://md-settings/reset
directly doesn't add the overscroll like it should.

Also, going into the Search Engines subpage and then
using the menu to go to Reset settings doesn't add
the overscroll like it should.

Are those (or could the fixes be) part of this CL

[michaelpg, 2016-09-07 22:34:12]

On 2016/09/07 20:33:32, dschuyler wrote:
> On 2016/09/07 03:12:01, michaelpg wrote:
> > PTAL...
> 
> Thanks, I tried out the CL.
> 
> I noticed that going to the chrome://md-settings/reset
> directly doesn't add the overscroll like it should.

Existing issue: https://bugs.chromium.org/p/chromium/issues/detail?id=637508

That is a separate timing issue that I think can be resolved by fixing our
<link> imports, working on a CL for that.

> 
> Also, going into the Search Engines subpage and then
> using the menu to go to Reset settings doesn't add
> the overscroll like it should.

Bleh, thanks for catching that. That's a regression. So, okay, keeping the
setTimeout to recalculate overscroll after the section animatiotn begins will
help.

I found another bug, which isn't affected by this CL AFAICT: going into the
Search Engines subpage and then using the menu to go to On Startup. This is
happening because after the overscroll is correctly set in the setTimeout, a
scroll event fires, calling boundScroll_ which resets the overscroll to 0.

> 
> Are those (or could the fixes be) part of this CL

[michaelpg, 2016-09-07 22:39:17]

with setTimeout

[dschuyler, 2016-09-08 21:00:37]

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

https://codereview.chromium.org/2312423003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:192:
this.setOverscroll_(this.overscrollHeight_());
I added some logging to see how often this call
has correct data (compared to the version called
after a timeout). It occasionally does have the 
correct value, such as when the net effect is 
scrolling down to a section. It will also get 
incorrect values, such as when the net effect is
scrolling up to a section.

Is there some way to chain the actions with a 
promise or events (avoiding timeouts or using
stale data)?

[Dan Beam, 2016-09-26 18:22:57]

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

[Dan Beam, 2016-09-26 18:22:57]

what's the status on this CL?

[michaelpg, 2016-09-28 22:26:59]

PTAL. I've consolidated freezing scrolling & overscroll into settings-main,
triggered by an event from basic/advanced.

I think this fixes most issues without creating any regressions. Dave, would you
mind playing around with it to check my work?

Couple examples:

1. chrome://md-settings
2. side nav =>

[michaelpg, 2016-09-28 22:27:43]

oops, full message:

On 2016/09/28 22:26:59, michaelpg wrote:
> PTAL. I've consolidated freezing scrolling & overscroll into settings-main,
> triggered by an event from basic/advanced.
> 
> I think this fixes most issues without creating any regressions. Dave, would
you
> mind playing around with it to check my work?
> 
> Couple examples:
> 
> 1. chrome://md-settings
> 2. side nav => on startup (or advanced => reset)
> 
> 1. chrome://md-settings
> 2. open Manage Profile
> 3. side nav => on startup (or advanced => reset)

[michaelpg, 2016-09-28 22:29:35]

Description was changed from

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This works in my testing, but there are so many permutations to check that it's
possible this breaks some other behavior. We shouldn't speculatively keep the
setTimeout in (calling call setOverscroll_ twice, sync and async). But if it
turns out this CL causes problems, that will be a likely suspect.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-09-28 22:33:11]

Here's an example test showing the timing struggles. Note
the "guaranteed to flake or your money back" line:
https://codereview.chromium.org/2378823004

[Dan Beam, 2016-09-29 03:04:20]

this seems fine to me, but deferring to dschuyler@

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

https://codereview.chromium.org/2312423003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:140: * @param
{boolean} detail True to freeze, false to unfreeze.
why can't we just name this "freeze" instead of "detail"?

[michaelpg, 2016-09-29 03:10:50]

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

https://codereview.chromium.org/2312423003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:140: * @param
{boolean} detail True to freeze, false to unfreeze.
On 2016/09/29 03:04:20, Dan Beam wrote:
> why can't we just name this "freeze" instead of "detail"?

I can if you want. My rationale: "detail" is what the function is actually
invoked with, so it's a common convention. IOW, because onFoo_ handlers called
by Polymer feel more like "overriding" to me so renaming parameters seems weird.

[dschuyler, 2016-09-29 21:51:31]

On 2016/09/28 22:27:43, michaelpg wrote:
> oops, full message:
> 
> On 2016/09/28 22:26:59, michaelpg wrote:
> > PTAL. I've consolidated freezing scrolling & overscroll into settings-main,
> > triggered by an event from basic/advanced.
> > 
> > I think this fixes most issues without creating any regressions. Dave, would
> you
> > mind playing around with it to check my work?
> > 
> > Couple examples:
> > 
> > 1. chrome://md-settings
> > 2. side nav => on startup (or advanced => reset)
> > 
> > 1. chrome://md-settings
> > 2. open Manage Profile
> > 3. side nav => on startup (or advanced => reset)

Thanks!

I patched in Patch#3 and tried it out. Here are some
things I found.

My takeaway is that at a user level the behavior is
similar with and without the patch. It is better, but 
it's hard to notice at a high (user) level.

The improvement:

The thing that I found clearly better with the patch
is doing this: using the side nav, go to reset, then
system, then a11y, then system, then reset. (i.e.
kinda go up the items and back down to the bottom).
Without the patch going up works, while down doesn't.
With the patch both directions work.

The not improved:

If I navigate directly to 
chrome://md-settings/downloadsDirectory
the overscroll is not set (and the Downloads settings
doesn't scroll to the top). A similar thing happens for
chrome://md-settings/reset and chrome://md-settings/search.
(and likely others that I didn't test). Without this
patch the overscroll/scroll works about 1 in 6 times.

If I navigate to chrome://md-settings/reset using
the side nav (it works) and then click to refresh
the page it sometimes doesn't add the overscroll
or scroll the section to the top (it did it 
correctly one time out of several attempts). Without
the patch the behavior is similar.

If I open the search engines subpage and then select
About from the side nav, the About Chromium section
will appear and then jump slightly to the right (my
guess is that it's the scroll bar width). Without
the patch the behavior is similar.

[dschuyler, 2016-09-29 21:53:18]

lgtm

[michaelpg, 2016-09-29 22:47:40]

On 2016/09/29 21:51:31, dschuyler wrote:
> On 2016/09/28 22:27:43, michaelpg wrote:
> > oops, full message:
> > 
> > On 2016/09/28 22:26:59, michaelpg wrote:
> > > PTAL. I've consolidated freezing scrolling & overscroll into
settings-main,
> > > triggered by an event from basic/advanced.
> > > 
> > > I think this fixes most issues without creating any regressions. Dave,
would
> > you
> > > mind playing around with it to check my work?
> > > 
> > > Couple examples:
> > > 
> > > 1. chrome://md-settings
> > > 2. side nav => on startup (or advanced => reset)
> > > 
> > > 1. chrome://md-settings
> > > 2. open Manage Profile
> > > 3. side nav => on startup (or advanced => reset)
> 
> Thanks!
> 
> I patched in Patch#3 and tried it out. Here are some
> things I found.

Thanks! And thanks for the detailed write-up.

> 
> My takeaway is that at a user level the behavior is
> similar with and without the patch. It is better, but 
> it's hard to notice at a high (user) level.
> 
> The improvement:
> 
> The thing that I found clearly better with the patch
> is doing this: using the side nav, go to reset, then
> system, then a11y, then system, then reset. (i.e.
> kinda go up the items and back down to the bottom).
> Without the patch going up works, while down doesn't.
> With the patch both directions work.

Yes, this is what the CL is intended to fix (phew).

> 
> The not improved:
> 
> If I navigate directly to 
> chrome://md-settings/downloadsDirectory
> the overscroll is not set (and the Downloads settings
> doesn't scroll to the top). A similar thing happens for
> chrome://md-settings/reset and chrome://md-settings/search.
> (and likely others that I didn't test). Without this
> patch the overscroll/scroll works about 1 in 6 times.

Can't repro with or without patch. I think this stopped happening
after dpapad's https://codereview.chromium.org/2360873005

> 
> If I navigate to chrome://md-settings/reset using
> the side nav (it works) and then click to refresh
> the page it sometimes doesn't add the overscroll
> or scroll the section to the top (it did it 
> correctly one time out of several attempts). Without
> the patch the behavior is similar.

I've seen this too, but can't reproduce at the moment.

> 
> If I open the search engines subpage and then select
> About from the side nav, the About Chromium section
> will appear and then jump slightly to the right (my
> guess is that it's the scroll bar width). Without
> the patch the behavior is similar.

Filed crbug.com/651616

[michaelpg, 2016-09-30 01:12:13]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-09-30 01:12:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2312423003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-30 01:12:36]

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

[michaelpg, 2016-09-30 01:30:05]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2016-10-03 20:24:18]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-10-03 20:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2312423003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-03 20:25:01]

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-10-03 22:31:33]

Description was changed from

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-03 22:31:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 22:33:28]

Description was changed from

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Set overscroll before attempting to scroll to section

Fixes scrolling when tapping an item in the drawer nav menu below the current
scroll position.

https://codereview.chromium.org/2230123002 made MainPageBehavior try scrolling
to a section synchronously, but settings-main updates the overscroll after a
setTimeout. We should be able to update the overscroll synchronously as well --
we originally had issues with tests but hopefully the code is now more robust.

This consolidates toggleScrolling_ into settings-main via the freeze-scroll
event.

Testing is difficult because of timing issues: we have to wait for the page
to be fully 100% loaded before testing scrolling, or sections could change
height and interfere with the test. But we don't have any way of knowing that.

BUG=644583
R=dschuyler@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4a12c6d364a12f51c18854c0b16bae824a55c88e
Cr-Commit-Position: refs/heads/master@{#422569}
==========

[commit-bot: I haz the power, 2016-10-03 22:33:29]

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