[MD settings] hide scrollbar during animation to subpage

[MD settings] hide scrollbar during animation to subpage

This CL will hide overflow during the horizontal swipe page animation.
The overflow will be reset after the animation completes.

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

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

[dschuyler, 2017-03-17 23:27:04]

Description was changed from

==========
[MD settings] hide scrollbar during animation to subpage

This CL will hide overflow during the horizontal swipe page animation.
The overflow will be reset after the animation completes.

BUG=645346
==========

to

==========
[MD settings] hide scrollbar during animation to subpage

This CL will hide overflow during the horizontal swipe page animation.
The overflow will be reset after the animation completes.

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

[dschuyler, 2017-03-17 23:27:06]

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

[commit-bot: I haz the power, 2017-03-17 23:27:52]

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

[dschuyler, 2017-03-17 23:34:14]

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

[dschuyler, 2017-03-17 23:34:30]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-17 23:34:41]

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

[dschuyler, 2017-03-18 00:02:50]

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

[commit-bot: I haz the power, 2017-03-18 00:03:45]

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

[dschuyler, 2017-03-18 00:12:46]

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

[dschuyler, 2017-03-18 00:12:47]

fyi, here is a list of things that were tested for with this CL:
- load directly to chrome://settings/content on a small window, scroll bars
appear
- a scroll bar is present in manage certs when the window is smaller than the
content
- search for "manage" doesn't clip the yellow balloon text in the results.
- no horizontal scrollbar flash when going back from subsubpage
- correct scroll bar in about page
- correct scroll bar in customize fonts

The fix is done by removing the scroll bar during the animation. We saw that
this matches the behavior of the expand animations from the main page (that was
done previously by michaelpg@).

[Dan Beam, 2017-03-18 00:23:23]

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

[Dan Beam, 2017-03-18 00:23:24]

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

https://codereview.chromium.org/2758863004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:84:
settings.Route.SITE_SETTINGS.contains(oldRoute)) {
can you do this without accessing specific routes (instead using .depth,
.parent, or .section?)

[dschuyler, 2017-03-18 00:47:17]

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

[commit-bot: I haz the power, 2017-03-18 00:47:58]

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

[dschuyler, 2017-03-18 00:48:26]

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

[commit-bot: I haz the power, 2017-03-18 00:48:42]

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

[dschuyler, 2017-03-18 00:49:59]

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

https://codereview.chromium.org/2758863004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:84:
settings.Route.SITE_SETTINGS.contains(oldRoute)) {
On 2017/03/18 00:23:24, Dan Beam wrote:
> can you do this without accessing specific routes (instead using .depth,
> .parent, or .section?)

Excellent point.
Done.

[Dan Beam, 2017-03-18 00:53:27]

this generally seems fine to me

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

https://codereview.chromium.org/2758863004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:83: if
(oldRoute && oldRoute.depth == 3 && oldRoute.depth > newRoute.depth)
why == 3?

[dschuyler, 2017-03-18 01:00:07]

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

[dschuyler, 2017-03-18 01:00:29]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-18 01:01:00]

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

[dschuyler, 2017-03-18 01:02:31]

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

https://codereview.chromium.org/2758863004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:83: if
(oldRoute && oldRoute.depth == 3 && oldRoute.depth > newRoute.depth)
On 2017/03/18 00:53:26, Dan Beam wrote:
> why == 3?

How about isSubpage() and I remove the 3.

[dpapad, 2017-03-18 01:19:15]

LGTM. Thanks for investigating this tricky bug.

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

https://codereview.chromium.org/2758863004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:52: * This
may catch unrelated animations finishing.
Nit (optional): The last sentence does not make it clear if this is a TODO to be
fixed or if it is harmless. Perhaps rehprase to be more explicit as follows

"This may catch unrelated animations finishing, which is not known to cause any
issues."

Having said that, looking at the code, it seems that such an event can be fired
only by our settings-animated-pages, so I don't see how anything unrelated to
page transitions could trigger it.

[dschuyler, 2017-03-18 01:33:37]

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

[commit-bot: I haz the power, 2017-03-18 01:33:49]

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

[dschuyler, 2017-03-18 01:34:36]

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

https://codereview.chromium.org/2758863004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:52: * This
may catch unrelated animations finishing.
On 2017/03/18 01:19:15, dpapad wrote:
> Nit (optional): The last sentence does not make it clear if this is a TODO to
be
> fixed or if it is harmless. Perhaps rehprase to be more explicit as follows
> 
> "This may catch unrelated animations finishing, which is not known to cause
any
> issues."
> 
> Having said that, looking at the code, it seems that such an event can be
fired
> only by our settings-animated-pages, so I don't see how anything unrelated to
> page transitions could trigger it.

Done.

[commit-bot: I haz the power, 2017-03-18 02:51:41]

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

[commit-bot: I haz the power, 2017-03-18 02:51:42]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-03-21 07:12:50]

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

https://codereview.chromium.org/2758863004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:85: if
(oldRoute && newRoute.isSubpage() && oldRoute.depth > newRoute.depth)
i'm still confused.

wouldn't we want to handle chopping off overflow when navigating from a subpage
to a subsubpage as well as from subsubpage to subpage?

does this accomplish that?  seems like it only addresses subsubpage -> subpage

[dschuyler, 2017-03-21 18:19:19]

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

https://codereview.chromium.org/2758863004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:85: if
(oldRoute && newRoute.isSubpage() && oldRoute.depth > newRoute.depth)
On 2017/03/21 07:12:50, Dan Beam wrote:
> i'm still confused.
> 
> wouldn't we want to handle chopping off overflow when navigating from a
subpage
> to a subsubpage as well as from subsubpage to subpage?
> 
> does this accomplish that?  seems like it only addresses subsubpage -> subpage

To the best of my knowledge an erroneous scroll bar does not appear during that
transition. Even running a 1/10 speed (using the animation dev control) that
transition looks ok. I think I was looking at it as a hack/hammer to be used
sparingly, but I named that var isSubpageAnimating which is more general than a
var name like hideErroneousScrollbar. It sounds like I should either change the
var name, comment the existing var more clearly, or maybe change the test to
something like:
if (oldRoute && (oldRoute.isSubpage() || newRoute.isSubpage())
since a subpage is actually animating while going into a sub-subpage.

[Dan Beam, 2017-03-21 20:39:28]

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

https://codereview.chromium.org/2758863004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:85: if
(oldRoute && newRoute.isSubpage() && oldRoute.depth > newRoute.depth)
On 2017/03/21 18:19:18, dschuyler wrote:
> On 2017/03/21 07:12:50, Dan Beam wrote:
> > i'm still confused.
> > 
> > wouldn't we want to handle chopping off overflow when navigating from a
> subpage
> > to a subsubpage as well as from subsubpage to subpage?
> > 
> > does this accomplish that?  seems like it only addresses subsubpage ->
subpage
> 
> To the best of my knowledge an erroneous scroll bar does not appear during
that
> transition. Even running a 1/10 speed (using the animation dev control) that
> transition looks ok. I think I was looking at it as a hack/hammer to be used
> sparingly, but I named that var isSubpageAnimating which is more general than
a
> var name like hideErroneousScrollbar. It sounds like I should either change
the
> var name, comment the existing var more clearly,

how about name hideOverflow?

> or maybe change the test to
> something like:
> if (oldRoute && (oldRoute.isSubpage() || newRoute.isSubpage())
> since a subpage is actually animating while going into a sub-subpage.

yeah, this ^ conditional is what i was thinking but what about loading
chrome://settings/content/cookies directly?  do we need to hide overflow for
this?  navigating directly to a subpage in a situation in which !oldRoute might
be true.

[dschuyler, 2017-03-22 00:41:40]

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

[commit-bot: I haz the power, 2017-03-22 00:42:08]

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

[dschuyler, 2017-03-22 00:58:50]

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

https://codereview.chromium.org/2758863004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:85: if
(oldRoute && newRoute.isSubpage() && oldRoute.depth > newRoute.depth)
On 2017/03/21 20:39:28, Dan Beam wrote:
> On 2017/03/21 18:19:18, dschuyler wrote:
> > On 2017/03/21 07:12:50, Dan Beam wrote:
> > > i'm still confused.
> > > 
> > > wouldn't we want to handle chopping off overflow when navigating from a
> > subpage
> > > to a subsubpage as well as from subsubpage to subpage?
> > > 
> > > does this accomplish that?  seems like it only addresses subsubpage ->
> subpage
> > 
> > To the best of my knowledge an erroneous scroll bar does not appear during
> that
> > transition. Even running a 1/10 speed (using the animation dev control) that
> > transition looks ok. I think I was looking at it as a hack/hammer to be used
> > sparingly, but I named that var isSubpageAnimating which is more general
than
> a
> > var name like hideErroneousScrollbar. It sounds like I should either change
> the
> > var name, comment the existing var more clearly,
> 
> how about name hideOverflow?

That would work for me :), but it seems to be 'presentational', which this style
guide advises against:
https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Naming.

I'll make the isSubpageAnimation accurate instead.
 
> > or maybe change the test to
> > something like:
> > if (oldRoute && (oldRoute.isSubpage() || newRoute.isSubpage())
> > since a subpage is actually animating while going into a sub-subpage.
> 
> yeah, this ^ conditional is what i was thinking 

This is done. So it is now really an 'is a subpage animating' variable. 

> but what about loading
> chrome://settings/content/cookies directly?  do we need to hide overflow for
> this?  navigating directly to a subpage in a situation in which !oldRoute
might
> be true.

When going directly to the cookies page, it does the vertical expand animation
rather than a horizontal swipe animation (so the horizontal swipe issue hasn't
been seen there).

[commit-bot: I haz the power, 2017-03-22 01:40:05]

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

[commit-bot: I haz the power, 2017-03-22 01:40:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dan Beam, 2017-03-22 01:52:37]

lgtm let's give it another try

[dschuyler, 2017-03-22 20:18:38]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-03-22 20:18:38]

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/2758863004/#ps140001
(title: "animating when either is a subpage")

[commit-bot: I haz the power, 2017-03-22 20:19:10]

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-22 20:23:56]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490213918071670,
"parent_rev": "3acae57c28ecf42a0089f5649ed59d476968e539", "commit_rev":
"cd286e8aba904530c17eeafbd626769822c8a746"}

[commit-bot: I haz the power, 2017-03-22 20:24:43]

Description was changed from

==========
[MD settings] hide scrollbar during animation to subpage

This CL will hide overflow during the horizontal swipe page animation.
The overflow will be reset after the animation completes.

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

to

==========
[MD settings] hide scrollbar during animation to subpage

This CL will hide overflow during the horizontal swipe page animation.
The overflow will be reset after the animation completes.

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

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

[commit-bot: I haz the power, 2017-03-22 20:24:47]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cd286e8aba904530c17eeafbd626...