Settings page sliding transition

Schedule sliding transition after height change

On the BrowserOptions page, sections that are supposed to hide with an upward sliding transition instead hide instantly.

This is apparently because the style recalc that adds the CSS transition also sets the new section height (i.e., the first style recalc usually doesn't happen before the setTimeout is triggered in animatedSectionHeightChange_). To work around this, force a recalc before setting the new height.

BUG=394053
R=dbeam@chromium.org

Committed: https://crrev.com/fdda19490270c1b6d35b745b5ef16cc724bb7643
Cr-Commit-Position: refs/heads/master@{#294285}

[Dan Beam, 2014-07-15 23:04:15]

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:675:
window.requestAnimationFrame(function() {
please handle the case where a user toggles the checkbox fast enough that the
code inside rAF() doesn't have time to run yet (like the old code)

[michaelpg, 2014-07-15 23:18:32]

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:675:
window.requestAnimationFrame(function() {
On 2014/07/15 23:04:15, Dan Beam wrote:
> please handle the case where a user toggles the checkbox fast enough that the
> code inside rAF() doesn't have time to run yet (like the old code)

will do. Does this make sense to you otherwise? I don't grok the renderer + DOM
well enough to prove that this works or that the old way doesn't work, but it
works for me empirically.

[Dan Beam, 2014-08-21 20:37:49]

what's the deal on this?

[michaelpg, 2014-08-21 20:52:48]

On 2014/08/21 20:37:49, Dan Beam wrote:
> what's the deal on this?

I tried a couple of things but couldn't find a bug-free implementation so it's
been on the back burner till I could rethink it. I can take a pass at it today
actually.

[michaelpg, 2014-08-22 22:17:58]

This addresses the original bug.

I don't know why, but when animatedSectionHeightChange_ is called as a
setTimeout callback, the hiding animation doesn't play:

http://jsfiddle.net/ycuzrzne/

It seems like the preference changed event handler functions the same way.

Using requestAnimationFrame instead of setTimeout in the fiddle works, and using
rAF down the call stack after the event handler for the preference change also
makes the hiding animation work in the Settings page.

I'd like to leave a comment explaining why rAF is necessary (and only for the
hiding transition) but I haven't figured out why yet, something something DOM
layout non-deterministic mumble mumble.

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:675:
window.requestAnimationFrame(function() {
On 2014/07/15 23:18:32, michaelpg wrote:
> On 2014/07/15 23:04:15, Dan Beam wrote:
> > please handle the case where a user toggles the checkbox fast enough that
the
> > code inside rAF() doesn't have time to run yet (like the old code)
> 
> will do. Does this make sense to you otherwise? I don't grok the renderer +
DOM
> well enough to prove that this works or that the old way doesn't work, but it
> works for me empirically.

The current implementation has another bug. If you deselect "Show home button",
and then select it right before the home page section finishes its upward slide,
the home page section isn't shown. (If you then deselect "Show home button"
again, the section will suddenly appear at full height and slide back up.)

Here's how this happens:
* uncheck the checkbox:
* * set style.height to container.offsetHeight (15px)
* * schedule the timeout
* timeout fires: set style.height to 0
* animation is ongoing...
* check the checkbox
* * set style.height to 0
* * schedule the timeout
* animation ends! section.hidden = true;
* timeout fires: set style.height to container.offsetHeight (0, because the
section is hidden -- uh-oh!)


I also don't like that we dispatch a fake webkitTransitionEnd event if the
section is currently animating, because that has the jarring effect of popping
the section all the way in or out before animating it again.

[Dan Beam, 2014-09-05 00:16:48]

On 2014/08/22 22:17:58, michaelpg wrote:
> This addresses the original bug.
> 
> I don't know why, but when animatedSectionHeightChange_ is called as a
> setTimeout callback, the hiding animation doesn't play:
> 
> http://jsfiddle.net/ycuzrzne/
> 
> It seems like the preference changed event handler functions the same way.
> 
> Using requestAnimationFrame instead of setTimeout in the fiddle works, and
using
> rAF down the call stack after the event handler for the preference change also
> makes the hiding animation work in the Settings page.
> 
> I'd like to leave a comment explaining why rAF is necessary (and only for the
> hiding transition) but I haven't figured out why yet, something something DOM
> layout non-deterministic mumble mumble.

re-publish when you figure out why

> 
>
https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://codereview.chromium.org/393943003/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/browser_options.js:675:
> window.requestAnimationFrame(function() {
> On 2014/07/15 23:18:32, michaelpg wrote:
> > On 2014/07/15 23:04:15, Dan Beam wrote:
> > > please handle the case where a user toggles the checkbox fast enough that
> the
> > > code inside rAF() doesn't have time to run yet (like the old code)
> > 
> > will do. Does this make sense to you otherwise? I don't grok the renderer +
> DOM
> > well enough to prove that this works or that the old way doesn't work, but
it
> > works for me empirically.
> 
> The current implementation has another bug. If you deselect "Show home
button",
> and then select it right before the home page section finishes its upward
slide,
> the home page section isn't shown. (If you then deselect "Show home button"
> again, the section will suddenly appear at full height and slide back up.)
> 
> Here's how this happens:
> * uncheck the checkbox:
> * * set style.height to container.offsetHeight (15px)
> * * schedule the timeout
> * timeout fires: set style.height to 0
> * animation is ongoing...
> * check the checkbox
> * * set style.height to 0
> * * schedule the timeout
> * animation ends! section.hidden = true;
> * timeout fires: set style.height to container.offsetHeight (0, because the
> section is hidden -- uh-oh!)
> 
> 
> I also don't like that we dispatch a fake webkitTransitionEnd event if the
> section is currently animating, because that has the jarring effect of popping
> the section all the way in or out before animating it again.

[michaelpg, 2014-09-10 22:03:19]

I started a discussion with a simplified example at https://crbug.com/411103.

timloh: "Transitions only fire if we've done a style recalc on both sides of the
transition"
dstockwell: "Style recalc is lazy. In the cases where it's not working as you
expect it's likely that the style changes are taking place simultaneously -- the
transition property is getting set in the recalc that changes the height from
'auto' to '0'."

My best guess is that adding requestAnimationFrame before changing the style
works because it causes the height change to be set before the next style
recalc, so the styles are set separately and the transition fires. But I'm not
100% on that. Manually forcing the style recalc may look more hacky, but I think
is the more correct solution.

dstockwell suggests using Element.animate instead of CSS transitions, but that's
more useful in hiding than in showing (we can't set a smooth transition from a
height of 0 to 'auto').

[Dan Beam, 2014-09-10 22:56:35]

lgtm


it boggles my mind that

  something.onclick = animate;

behaves *more* reliably than

  something.onclick = function() { setTimeout(animate); };


unfortunately, this is par for the course when it comes to transition timing (in
my experience) :(.

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:748: // Force a style recalc
before starting the animation.
can you please add a 

  /** @suppress {uselessCode} */
  section.offsetHeight;

to make the closure compiler happy?

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:757: });
i don't think you need anything involving setTimeout() if you're flushing styles
with .offsetHeight.  you can probably remove anything to do with
|this.sectionHeightChangeTimeout_|.

[michaelpg, 2014-09-10 23:19:52]

Thanks.

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:748: // Force a style recalc
before starting the animation.
On 2014/09/10 22:56:34, Dan Beam wrote:
> can you please add a 
> 
>   /** @suppress {uselessCode} */
>   section.offsetHeight;
> 
> to make the closure compiler happy?

Done.

https://codereview.chromium.org/393943003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:757: });
On 2014/09/10 22:56:34, Dan Beam wrote:
> i don't think you need anything involving setTimeout() if you're flushing
styles
> with .offsetHeight.  you can probably remove anything to do with
> |this.sectionHeightChangeTimeout_|.

Done.

[michaelpg, 2014-09-10 23:19:57]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:49:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/393943003/60001

[commit-bot: I haz the power, 2014-09-11 01:37:32]

Committed patchset #4 (id:60001) as 36feab56fbd0214e01031f9548240b20c374656e

[commit-bot: I haz the power, 2014-09-11 01:44:15]

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