Add an animated transition/slide when flyout sidebars appear and hide

RESOLVED FIXED in Firefox 22

Status

Firefox for Metro
Theme
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mbrubeck, Assigned: rsilveira)

Tracking

({polish})

Trunk
Firefox 22
All
Windows 8.1
polish
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Rodrigo, can you take this bug?  Here are some pointers to the relevant code (you'll notice there is already a "transition" property but no associated "transforms":

http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/flyoutpanel.css

http://hg.mozilla.org/mozilla-central/file/885cde564ff3/browser/metro/theme/browser.css#l558

http://hg.mozilla.org/mozilla-central/file/885cde564ff3/browser/metro/theme/browser.css#l731

http://hg.mozilla.org/mozilla-central/file/885cde564ff3/browser/metro/base/content/browser.xul#l421
Assignee: nobody → rsilveira
Whiteboard: [metro-mvp?]
Created attachment 718108 [details] [diff] [review]
Added a slide in animation for flyout sidebars.
Attachment #718108 - Flags: feedback?(mbrubeck)

Comment 3

4 years ago
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.
(Reporter)

Comment 4

4 years ago
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.)
Attachment #718108 - Flags: feedback?(mbrubeck) → feedback+
Created attachment 718504 [details] [diff] [review]
Addressing feedback comments

Addressing review comments by mbrubeck and fryn.
Attachment #718108 - Attachment is obsolete: true
Attachment #718504 - Flags: review?(mbrubeck)
Attachment #718504 - Flags: review?(fyan)
(Reporter)

Comment 6

4 years ago
Comment on attachment 718504 [details] [diff] [review]
Addressing feedback comments

Looks good, thanks!
Attachment #718504 - Flags: review?(mbrubeck) → review+

Comment 7

4 years ago
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.
Attachment #718504 - Flags: review?(fyan) → review+

Comment 8

4 years ago
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. :)

Updated

4 years ago
Status: NEW → ASSIGNED
Created attachment 718632 [details] [diff] [review]
Using cubic-bezier and duration based on microsoft's winjs animations.

New animation parameters reviewed through IRC.
Attachment #718632 - Flags: review+
Attachment #718632 - Flags: checkin?(fyan)

Comment 10

4 years ago
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
Target Milestone: --- → Firefox 22
Attachment #718632 - Flags: checkin?(fyan)
Attachment #718504 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a0d8482e5532
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.