MD Settings: Remove subpage animation when landing directly on it.

Experiment to remove subpage animation when landing directly on it.

BUG=700547, 722790
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[scottchen, 2017-04-18 22:35:14]

Description was changed from

==========
Experiment to remove subpage animation when landing directly on it.

BUG=700547
==========

to

==========
Experiment to remove subpage animation when landing directly on it.

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

[scottchen, 2017-04-19 18:03:50]

Description was changed from

==========
Experiment to remove subpage animation when landing directly on it.

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

to

==========
Experiment to remove subpage animation when landing directly on it.

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

[scottchen, 2017-04-19 18:05:21]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, dpapad@chromium.org

[scottchen, 2017-04-19 18:05:22]

attempt to remove the animation with minimal code-change, you can see the result
screen capture here:
https://bugs.chromium.org/p/chromium/issues/detail?id=700547#c6

If doesn't look good enough still, we can attempt to rework how we show
main/subpage views but it'll take a lot more time.

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

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:93:
.setAttribute('hidden', true);
without this line, the basic page will briefly appear before landing on the
subpage:
https://drive.google.com/a/google.com/file/d/0ByIc0PEad7DmN1IzQnR0TDFYQkE/vie...

[Dan Beam, 2017-04-22 01:31:22]

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

[Dan Beam, 2017-04-22 01:31:22]

i think michaelpg@ is probably the best reviewer for this...

[michaelpg, 2017-04-22 04:01:59]

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

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:92:
document.body.querySelector('settings-ui::shadow #container')
::shadow makes me sad, is there no other way to get to the container?

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:93:
.setAttribute('hidden', true);
On 2017/04/19 18:05:22, scottchen wrote:
> without this line, the basic page will briefly appear before landing on the
> subpage:
>
https://drive.google.com/a/google.com/file/d/0ByIc0PEad7DmN1IzQnR0TDFYQkE/vie...

Yeah, it's probably necessary unless we want to make bigger changes. It would be
nice if we could call toggleOtherSectionsHidden, but that doesn't work if the
other sections aren't stamped yet >_<

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:123:
document.body.querySelector('settings-ui::shadow #container')
Does this only take effect on the first call? Perhaps it's better to unset this
up on line 95, in the setTimeout (the same place we add the attribute) before
calling tryTransitionToSection.

e.g., something like
  setTimeout(function() {
    document.body......removeAttribute('hidden');
    this.tryTransitionToSection_(scrollToSection, true);
  }.bind(this));

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:221:
this.scroller.scrollTop = 0;
when is this not starting at 0?

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:228: //
Try to wait for the section to be created.
We *might* not need this anymore if the goal is to show sub-pages immediately on
load, so you could replace this with the body of the `if` above and not have to
add the |immediate| parameter.

(But maybe it's still necessary if we haven't stamped Advanced yet, and we
navigate to an Advanced subpage?)

[scottchen, 2017-04-26 17:26:41]

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

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:92:
document.body.querySelector('settings-ui::shadow #container')
On 2017/04/22 04:01:58, michaelpg wrote:
> ::shadow makes me sad, is there no other way to get to the container?

I changed settings-ui to pass a "shouldHideContainer" flag that can be changed
by mainPageBehavior.

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:93:
.setAttribute('hidden', true);
On 2017/04/22 04:01:58, michaelpg wrote:
> On 2017/04/19 18:05:22, scottchen wrote:
> > without this line, the basic page will briefly appear before landing on the
> > subpage:
> >
>
https://drive.google.com/a/google.com/file/d/0ByIc0PEad7DmN1IzQnR0TDFYQkE/vie...
> 
> Yeah, it's probably necessary unless we want to make bigger changes. It would
be
> nice if we could call toggleOtherSectionsHidden, but that doesn't work if the
> other sections aren't stamped yet >_<

Acknowledged.

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:123:
document.body.querySelector('settings-ui::shadow #container')
On 2017/04/22 04:01:58, michaelpg wrote:
> Does this only take effect on the first call? Perhaps it's better to unset
this
> up on line 95, in the setTimeout (the same place we add the attribute) before
> calling tryTransitionToSection.
> 
> e.g., something like
>   setTimeout(function() {
>     document.body......removeAttribute('hidden');
>     this.tryTransitionToSection_(scrollToSection, true);
>   }.bind(this));

Done.

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:221:
this.scroller.scrollTop = 0;
On 2017/04/22 04:01:58, michaelpg wrote:
> when is this not starting at 0?

Never, this is not necessary. Removing.

https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:228: //
Try to wait for the section to be created.
On 2017/04/22 04:01:58, michaelpg wrote:
> We *might* not need this anymore if the goal is to show sub-pages immediately
on
> load, so you could replace this with the body of the `if` above and not have
to
> add the |immediate| parameter.
> 
> (But maybe it's still necessary if we haven't stamped Advanced yet, and we
> navigate to an Advanced subpage?)

Using !section.canAnimateExpand() to indicate first-load doesn't seem explicit
enough - canAnimateExpand() can also return false if it's half way through the
expanding animation, so I think it's safer to keep them separate.

[dpapad, 2017-04-26 20:16:20]

michaelpg@ is a better reviewer for this. Removing myself.

[dpapad, 2017-04-26 20:16:30]

dpapad@chromium.org changed reviewers:
- dpapad@chromium.org

[michaelpg, 2017-04-27 20:00:20]

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

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.js:71:
shouldHideContainer: {
this should be added by the behavior - is there a need to override it?

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

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:34: //
Controls to show or hide parent container during loading state.
nit: "Whether to hide parent container..."

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:131: *
@param {boolean} scrollToSection
document immediate

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:226: if
(immediate) {
how about making this a separate function, instead of introducing a parameter?
then invoke in tryTransitionToSection_:

if (currentRoute.isSubpage()) {
  if (immediate)
    this.expandSectionImmediate_(currentSection);
  else
    promise = ....

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:229:
this.classList.add('showing-subpage');
if so, we could create a helper method for the common stuff in these lines and
the lines beginning on 254

[scottchen, 2017-05-01 19:25:07]

I've updated the CL based on your comments, michaelpg@.

I also had to add a couple slightly unfortunate additions, since I was noticing
some edge-case kinks. PTAL.

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

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.js:71:
shouldHideContainer: {
On 2017/04/27 20:00:20, michaelpg wrote:
> this should be added by the behavior - is there a need to override it?

Looks like I don't need it.

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

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:34: //
Controls to show or hide parent container during loading state.
On 2017/04/27 20:00:20, michaelpg wrote:
> nit: "Whether to hide parent container..."

Done.

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:131: *
@param {boolean} scrollToSection
On 2017/04/27 20:00:20, michaelpg wrote:
> document immediate

Done.

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:226: if
(immediate) {
On 2017/04/27 20:00:20, michaelpg wrote:
> how about making this a separate function, instead of introducing a parameter?
> then invoke in tryTransitionToSection_:
> 
> if (currentRoute.isSubpage()) {
>   if (immediate)
>     this.expandSectionImmediate_(currentSection);
>   else
>     promise = ....

Done.

https://codereview.chromium.org/2825203003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:229:
this.classList.add('showing-subpage');
On 2017/04/27 20:00:20, michaelpg wrote:
> if so, we could create a helper method for the common stuff in these lines and
> the lines beginning on 254

Done.

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

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:171:
this.shouldHideContainer = true;
I had to add this, since the basic page was still briefly showing up while the
advanced page was being loaded.

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:180:
this.shouldHideContainer = false;
I had to add this, since the basic page was still briefly showing up while the
advanced page was being loaded.

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:233:
this.fire('resize');
I had to add this, otherwise iron-lists inside of sub-pages would not load
correctly (since the subpage did not have a size when iron-list attached.)

[scottchen, 2017-05-01 19:33:54]

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

[commit-bot: I haz the power, 2017-05-01 19:34:31]

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, 2017-05-01 19:49:11]

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

[commit-bot: I haz the power, 2017-05-01 19:49:15]

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

[scottchen, 2017-05-02 22:32:41]

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

[commit-bot: I haz the power, 2017-05-02 22:33:06]

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, 2017-05-02 23:33:38]

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

[commit-bot: I haz the power, 2017-05-02 23:33:39]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-05-03 00:17:37]

On 2017/05/01 19:25:07, scottchen wrote:
> I've updated the CL based on your comments, michaelpg@.
> 
> I also had to add a couple slightly unfortunate additions, since I was
noticing
> some edge-case kinks. PTAL.

Yeah... as we discussed in chat, by no fault of your own, these additional edge
cases might take this file past the complexity we want to allow. I made a couple
suggestions, and the changes here don't *seem* incorrect, so I'll leave it up to
you to discuss with the team whether the extra complexity is worth it (how much
faster does the subpage show up?).

It's also dependent on how much this code is going to change. If there are
serious plans to change the section/subpage visuals, you'll be refactoring this
anyway, so a few extra lines won't matter in the long run.

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

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:152:
this.scroller.scrollTop = 0;
Something that may help for this whole function, if you're up for it: any code
path that does NOT set |promise| can just early return. If we do that, and we
check for that path first, it may simplify the logic.

so for example, this block would be:

if (expandedSection) {
  if (currentRoute.isSubpage() && expandedSection == currentSection) {
    // Keep the section expanded, but scroll to top while sliding to another
subpage.
    this.scroller.scrollTop = 0;
    return;
  }
  promise = this.collapseSection_(expandedSection);
}

and from there, maybe some additional ways to get clarity become obvious (or
not).

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:164:
this.tagName == 'SETTINGS-BASIC-PAGE' &&
We can remove this line, since there's only one MainPageBehavior visible at a
time now (Advanced used to be a MainPageBehavior itself)

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:170: //
Hide the container again while Advanecd Page template is being loaded.
Advanced

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:171:
this.shouldHideContainer = true;
On 2017/05/01 19:25:07, scottchen wrote:
> I had to add this, since the basic page was still briefly showing up while the
> advanced page was being loaded.

Add a TODO here: Avoid recursion by checking if the Advanced template has been
stamped yet.

(that may help you deal with shouldHideContainer more cleanly)

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:180:
this.shouldHideContainer = false;
On 2017/05/01 19:25:07, scottchen wrote:
> I had to add this, since the basic page was still briefly showing up while the
> advanced page was being loaded.

I'm hoping we can factor that out the same way as my comment above, after this
CL lands.

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:233:
this.fire('resize');
On 2017/05/01 19:25:07, scottchen wrote:
> I had to add this, otherwise iron-lists inside of sub-pages would not load
> correctly (since the subpage did not have a size when iron-list attached.)

we do this in a few other places, probably doesn't need the TODO.

Does firing 'iron-resize' instead work? (rather than fake a document resize
event)

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:277: /**
@private */
annotate section

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

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.js:102:
immediateExpand: function(container) {
annotate container

[scottchen, 2017-05-03 15:59:17]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, dpapad@chromium.org

[scottchen, 2017-05-03 15:59:18]

Before I jump into refactoring, I just want to make sure that we're up for
landing this afterwards.

@dbeam @dpapad:
Thoughts on michaelpg's latest comments on whether or not we'd want to maintain
the complexity? Would it be acceptable once the michaelpg's refactoring
suggestions are addressed?

[dpapad, 2017-05-03 17:24:27]

On 2017/05/03 at 15:59:18, scottchen wrote:
> Before I jump into refactoring, I just want to make sure that we're up for
landing this afterwards.

If I am reading Michael's suggestions correctly I would say that is more of a
cleanup and less of a refactoring.

This part of the codebase has become overly complex over time (even before this
CL), and if we are going to invest time into a large scale refactoring, I
suggest writing a design doc first with clear requirements, for example
 - support scrolling to section,
 - support preserving scroll position when exiting subpage
 - Support showing main page on the left and subpage on the right on large
monitors (or showing top-level cards side by side?)
 - Perhaps remove usage of neon-animation

I had mentioned in the past the idea of a singleton "view switcher" and of a
"view" where subpages don't need to be children of their parent pages in the DOM
(the view switcher exposes an API to show a view, hide a view, transition
between views etc). This would make the implementation of
the animations much easier IMO (no more behaviors, no more About + Basic
inheriting same polluted logic), I have not proved that, but I have used similar
concepts of a "view switcher" in a few occasions in the past and they did make
things easier.

> 
> @dbeam @dpapad:
> Thoughts on michaelpg's latest comments on whether or not we'd want to
maintain the complexity? Would it be acceptable once the michaelpg's refactoring
suggestions are addressed?

Just re-iterating my point above, I think we don't want to maintain the
complexity of the existing code (even without this CL), since it is already more
complex than we are comfortable with (speaking for at least for myself, and
probably also for @tommycli with whom I have discussed this part of the codebase
in the past). This CL does not add much more complexity, so perhaps is fine if
the user-visible benefits are worth it (faster navigation). But if we endup
tweaking this file in the future, I highly recommend and large scale rewriting.

[dpapad, 2017-05-03 17:47:20]

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

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:164:
this.tagName == 'SETTINGS-BASIC-PAGE' &&
On 2017/05/03 at 00:17:36, michaelpg wrote:
> We can remove this line, since there's only one MainPageBehavior visible at a
time now (Advanced used to be a MainPageBehavior itself)

I doubt this can be removed. The "About" page, even though not visible,
implements this behavior too, which complicates things. The tagName check here
is a clear example of how subclass logic has crippled into the superclass
(behavior) and I believe it was added for a good reason (it can't just be
removed without breaking anything).

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

https://codereview.chromium.org/2825203003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:35:
shouldHideContainer: {type: Boolean, value: false, notify: true},
Let's add a trailing comma after "notify: true" which forces clang-format to
allow one element per line.

[michaelpg, 2017-05-03 21:53:37]

On 2017/05/03 17:24:27, dpapad wrote:
> On 2017/05/03 at 15:59:18, scottchen wrote:
> > Before I jump into refactoring, I just want to make sure that we're up for
> landing this afterwards.
> 
> If I am reading Michael's suggestions correctly I would say that is more of a
> cleanup and less of a refactoring.
> 
> This part of the codebase has become overly complex over time (even before
this
> CL), and if we are going to invest time into a large scale refactoring, I
> suggest writing a design doc first with clear requirements, for example
>  - support scrolling to section,
>  - support preserving scroll position when exiting subpage
>  - Support showing main page on the left and subpage on the right on large
> monitors (or showing top-level cards side by side?)
>  - Perhaps remove usage of neon-animation

Good idea. The current code grew organically every time a new feature was
required, and the design is really muddled now.

> 
> I had mentioned in the past the idea of a singleton "view switcher" and of a
> "view" where subpages don't need to be children of their parent pages in the
DOM
> (the view switcher exposes an API to show a view, hide a view, transition
> between views etc).

Does this break any Polymer data binding magic?

> This would make the implementation of
> the animations much easier IMO (no more behaviors, no more About + Basic
> inheriting same polluted logic), I have not proved that, but I have used
similar
> concepts of a "view switcher" in a few occasions in the past and they did make
> things easier.
> 
> > 
> > @dbeam @dpapad:
> > Thoughts on michaelpg's latest comments on whether or not we'd want to
> maintain the complexity? Would it be acceptable once the michaelpg's
refactoring
> suggestions are addressed?
> 
> Just re-iterating my point above, I think we don't want to maintain the
> complexity of the existing code (even without this CL), since it is already
more
> complex than we are comfortable with (speaking for at least for myself, and
> probably also for @tommycli with whom I have discussed this part of the
codebase
> in the past). This CL does not add much more complexity, so perhaps is fine if
> the user-visible benefits are worth it (faster navigation). But if we endup
> tweaking this file in the future, I highly recommend and large scale
rewriting.

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

https://codereview.chromium.org/2825203003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:164:
this.tagName == 'SETTINGS-BASIC-PAGE' &&
On 2017/05/03 17:47:20, dpapad wrote:
> On 2017/05/03 at 00:17:36, michaelpg wrote:
> > We can remove this line, since there's only one MainPageBehavior visible at
a
> time now (Advanced used to be a MainPageBehavior itself)
> 
> I doubt this can be removed. The "About" page, even though not visible,
> implements this behavior too, which complicates things. The tagName check here
> is a clear example of how subclass logic has crippled into the superclass
> (behavior) and I believe it was added for a good reason (it can't just be
> removed without breaking anything).

you're correct, thanks!

[dpapad, 2017-05-03 22:07:47]

> > I had mentioned in the past the idea of a singleton "view switcher" and of a
> > "view" where subpages don't need to be children of their parent pages in the
DOM
> > (the view switcher exposes an API to show a view, hide a view, transition
> > between views etc).
> 
> Does this break any Polymer data binding magic?

Yes. It would also break how search works as of today (which leverages the
parent-child DOM relationships). As said,
this would be a mid to large scale refactoring, which would require tweaking
various things, and would need to be
fleshed out more.

[Dan Beam, 2017-05-03 22:36:04]

when I imagined this CL's content, it was a bunch of animation times being set
to 0.  why can't we follow our imagination?

[scottchen, 2017-05-03 23:10:57]

On 2017/05/03 22:36:04, Dan Beam wrote:
> when I imagined this CL's content, it was a bunch of animation times being set
> to 0.  why can't we follow our imagination?

dbeam@ to answer your question (though I *think* you were half-joking), turning
the animation times to 0 is actually the first thing I tried - the really
complex additional stuff is then added to address the fact that basic-page shows
for a split second...


So I think the consensus sounds like: we're okay to land this IF we think the
user benefit is worth maintaining some additional code in the complex system.

To reach a decision on "clean-up + land" OR "hold off + remake view manager
later", let's vote yes/no for whether you think the user value is worth?

[michaelpg, 2017-05-03 23:36:37]

On 2017/05/03 23:10:57, scottchen wrote:
> On 2017/05/03 22:36:04, Dan Beam wrote:
> > when I imagined this CL's content, it was a bunch of animation times being
set
> > to 0.  why can't we follow our imagination?
> 
> dbeam@ to answer your question (though I *think* you were half-joking),
turning
> the animation times to 0 is actually the first thing I tried - the really
> complex additional stuff is then added to address the fact that basic-page
shows
> for a split second...
> 
> 
> So I think the consensus sounds like: we're okay to land this IF we think the
> user benefit is worth maintaining some additional code in the complex system.
> 
> To reach a decision on "clean-up + land" OR "hold off + remake view manager
> later", let's vote yes/no for whether you think the user value is worth?

could we have that discussion on the bug? Has bettes or tbuckley been consulted?
crbug.com/700547

[Dan Beam, 2017-05-04 02:11:01]

On 2017/05/03 23:10:57, scottchen wrote:
> On 2017/05/03 22:36:04, Dan Beam wrote:
> > when I imagined this CL's content, it was a bunch of animation times being
set
> > to 0.  why can't we follow our imagination?
> 
> dbeam@ to answer your question (though I *think* you were half-joking),
turning
> the animation times to 0 is actually the first thing I tried - the really
> complex additional stuff is then added to address the fact that basic-page
shows
> for a split second...
> 
> 
> So I think the consensus sounds like: we're okay to land this IF we think the
> user benefit is worth maintaining some additional code in the complex system.

why can't we just land a simpler patch that turns the times to 0 first?  it
still makes it better, just not perfect, right?

> 
> To reach a decision on "clean-up + land" OR "hold off + remake view manager
> later", let's vote yes/no for whether you think the user value is worth?

[scottchen, 2017-05-04 20:34:49]

> why can't we just land a simpler patch that turns the times to 0 first?  it
> still makes it better, just not perfect, right?

There was a discussion (along with a screen capture of what you described)
earlier in patch#2 about this: 
See comments after #93:
https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...

I think having basic-page briefly flash in before immediately jumping to the
subpage looks even more broken than having the animation, and I think Michael
was agreeing with that in his comment (Demetrios as well, in person).

[michaelpg, 2017-05-05 20:40:34]

On 2017/05/04 20:34:49, scottchen wrote:
> > why can't we just land a simpler patch that turns the times to 0 first?  it
> > still makes it better, just not perfect, right?
> 
> There was a discussion (along with a screen capture of what you described)
> earlier in patch#2 about this: 
> See comments after #93:
>
https://codereview.chromium.org/2825203003/diff/20001/chrome/browser/resource...
> 
> I think having basic-page briefly flash in before immediately jumping to the
> subpage looks even more broken than having the animation, and I think Michael
> was agreeing with that in his comment (Demetrios as well, in person).

We could conceivably set settings.animation.Timing.DURATION to 0, then set it
back when the animation completes. Quick and dirty proof of concept:
https://codereview.chromium.org/2865753002

[Dan Beam, 2017-05-23 16:38:03]

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

[Dan Beam, 2017-05-23 16:38:04]

michaelpg@'s a better reviewer for this stuff

[scottchen, 2017-05-23 23:55:22]

On 2017/05/23 16:38:04, Dan Beam wrote:
> michaelpg@'s a better reviewer for this stuff

michaelpg@ I just uploaded a new patch based on the discussion in your
proof-of-concept CL. I changed the show/hide mechanism to depend on events
instead of passing the boolean down, which helped cut down the number of lines
changed and also feels more decoupled.

PTAL.

[scottchen, 2017-05-23 23:55:34]

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

[commit-bot: I haz the power, 2017-05-23 23:56:53]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-24 20:17:33]

As dbeam@ said, Michael is a better reviewer for this. Moving myself to the cc
list.

[dpapad, 2017-05-24 20:17:45]

dpapad@chromium.org changed reviewers:
- dpapad@chromium.org

[michaelpg, 2017-05-24 21:29:40]

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

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:98:
this.fire('hide-container');
Can we limit this event to the !oldRoute case? In cases where oldRoute exists
but this.scrollHeight == 0, which might happen when navigating to an Advanced
sub-page IIRC, I don't think we want to hide the container.

So I'm proposing logic that might work like this:

if (!oldRoute || this.scrollHeight == 0) {
  if (!oldRoute) fire hide-container;
  setTimeout(
    if (!oldRoute) fire 'show-container';
    // call tryTransition(immediate = !oldRoute);
  )
} else {
  // call tryTransition;
}

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:157:
this.expandSectionImmediate_(currentSection);
maybe we should [only?] fire show-container after this?

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:169: //
Hide the container again while Advanecd Page template is being loaded.
Advanced

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:170:
this.fire('hide-container');
does this work under both edge cases?

1. immediately loading some advanced subpage URL
2. loading the basic page, then navigateTo an advanced subpage URL

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:179:
this.fire('show-container');
Do we need this both here *and* in the setTimeout in currentRouteChanged?

[scottchen, 2017-05-25 19:28:30]

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

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:98:
this.fire('hide-container');
On 2017/05/24 21:29:40, michaelpg wrote:
> Can we limit this event to the !oldRoute case? In cases where oldRoute exists
> but this.scrollHeight == 0, which might happen when navigating to an Advanced
> sub-page IIRC, I don't think we want to hide the container.
> 
> So I'm proposing logic that might work like this:
> 
> if (!oldRoute || this.scrollHeight == 0) {
>   if (!oldRoute) fire hide-container;
>   setTimeout(
>     if (!oldRoute) fire 'show-container';
>     // call tryTransition(immediate = !oldRoute);
>   )
> } else {
>   // call tryTransition;
> }
> 

I think at this point it's probably easier to separate !oldRoute and
scrollHeight == 0 logic because they're pretty different.

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:157:
this.expandSectionImmediate_(currentSection);
On 2017/05/24 21:29:40, michaelpg wrote:
> maybe we should [only?] fire show-container after this?

Acknowledged. The new logic in currentRouteChanged should be equivalent to what
you suggested here.

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:169: //
Hide the container again while Advanecd Page template is being loaded.
On 2017/05/24 21:29:40, michaelpg wrote:
> Advanced

Done.

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:170:
this.fire('hide-container');
On 2017/05/24 21:29:40, michaelpg wrote:
> does this work under both edge cases?
> 
> 1. immediately loading some advanced subpage URL
> 2. loading the basic page, then navigateTo an advanced subpage URL

I tried:
1. going to basic page -> click advanced -> click autofill to go to subpage,
everything stays visible
2. refresh on the autofill subpage, page is correctly hidden until subpage
loads.

So I think in both cases you mentioned it works correctly.

https://codereview.chromium.org/2825203003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:179:
this.fire('show-container');
On 2017/05/24 21:29:40, michaelpg wrote:
> Do we need this both here *and* in the setTimeout in currentRouteChanged?

This is to re-show the container after hiding it on line 170.

[michaelpg, 2017-05-25 22:39:58]

lgtm

[scottchen, 2017-05-25 23:09:53]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-05-25 23:11:14]

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-05-26 00:01:18]

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

[commit-bot: I haz the power, 2017-05-26 00:01:19]

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_...)

[scottchen, 2017-05-26 00:57:12]

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

[commit-bot: I haz the power, 2017-05-26 00:58:49]

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, 2017-05-26 01:50:14]

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

[commit-bot: I haz the power, 2017-05-26 01:50:16]

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_...)

[scottchen, 2017-05-31 00:26:09]

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

[commit-bot: I haz the power, 2017-05-31 00:26:59]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-05-31 00:36:35]

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

[scottchen, 2017-05-31 00:36:36]

Worked with dpapad to find out why the test was failing. 

Summary: 
The existing test for page visibility after search wasn't actually working as
intended. 

This was happening:
- assertAdvancedVisibilityAfterSearch() was searching then clearing, and testing
#basicPage's visibility (making the assumption that route is changed to
basicPage).

- the route was actually not changed to basicPage. Instead, it stayed on the
SITE_SETTINGS route the whole time.

- the test was coincidentally passing, just because #basicPage was visible while
the subpage is animating, and the test didn't wait.

- when this CL changed the subpage transition to immediate, #basicPage isn't
accidentally visible anymore, thus revealing this problem.

I adjusted the test so that it explicitly mimic how clearing search navigates to
the basic page, instead of assuming. 

michaelpg@ or dpapad@ PTAL at the updated test and reLG, thanks!

[dpapad, 2017-05-31 00:40:42]

Test changes LGTM with nits.

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_main_test.js (right):

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:196: * @param {string}
Expected 'display' value for the basic page.
Obsolete parameter.

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:209:
settings.Route.BASIC);  // Imitate clear search.
// Imitate clear search.
settings.navigateTo(settings.Route.BASIC);

[scottchen, 2017-05-31 18:25:28]

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_main_test.js (right):

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:196: * @param {string}
Expected 'display' value for the basic page.
On 2017/05/31 00:40:42, dpapad wrote:
> Obsolete parameter.

Done.

https://codereview.chromium.org/2825203003/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_main_test.js:209:
settings.Route.BASIC);  // Imitate clear search.
On 2017/05/31 00:40:42, dpapad wrote:
> // Imitate clear search.
> settings.navigateTo(settings.Route.BASIC);

Done.

[scottchen, 2017-05-31 18:25:35]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-05-31 18:25:35]

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

[commit-bot: I haz the power, 2017-05-31 18:26:23]

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-05-31 19:24:05]

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

[commit-bot: I haz the power, 2017-05-31 19:24:07]

Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottchen, 2017-05-31 21:29:08]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-05-31 21:30:17]

CQ is trying da patch.

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

[scottchen, 2017-05-31 21:53:01]

Description was changed from

==========
Experiment to remove subpage animation when landing directly on it.

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

to

==========
Experiment to remove subpage animation when landing directly on it.

BUG=700547, 722790
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-05-31 22:21:35]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1496266148610930,
"parent_rev": "4c605f53266369c61044568d8e2dcbd77bc3a800", "commit_rev":
"d84ddac6a6b61604f0057ccb8046144287f795ec"}

[commit-bot: I haz the power, 2017-05-31 22:21:52]

Description was changed from

==========
Experiment to remove subpage animation when landing directly on it.

BUG=700547, 722790
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Experiment to remove subpage animation when landing directly on it.

BUG=700547, 722790
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-05-31 22:21:53]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/d84ddac6a6b61604f0057ccb8046...