Last Comment Bug 843742 - Add an animated transition/slide when flyout sidebars appear and hide
: Add an animated transition/slide when flyout sidebars appear and hide
Status: RESOLVED FIXED
: polish
Product: Firefox for Metro
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Windows 8.1
: -- normal (vote)
: Firefox 22
Assigned To: Rodrigo Silveira [:rsilveira] [:rodms]
:
:
Mentors:
Depends on: 817641
Blocks: 807691
  Show dependency treegraph
 
Reported: 2013-02-21 11:53 PST by Matt Brubeck (:mbrubeck)
Modified: 2014-07-24 11:06 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Added a slide in animation for flyout sidebars. (2.74 KB, patch)
2013-02-25 15:04 PST, Rodrigo Silveira [:rsilveira] [:rodms]
mbrubeck: feedback+
Details | Diff | Splinter Review
Addressing feedback comments (2.50 KB, patch)
2013-02-26 10:16 PST, Rodrigo Silveira [:rsilveira] [:rodms]
mbrubeck: review+
fryn: review+
Details | Diff | Splinter Review
Using cubic-bezier and duration based on microsoft's winjs animations. (2.59 KB, patch)
2013-02-26 13:31 PST, Rodrigo Silveira [:rsilveira] [:rodms]
bugzilla: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2013-02-21 11:53:24 PST
We originally wanted to animate flyout panels (like "About" and "Options"), but this caused problems because of bug 817641.  It should be possible now that bug 817641 is fixed.
Comment 2 Rodrigo Silveira [:rsilveira] [:rodms] 2013-02-25 15:04:57 PST
Created attachment 718108 [details] [diff] [review]
Added a slide in animation for flyout sidebars.
Comment 3 Frank Yan (:fryn) 2013-02-25 15:36:07 PST
Comment on attachment 718108 [details] [diff] [review]
Added a slide in animation for flyout sidebars.

Review of attachment 718108 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/flyoutpanel.xml
@@ +52,5 @@
> +        <body>
> +          <![CDATA[
> +            this.removeAttribute("visible");
> +            this.removeEventListener("transitionend", this._onAfterSlideOut);
> +            DialogUI.popPopup(this);

It'd be nice if we could call popPopup() immediately inside hide(), so the user could interact with the rest of the UI without any delay, but the DialogUI isn't built to handle that race condition right now, so putting it here is the safe thing to do for now.

@@ +61,5 @@
>        <method name="hide">
>          <body>
>            <![CDATA[
> +            if (this.isVisible) {
> +              this.addEventListener("transitionend", this._onAfterSlideOut);            

Nit: you added some trailing whitespace here.

::: browser/metro/theme/flyoutpanel.css
@@ +10,5 @@
>    -moz-border-start-style: solid;
>    visibility: collapse;
>    position: fixed;
> +  transition: transform 0.4s ease;
> +  transform: translateX(100%);

You can use percentages for translation transformations?!
This is amazing. I learned something new today.
Comment 4 Matt Brubeck (:mbrubeck) 2013-02-26 09:46:13 PST
Comment on attachment 718108 [details] [diff] [review]
Added a slide in animation for flyout sidebars.

Review of attachment 718108 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/flyoutpanel.xml
@@ +71,5 @@
>  
>        <method name="show">
>          <body>
>            <![CDATA[
> +            if (!this.isVisible) {

Minor request: Could you change this to

  if (this.isVisible)
    return;

and make a similar change in the hide() method above?  We tend to use early returns like this to make it explicit that nothing further down in the function will execute in this case.  (It also has the minor benefit of reducing overall indentation.)
Comment 5 Rodrigo Silveira [:rsilveira] [:rodms] 2013-02-26 10:16:40 PST
Created attachment 718504 [details] [diff] [review]
Addressing feedback comments

Addressing review comments by mbrubeck and fryn.
Comment 6 Matt Brubeck (:mbrubeck) 2013-02-26 10:48:31 PST
Comment on attachment 718504 [details] [diff] [review]
Addressing feedback comments

Looks good, thanks!
Comment 7 Frank Yan (:fryn) 2013-02-26 12:39:37 PST
Comment on attachment 718504 [details] [diff] [review]
Addressing feedback comments

Review of attachment 718504 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/theme/flyoutpanel.css
@@ +9,5 @@
>    background-color: #ffffff;
>    -moz-border-start-style: solid;
>    visibility: collapse;
>    position: fixed;
> +  transition: transform 0.4s ease;

I'd like to revisit our animations later to see how we compare to the native ones in terms of duration and easing.
We don't necessarily have to match the native ones; if the native duration or easing don't work well for us, we can try to pick better ones.

Rodrigo mentioned that Microsoft uses exponential functions for easing, which cubic bezier functions can only approximate, but that's okay.
Comment 8 Frank Yan (:fryn) 2013-02-26 12:58:04 PST
Rodrigo just found that Microsoft uses:
duration: 367ms
timing-function: cubic-bezier(0.1, 0.9, 0.2, 1)

Let's go with that and see how we like it. :)
Comment 9 Rodrigo Silveira [:rsilveira] [:rodms] 2013-02-26 13:31:47 PST
Created attachment 718632 [details] [diff] [review]
Using cubic-bezier and duration based on microsoft's winjs animations.

New animation parameters reviewed through IRC.
Comment 10 Frank Yan (:fryn) 2013-02-26 13:57:23 PST
Your first patch just landed! Yay! \o/ :D
It will be marked RESOLVED FIXED once mozilla-inbound merges in mozilla-central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d8482e5532
Comment 11 Ed Morley [:emorley] 2013-02-27 05:45:14 PST
https://hg.mozilla.org/mozilla-central/rev/a0d8482e5532

Note You need to log in before you can comment on or make changes to this bug.