test instant subpage load with animation duration 0

test instant subpage load with animation duration 0

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

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

Description was changed from

==========
test instant subpage load with animation duration 0

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

to

==========
test instant subpage load with animation duration 0

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

[michaelpg, 2017-05-05 20:42:45]

https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:111:
document.body.querySelector('settings-ui::shadow #container').style.visibility =
'hidden';
using visibility: hidden instead of display: none so the section still has a
size, otherwise various errors occur because things have no size and
tryTransitionToSection enters an infinite loop

[scottchen, 2017-05-22 17:36:45]

On 2017/05/05 20:42:45, michaelpg wrote:
>
https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> (right):
> 
>
https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/settings_page/main_page_behavior.js:111:
> document.body.querySelector('settings-ui::shadow #container').style.visibility
=
> 'hidden';
> using visibility: hidden instead of display: none so the section still has a
> size, otherwise various errors occur because things have no size and
> tryTransitionToSection enters an infinite loop

This is pretty much what I tried initially before making the immediateExpand()
function, but I thought messing with the global animation duration config is
kind of hacky. Would this approach be considered more landing-friendly than my
CL because this has less code change?

[scottchen, 2017-05-22 22:05:43]

On 2017/05/22 17:36:45, scottchen wrote:
> On 2017/05/05 20:42:45, michaelpg wrote:
> >
>
https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
> > File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
> > chrome/browser/resources/settings/settings_page/main_page_behavior.js:111:
> > document.body.querySelector('settings-ui::shadow
#container').style.visibility
> =
> > 'hidden';
> > using visibility: hidden instead of display: none so the section still has a
> > size, otherwise various errors occur because things have no size and
> > tryTransitionToSection enters an infinite loop
> 
> This is pretty much what I tried initially before making the immediateExpand()
> function, but I thought messing with the global animation duration config is
> kind of hacky. Would this approach be considered more landing-friendly than my
> CL because this has less code change?

Also, given your previous comment that we don't wanna use
querySelector(":shadow"), we'd still have to add most of the changes I have in
my CL, so we end up just trading the immediateExpand function for the
animation.DURATION = 0 code.

[michaelpg, 2017-05-22 22:10:51]

https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2865753002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:111:
document.body.querySelector('settings-ui::shadow #container').style.visibility =
'hidden';
On 2017/05/05 20:42:44, michaelpg wrote:
> using visibility: hidden instead of display: none so the section still has a
> size, otherwise various errors occur because things have no size and
> tryTransitionToSection enters an infinite loop

(Copying scottchen's response to this thread:)


> This is pretty much what I tried initially before making the immediateExpand()
> function, but I thought messing with the global animation duration config is
> kind of hacky. Would this approach be considered more landing-friendly than my
> CL because this has less code change?

I'm not sure. The code is already too complicated, because we keep adding
conditions that weren't planned for. But you're right that setting the duration
to 0 globally is hacky (and possibly risky if for some reason we don't set it
back).

I wonder if a compromise would be to have the DURATION always start at 0, and
only set DURATION to the normal value upon receiving some event to tell us that
the important stuff on the page has finished loading?

We might still have to fiddle with showing/hiding some things, but maybe
changing the start conditions instead of trying a bunch of async stuff would be
simpler.

> Also, given your previous comment that we don't wanna use
> querySelector(":shadow"), we'd still have to add most of the changes I have in
> my CL, so we end up just trading the immediateExpand function for the
> animation.DURATION = 0 code.

Can we simplify your CL with a one-off event we fire that causes settings-ui to
show/hide the container instead of data binding?