610545 - Arrowpanel animation

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

[sorry about the blank first comment, details below]

We would like to use css transitions to improve the usability of arrow panels. The objectives are:

1) Make it easier to notice that a panel has appeared
2) Convey to the user that the panel is ephemeral, that you can click outside to close it
3) Convey to the user that even when an arrowpanel is dismissed, you can still bring it back.  Draw a very clear association between the arrowpanel and the anchor that can be used to bring it back

Suggested animations:

When the arrowpanel appears (including being brought back): fade in
When the arrowpanel is acted on: fade out
When a stable arrowpanel like site identity or bookmarks is dismissed: fade out

When a message-based arrowpanel is dismissed through a click outside: fade out while simultaneously scaling towards the anchor

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating since this is important for helping users to realize that they can bring these dialogs back after they have been dismissed (also Frank already showed me this working).

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I love this idea, and would be first in line to look at patch approvals, but I don't think we should block on it.

Comment 4

[Alex Limi (:limi) — Firefox UX Team]

Re-nominating now that we have the concept of soft-blockers, which I think this bug is a prime example of.

It's high-value (I doubt many people will discover that doorhangers can be revisited/retrieved without it, which is a key feature), and seems low-risk if we do a simple CSS transition.

Comment 5

[Sheila Mooney]

Does this need a beta? If so, shouldn't we go ahead and mark it?

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Yeah, this should ideally be tested in a beta.

Comment 7

[:Margaret Leibovic]

Snatching from Frank!

For posterity, IRC comment from Frank:

you can use css3 transitions with -moz-transition-property: -moz-transform, opacity; -moz-transform: scale(0); opacity: 0; and then delay the closing the panel until the transition ends

Comment 8

[Frank Yan (:fryn)]

Thanks much, Margaret!

The above styles should be set on the panel itself. It works magically somehow, which is really cool. I've only tested this approach on OS X. We should test how well it works on Windows and Linux too, so if it doesn't work on either of those platforms, we can just disable the animation those platforms easily.

The hard part is the race condition of reopening a panel while it's still being closed. My intuition is that we'd just set -moz-transform back to scale(1) and opacity back to 1, so the transition immediately starts reversing. If we use a transitionend handler to close the panel after the close animation, we have to make sure that the aforementioned race condition doesn't break it. Everything else should be relatively easy.

If you run into any problems, just let me know. I'm also happy to give feedback on patches :)

Comment 9

[:Margaret Leibovic]

Alex, which panels do we want to animate? Popup notifications, site identity panel, and bookmark star panel? Are there any others?

Comment 10

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #9)
> Alex, which panels do we want to animate? Popup notifications, site identity
> panel, and bookmark star panel? Are there any others?

Password manager is one of the most important ones.

Comment 11

[Frank Yan (:fryn)]

(In reply to comment #10)
> (In reply to comment #9)
> > Alex, which panels do we want to animate? Popup notifications, site identity
> > panel, and bookmark star panel? Are there any others?
> 
> Password manager is one of the most important ones.

That's a subset of popup notifications.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, which panels do we want to animate? Popup notifications, site identity
>panel, and bookmark star panel? Are there any others?

That's everything that comes to mind, can anyone think of another one?

Comment 13

[Peter Henkel [:Terepin]]

Downloading add-on(s), geolocation request.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

all under notifications (plus indexeddb, etc.)

Comment 15

[Justin Dolske [:Dolske]]

Shouldn't the animations just be for _all_ arrowpanels? Are there some that we specifically don't want animated (and why)?

Comment 16

[:Margaret Leibovic]

(In reply to comment #15)
> Shouldn't the animations just be for _all_ arrowpanels? Are there some that we
> specifically don't want animated (and why)?

I think we want the animation to apply to arrow panels that the user can bring back, which describes the ones just mentioned.

I assumed we wouldn't want to apply the animation to the xul widget, since I don't know if all consumers would want that. For example, the animation might seem strange for the invalid form popup.

I've been investigating how we might want to actually do this, and after discussing my ideas with Frank this afternoon, I have some concerns about implementing a fix for this bug this late in the game. One of my concerns is that when clicking outside the popup, the popup hiding is handled by the widget level code, so we would probably have to make some onpopuphiding event listener that canceled the event, performed our animation, then hid the panel itself, and that seems kind of messy/sketchy. Frank mentioned the idea of making a patch to just animate showing the panel, but that can also be tricky if we want the scaling effect to originate from the arrow, not just the corner of the panel. In addition to these issues, I can imagine lots of timing edge cases that may cause problems.

I don't want to be a party pooper, but I don't think it will be easy to write a quick patch for this bug :(

Comment 17

[Frank Yan (:fryn)]

(In reply to comment #16)
> Frank mentioned the idea of making a
> patch to just animate showing the panel, but that can also be tricky if we want
> the scaling effect to originate from the arrow, not just the corner of the
> panel.

For more context, the proof-of-concept demo that I created for this made the scaling effect seem to originate from the anchor (using -moz-transition-origin) by estimation, not actually calculating where the anchor is relative to the panel.

> I don't want to be a party pooper, but I don't think it will be easy to
> write a quick patch for this bug :(

I agree. While I too would love to have this in Firefox 4, we're still having problems with the reliability of "transitionend" in our tab closing animation code, so this may be too risky to introduce this late in the cycle.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

(er updating flag to "-" as per previous comment!)

Comment 20

[Bryan Clark (DevTools PM) [:clarkbw]]

The F1/Sharing Panel would really love to see this land in time for FF6 so our panel has a much clearer association with the share button it opens from and when a person shares that the panel associates with the icon again where share progress is indicated.  Thanks!

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> The F1/Sharing Panel would really love to see this land in time for FF6

This bug lacks a clear scope, it's more like a meta bugs describing various issues with the iframe approach.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> (In reply to comment #20)
> > The F1/Sharing Panel would really love to see this land in time for FF6
> 
> This bug lacks a clear scope, it's more like a meta bugs describing various
> issues with the iframe approach.

Oops, sorry, I thought we were in bug 649144.

Comment 23

[:Margaret Leibovic]

I'm unassigning myself for now because I haven't been working on this recently.

Bryan, this is kind of messy for the reasons described in comment 16. The patch I was playing around with only added the animation to popup notifications, and if we keep adding arrow panels to the product that want this animation, adding animation effects to individual panels is probably not a practical/scalable approach.

However, maybe this will be easier now that CSS animations have landed. If we can do this purely in CSS, perhaps we could just add rules to the toolkit styles to animate an arrow panel if an "animate" attribute (or something like that) is set on the panel.

Comment 24

[Frank Yan (:fryn)]

(In reply to comment #23)
> However, maybe this will be easier now that CSS animations have landed.
I don't think animations will help with this at all, sadly. :(

> perhaps we could just add rules to the toolkit
> styles to animate an arrow panel if an "animate" attribute (or something like
> that) is set on the panel.

Implementing such an attribute in toolkit/ seems to be the best approach.
Let's change components.

Comment 25

[Frank Yan (:fryn)]

CC'ing Neil for input.

Comment 26

[Neil Deakin]

Arrow panels already support fade="fast" and fade="slow". You could just add some additional variations.

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #23)
> Bryan, this is kind of messy for the reasons described in comment 16. The patch
> I was playing around with only added the animation to popup notifications, and
> if we keep adding arrow panels to the product that want this animation, adding
> animation effects to individual panels is probably not a practical/scalable
> approach.

Damn :(  We mostly want the animation back toward the arrow point however scaling the panel size down isn't really a good option for us because we're using a web iframe which would get resized as the panel scales.  

A general fade would be possible but I feel that implementing this in the hidePopup(fade="fast") would probably help us the most here.

Thanks anyway!

(In reply to comment #26)
> Arrow panels already support fade="fast" and fade="slow". You could just add
> some additional variations.

These didn't work for us initially and I'll have to look up why.  I remember it had something to do with not interacting very well with the hide/open Popup functions that keep panel.state.  Again I don't remember exactly but I believe we had to remove the fade attribute after the popup was hidden, otherwise it would fade immediately on open.  Like above we were looking for a fade as part of the hidePopup function, not something automatic after a timeout.

Comment 28

[:Margaret Leibovic]

Unfortunately, fade isn't what we want for these panels. We want to make panels animate in/out, but fade just makes the panel fade away a certain amount of time after it is opened.

I'm working on a patch that lets you add an animate attribute to the panel to get a combined fade/scale effect. I got the main idea of the patch to work, but I still need to make sure it doesn't interfere with the fade logic, and I need to make it handle cases where the arrow isn't in the top-left. I will post something soon!

Comment 29

[:Margaret Leibovic]

Attachment: WIP patch

I changed the way fade works to set the opacity in CSS, so that it's clearer to understand what's going on with the different opacity transitions, since I introduced a new one with this animation.

I don't think I used the correct logic to determine the -moz-transform-origin values. It seems to work when the arrows are on the top/bottom, except for some reason the arrow disappears when the panel is anchored on the bottom left :/

Also, this logic just makes the the transform origin at the corner of the panel closet to the arrow, but maybe we could try to use pixel length values to get the origin right at the point of the arrow.

Comment 30

[:Margaret Leibovic]

Attachment: WIP patch

Doh, I messed up the fadeout selector. This should work.

Comment 31

[Alex Faaborg [:faaborg] (Firefox UX)]

>Unfortunately, fade isn't what we want for these panels. We want to make panels
>animate in/out, but fade just makes the panel fade away a certain amount of
>time after it is opened.

We may want to do a mixture of fade versus animate depending on the context.  For instance clicking outside to close (when the exact panel can be brought back), versus clicking a share button (where the panel is being dismissed from inside of the panel).

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

Also, this is really something that we're going to want to play with a bit in the nightly builds, since animation lives on an unfortunately fine line between cool and useful, and annoyingly over the top (I'm looking at you, OS X genie effect).

Comment 33

[Frank Yan (:fryn)]

(In reply to comment #31)
> >Unfortunately, fade isn't what we want for these panels. We want to make panels
> >animate in/out, but fade just makes the panel fade away a certain amount of
> >time after it is opened.
> 
> We may want to do a mixture of fade versus animate depending on the context. 
> For instance clicking outside to close (when the exact panel can be brought
> back), versus clicking a share button (where the panel is being dismissed from
> inside of the panel).

I think what Margaret is that the currently available fade attribute is an automatic timed fade that occurs without user action, which isn't what we want.

Comment 34

[Frank Yan (:fryn)]

*what Margaret meant, even.

Comment 35

[Frank Yan (:fryn)]

Comment on attachment 526349 [details] [diff] [review]
WIP patch

>      <panel id="identity-popup"
>             type="arrow"
> +           fade="fast"

This doesn't work. Clicking on the ID box causes it to fade out immediately.

> +      <handler event="popupshown" phase="target">
> +        if (this.getAttribute("animate")) {
> +          let arrowbox = document.getAnonymousElementByAttribute(this, "anonid", "arrowbox");
> +          let horizOrigin = arrowbox.pack == "start" ? "0" : "100%";
> +
> +          let side = this.getAttribute("side");
> +          let vertOrigin = side == "top" ? "0" : "100%";

This almost works, but since the arrow isn't exactly at a corner, but we probably ought to calculate the actual position of it relative to the panel. I realize implementing that isn't fun. :\

(By the way, left/right/top/bottom are valid origin values https://developer.mozilla.org/En/CSS/-moz-transform-origin , but it doesn't matter here, since we'll need more accurate numbers. /sigh )

> +  -moz-transform: scale(1);

IIRC, these lines aren't needed.

These issues aside, the scale animation is fantastic! Thanks for writing the patch. :)

Comment 36

[:Margaret Leibovic]

(In reply to comment #35)
> >      <panel id="identity-popup"
> >             type="arrow"
> > +           fade="fast"
> 
> This doesn't work. Clicking on the ID box causes it to fade out immediately.

Oops, that should be animate="true", not fade="fast". Probably leftover from when I was experimenting with fade. This is expected behavior with fade!

Comment 37

[Frank Yan (:fryn)]

(In reply to comment #36)
> This is expected behavior with fade!

> var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;

I just read through the code and see this now. I can't think of a case when you'd want a panel to begin fading out immediately upon being opened. Let's consider changing that? Even the documentation states "a short time" delay for both:

> slow: The panels slowly fades away after a short time.
> fast: The panels quickly fades away after a short time.

Either the documentation is wrong, or the code intended for fade="slow" and fade="fast" to have the same fadeDelay with different transition-durations.

Comment 38

[Frank Yan (:fryn)]

By the way, here's the documentation link, in case that's useful:
https://developer.mozilla.org/en/XUL/Attribute/panel.fade

Comment 39

[Neil Deakin]

(In reply to comment #37)
> (In reply to comment #36)
> > This is expected behavior with fade!
> 
> > var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;
> 
> I just read through the code and see this now. I can't think of a case when
> you'd want a panel to begin fading out immediately upon being opened.

This was specified by Alex Faaborg as a wanted feature in the original design.

A small popup that flashes on screen to indicate something then quickly fades away. Chrome had (perhaps still does) something like this to indicate a download is started.

Comment 40

[:Margaret Leibovic]

I'm not actively working on this, so I'm throwing it back in case someone else wants to give it a go.

Comment 41

[bogas04]

Is it in works now? Sorry for the bump.

Comment 42

[:Margaret Leibovic]

(In reply to bogas04 from comment #41)
> Is it in works now? Sorry for the bump.

No, no one is working on this right now, since it turned out to be kind of tricky, and it's not a top priority.

Comment 43

[anthonydam]

(In reply to Margaret Leibovic [:margaret] from comment #42)
> (In reply to bogas04 from comment #41)
> > Is it in works now? Sorry for the bump.
> 
> No, no one is working on this right now, since it turned out to be kind of
> tricky, and it's not a top priority.

It's a P1 priority : https://wiki.mozilla.org/Firefox/Features/UI_animations

Comment 44

[u428464]

This bug should become important with the Australis redesign that uses a lot of arrow panel (menu, downloads, etc.).

Comment 46

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

A little context for the URL that Dão just posted:
Type 4 (pre-selected) is the final design. Click »UX« in the location bar, the downloads button or the menu button (everything else doesn't respond).
See CSS for transformations and timing (anything with the class .type-4 is relevant)

Comment 47

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looking at the animations once again (http://phlsa.github.io/ff-bubble-animation), I think the »slurp« for closing a menu should only occur when the menu is closed without performing an action (i.e. the menu should just disappear if you e.g. press an item in the bookmarks panel).

Comment 48

[Andrei Vaida [:avaida]]

Since Bug 767133 and Bug 767861 are dependencies of this issue, I took the time to verify them using the latest Nightly (Build ID: 20140303030201) and the help of two MDN articles [1] & [2], on the following platforms:
- Windows 8.1 Pro 64-bit [3],
- Windows 7 64-bit [4],
- Windows XP 64-bit [5],
- Mac OS X 10.9.1 [6],
- Mac OS X 10.8.5 [7].

The following panels were verified: pop-up notifications, site identity panel, bookmark star panel, password manager, download panel, geolocation request, mixed content blocker, click-to-play doorhanger.

Potential issues (please confirm/advise):
1. The website identity panel [all mentioned platforms].
* Clicking twice on the identity panel of websites displaying their name in addition to their favicon (e.g. about:support page), or on websites providing Extended Validation Certificates (e.g. https://developer.mozilla.org/, https://login.live.com/) results in the reopening of that panel. 
* On the other hand, clicking twice exactly on the icon and not on the website's name, shows and hides the panel, as expected.

2. The password manager panel [all mentioned platforms].
* The sync-related section of the password manager doorhanger disappears after showing/hiding it multiple times. The same behavior is encountered for the bookmark star panel, after repeateadly bookmarking/unbookmarking websites.

3. The new (hamburger) Firefox menu. [all mentioned platforms]
* Clicking right next to the menu button brings up the menu, but pressing it again reopens the menu instead of closing it.
* I know that the Fx menu might not be related to this bug or have anything to do with this feature, but I thought I should mention it.

4. Two icons in the location bar. [all mentioned platforms]
Access the following URL: https://adalet.gov.tr/ and add it to your exception list. Two icons will be displayed in the location bar, CTP for flash and the mixed content blocker (shield) icon. Clicking once on each of them and then twice on each of them will not make the doorhanger disappear.


[1] http://mzl.la/1eV9SiA
[2] http://mzl.la/1kqUzX5
[3] Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[4] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[5] Mozilla/5.0 (Windows NT 5.2; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[6] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
[7] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 50

[Justin Dolske [:Dolske]]

We'd certainly _like_ this for Australis (see dupe'd bug 940130), if the fix here is safe and well-contained we should at least consider it for uplift to Firefox 29 w/ Australis.

Comment 51

[Neil Deakin]

Attachment: Animate panels, work in progress

This patch implements the animation more or less like the specified url. Still to do:

- The star button, identity popup and downloads popup work quite well. Other popups not so much.
- Perhaps the animation should be opt in for those cases where it doesn't work, or existing uses/addons that don't expect it.
- The skew effect needs to vary based on the direction the popup opens in.
- The closing animation always occurs. To clarify, the animation should only occur when clicking outside the popup and not any of the myriad other ways to close the panel.
- It should be implemented on all platforms but I've only tested on Mac.

Comment 52

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Neil Deakin from comment #51)
> Created attachment 8386918 [details] [diff] [review]
> Animate panels, work in progress

Good progress!

> This patch implements the animation more or less like the specified url.
> Still to do:
> 
> - The star button, identity popup and downloads popup work quite well. Other
> popups not so much.
> - Perhaps the animation should be opt in for those cases where it doesn't
> work, or existing uses/addons that don't expect it.
I think we can make a more informed decision on that once the patch has evolved a little further and we test it with some of the top add-ons.

> - The skew effect needs to vary based on the direction the popup opens in.
> - The closing animation always occurs. To clarify, the animation should only
> occur when clicking outside the popup and not any of the myriad other ways
> to close the panel.
I'm not sure if there's a way to reliably do that, but the close animation should *not* happen if the panel closes after performing an action in it (e.g. clicking a button on the panel).
I think that leaves clicking outside and pressing ESC for when the animation should happen.


As discussed, there's two other issues at the moment:
1) Some of the panels get clipped when the skew is applied
2) The bounce-part of the opening animation seems to be applied only to the contents of the panel, not the bubble itself.

Comment 53

[Neil Deakin]

Attachment: Animated arrow panels, v2

This patch adds:
 - fixes the closing skew effect for popups in all directions. The clipping doesn't affect them any more once the right skews are used.
 - makes the popup opt-in rather than opt-out using animate="closed"
 - enables the animation for the identity, star button and downloads panel
 - tested on Mac and Windows
 - the closing animation now only occurs when the popup is rolled up by clicking outside, losing focus, etc, or by pressing escape

There are three remaining issues:
 - Change the way a transition is detected. dbaron suggests using getComputedStyle but I don't think that's what I want here.
 - The bookmarks star panel handles the escape key manually so the animation doesn't occur.
 - The bounce issue mentioned in comment 52 probably isn't something I can fix.

Comment 54

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Love it!

(In reply to Neil Deakin from comment #53)
>  - The bounce issue mentioned in comment 52 probably isn't something I can
> fix.

This seems to be related to the frame issue. Looking at a screen capture, it looks like the entire bubble gets clipped (http://cl.ly/image/3V2K38120s13 – you see that the rounded corners are cut off).

Comment 55

[Neil Deakin]

Attachment: Animated arrow panels, v3

This patch adds a margin and makes some other minor changes. This isn't perfect and the arrowpanel tests still fail, but any other changes will just be tweaking the margins or the test a bit.

Comment 56

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

How are you planning to progress here? I think it makes little sense to have half the panels use the new animation and the other half using the old one. Should the other panels be implemented in a different bug?

Comment 57

[Neil Deakin]

The current behaviour is an opt-in. I can make it the default behaviour is desired.

I think someone else like mconley or Gijs would have to look at this for the bookmarks menu and the big toolbar popup, since there is some odd sizing functionality that I don't know much about that messes up the animation.

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 8390569 [details] [diff] [review]
Animated arrow panels, v3

This doesn't apply cleanly anymore. Hoping that there were just some innocent changes in the context, I blindly applied the failed hunks, but then I got build errors :(

Comment 59

[Neil Deakin]

Attachment: Animated arrow panels, v3.1

OK, here is an updated patch that builds ok.

Comment 60

[Neil Deakin]

Comment on attachment 8394201 [details] [diff] [review]
Animated arrow panels, v3.1

argh, that's the wrong patch!

Comment 61

[Neil Deakin]

Attachment: Animated arrow panels, v3.2

Comment 62

[Neil Deakin]

Gavin also suggested that we should make this opt-out instead. I will make a follow-up patch to do that so we can try both opt-in and opt-out separately.

Comment 63

[Dão Gottwald [:dao]]

I played around with on Linux and noticed that arrow panels were slightly misaligned, i.e. the arrow didn't point at the right spot on the anchor anymore. This happened for arrow panels with the animation as well as for those without it.

Comment 64

[[e-mail]]

I tried this with a third-party theme. As expected, because the theme didn't know that it needed to define a transition, the popup simply froze open and I had to close the window to get rid of it.

I don't quite understand the attribute twiddling in particular why the cancel value is so important.

Comment 65

[Neil Deakin]

> I tried this with a third-party theme. As expected, because the theme didn't
> know that it needed to define a transition, the popup simply froze open and
> I had to close the window to get rid of it.

Hmm. I think that's why I originally had the code check for the existence of a transition rather than assuming one was present, but dbaron wasn't keen on calling into nsTransitionManager.

The only other alternative is to move the transition into xul.css

> I don't quite understand the attribute twiddling in particular why the
> cancel value is so important.

The desired is have the transition only apply when cancelling the popup by clicking outside or pressing escape and not when clicking on, for example, an ok button in the panel.

I actually reused the animate attribute for both an indicator as to whether the animation should occur and the current state to help the transition.

Comment 66

[Neil Deakin]

Attachment: Animated arrow panels, v4

This version:
 - removes the bounce effect as if affects the margins and causes test failures. This might be something to add back later. This fixes the alignment issue Dao mentioned.
 - moves the transition into xul.css so that other themes won't be as affected. They can turn it off if desired.
 - adds a check to see if the style data has a transition assigned and only applies it in that caes
 - makes the animation opt-out with animate="false" and defaults to true.

Comment 67

[[e-mail]]

Comment on attachment 8395267 [details] [diff] [review]
Animated arrow panels, v4

This works; the only other idea I could come up with was to add a parameter to hidePopup so that callers could choose to explicitly prevent animation.

Comment 68

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looks good where it works!
Since the bounce seems to be unachievable for now, I'd like to adjust the timings of the animation a little. Should this be part of this bug or a separate one?

Did the animation of panels that don't have the new effect yet get faster with this bug?

I'm still not sure what will happen with the remaining panels now. Would you pass the bug over to someone else to apply the animation to them or is that a new bug as well?

Comment 69

[Neil Deakin]

(In reply to Philipp Sackl [:phlsa] from comment #68)
> Looks good where it works!
> Since the bounce seems to be unachievable for now, I'd like to adjust the
> timings of the animation a little. Should this be part of this bug or a
> separate one?
> 

Sure, just list them here.

> Did the animation of panels that don't have the new effect yet get faster
> with this bug?

I don't know.

> I'm still not sure what will happen with the remaining panels now. Would you
> pass the bug over to someone else to apply the animation to them or is that
> a new bug as well?

I could likely handle the bookmarks menu, but the big toolbar panel has some odd sizing problems and I don't know enough about the implementation issues and may not be able to get it to work. I'd suggested that someone else could take a look or at least provide some help.

I think you need to make a call as to whether we should go with the patch as is, or wait until that remaining work can be done.

Comment 70

[Neil Deakin]

I'm hitting an assertion with this patch if you try to open the panel while it is transitioning, so I'm going to post a followup patch which moves the transition handling to before HidePopupCallback.

Comment 71

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

We should definitely wait with landing until the rest of the work (i.e. the remaining panels) is done.
So do I see this right that the way to move forward is to transition this bug to someone else in the next iteration? Neil, do you have someone specific in mind?

Comment 72

[Neil Deakin]

Attachment: Part 2, fix assertion

This followup patch fixes the assertion by moving the transition handling code earlier before the popup is unhooked.

Comment 73

[Neil Deakin]

Attachment: Part 3 - animate bookmarks menu

This adds the animation to the bookmarks menu, although it doesn't seem to work the first time the menu is opened.

I don't like the animation on the menu personally. The delay it causes makes me think I should wait for it to finish before selecting something from the menu and I don't have to do that for other menus.

Comment 74

[:Gavin Sharp [email: [e-mail]]]

(In reply to Philipp Sackl [:phlsa] from comment #71)
> We should definitely wait with landing until the rest of the work (i.e. the
> remaining panels) is done.

I think we can deal with the inconsistency on trunk temporarily, and Neil can file a followup to fix the menupanel animation.

Comment 75

[Neil Deakin]

Comment on attachment 8397221 [details] [diff] [review]
Part 3 - animate bookmarks menu

Any thoughts on this. See comment 73.

Comment 76

[Tim Nguyen :ntim]

Will there be any way to fall back to the old animation ?
The new one seems too much distracting.

Comment 77

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

So here's a diff of some tweaks to the animation: http://pastebin.com/d7d1BCAa
This makes it noticeably faster and also less jarring on larger panels while preserving the general feel.

I'm giving a UI-r+ under the assumption that these changes get implemented.
Neil and Tim, does this address your concerns?

Comment 78

[Neil Deakin]

Attachment: Part 4 - Philipp's patch to improve animation

It is faster, although I'm still unsure. Perhaps we should just check these patches in and see what feedback is received.

Comment 79

[Neil Deakin]

Comment on attachment 8395267 [details] [diff] [review]
Animated arrow panels, v4

Don't think dao's review is needed here.

Comment 80

[Dão Gottwald [:dao]]

Comment on attachment 8395267 [details] [diff] [review]
Animated arrow panels, v4

>       <handler event="popupshowing" phase="target">
>       <![CDATA[
>         this.adjustArrowPosition();
>+        if (this.getAttribute("animate") != "false") {
>+          this.setAttribute("animate", "open");
>+        }
>+
>         // set fading
>         var fade = this.getAttribute("fade");
>         var fadeDelay = (fade == "fast") ? 1 : fade == "slow" ? 4000 : 0;
>         if (fadeDelay) {
>-          this._fadeTimer = setTimeout(function (self) {
>-            self.style.opacity = 0.2;
>-          }, fadeDelay, this);
>+          this._fadeTimer = setTimeout(() => this.hidePopup(), fadeDelay, this);
>         }
>       ]]>

I don't think the fade attribute is used anywhere. Can you remove this stuff?

>       <handler event="popuphiding" phase="target">
>-        clearTimeout(this._fadeTimer);
>-        this.style.removeProperty("opacity");
>-      </handler>
>-      <handler event="transitionend" phase="target">
>-      <![CDATA[
>-        if (event.propertyName == "opacity" &&
>-            event.originalTarget == this) {
>-          this.hidePopup();
>-          this.style.removeProperty("opacity");
>+        let animate = (this.getAttribute("animate") != "false");
>+
>+        if (this._fadeTimer) {
>+          clearTimeout(this._fadeTimer);
>+          if (animate) {
>+            this.setAttribute("animate", "fade");
>+          }
>         }

You don't even seem to make animate="fade" work anywhere in this patch (and you shouldn't, at least I don't see the point).

Comment 81

[Dão Gottwald [:dao]]

Comment on attachment 8395267 [details] [diff] [review]
Animated arrow panels, v4

>--- a/toolkit/themes/osx/global/popup.css
>+++ b/toolkit/themes/osx/global/popup.css
>@@ -20,27 +20,16 @@ menupopup > menu > menupopup {
> 
> panel[titlebar] {
>   -moz-appearance: none; /* to disable rounded corners */
> }
> 
> panel[type="arrow"] {
>   -moz-appearance: none;
>   background: transparent;
>-  transition: opacity 300ms;
>-}
>-
>-.panel-arrowcontainer[panelopen] {
>-  transition-duration: 200ms, 150ms;
>-  transition-property: opacity, transform;
>-  transition-timing-function: ease-out;
>-}
>-
>-.panel-arrowcontainer:not([panelopen]) {
>-  opacity: 0;
> }
> 
> .panel-arrowcontainer:not([panelopen])[side="top"] {
>   transform: translateY(-20px);
> }
> 
> .panel-arrowcontainer:not([panelopen])[side="bottom"] {
>   transform: translateY(20px);

The translateY stuff is dead code now, isn't it?

Comment 82

[Dão Gottwald [:dao]]

Comment on attachment 8398461 [details] [diff] [review]
Part 4 -  Philipp's patch to improve animation

> #BMB_bookmarksPopup {
>   -moz-appearance: none;
>   -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-arrow");
>   background: transparent;
>   border: none;
>-  transform: scale(.1);
>+  transform: scale(.7);

Why are you using scale(.7) here but scale(.8) in xul.css?

> #BMB_bookmarksPopup[arrowposition="after_start"][animate="cancel"],
> #BMB_bookmarksPopup[arrowposition="before_end"][animate="cancel"] {
>-  transform: scale(.1) skew(30deg, 20deg);
>+  transform: scale(.7) skew(10deg, 10deg);
> }

Why are you using scale(.7) here but scale(.6) in xul.css?

Comment 83

[Neil Deakin]

(In reply to Dão Gottwald [:dao] from comment #80)
> I don't think the fade attribute is used anywhere. Can you remove this stuff?

I left support for fade in as it was listed in the documentation.

> You don't even seem to make animate="fade" work anywhere in this patch (and
> you shouldn't, at least I don't see the point).

It still works. animate="fade" makes the panel always transition away rather than only when cancelling. The only thing it doesn't do is add the skew, but I could add a few extra lines to do it.

Comment 84

[Neil Deakin]

Philipp, see comment 82.

Comment 85

[Dão Gottwald [:dao]]

(In reply to Neil Deakin from comment #83)
> (In reply to Dão Gottwald [:dao] from comment #80)
> > I don't think the fade attribute is used anywhere. Can you remove this stuff?
> 
> I left support for fade in as it was listed in the documentation.

We can just update the documentation. I think this was a quick shot that came along with the initial arrow panel implementation, but it was obsoleted long ago by bug 767861, bug 767133 and now again by this one.

Comment 86

[Dão Gottwald [:dao]]

Note that we never used the fade attribute anywhere, not even before bug 767861 and bug 767133.

Comment 87

[Neil Deakin]

(In reply to Dão Gottwald [:dao] from comment #85)
> We can just update the documentation. I think this was a quick shot that
> came along with the initial arrow panel implementation, but it was obsoleted
> long ago by bug 767861, bug 767133 and now again by this one.

I don't see how those bugs make the fade attribute obsolete, but I can remove it in a followup bug. I want to get these patches checked in.

Comment 88

[Neil Deakin]

Checked in all but the bookmarks related patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/25877e8f89c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/efbf80a15a2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0539570fb726

Comment 89

[Ed Morley [:emorley]]

Backed out for browser-chrome failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0539570fb726&jobname=browser-chrome

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f81133c315
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e13eb216fc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c78aa5b01ff

Comment 90

[Dão Gottwald [:dao]]

Unrelated to the test failures: Addressing comment 81 made the panelopen attribute unused, allowing it to be removed from popup.xml.

Comment 91

[Neil Deakin]

I had thought it was being used here http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.css#25 but I see now that those aren't panels and some other code is setting them. I can remove the panelopen attribute changing code in the fix for the test failures.

Comment 92

[Tim Nguyen :ntim]

Ok, I just tested the patches. Here are the problems I ran into :
- Menu Panel slides down but without a transition which feels bulky
- The new animation gives headaches (it's too fast, but it's mostly the scale animation)
- A scrollbar appears at the beginning of the transition
- When items are placed in CTR add-on bar, panels are not anchored properly (they used to appear correctly before this was implemented).

Comment 93

[Ed Lee :Mardak]

Attachment: sample animation webm video

I happened across this bug and noticed there were no videos/images of what's landing, so I made this.

Comment 94

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #82)
> Comment on attachment 8398461 [details] [diff] [review]
> Part 4 -  Philipp's patch to improve animation
> 
> > #BMB_bookmarksPopup {
> >   -moz-appearance: none;
> >   -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-arrow");
> >   background: transparent;
> >   border: none;
> >-  transform: scale(.1);
> >+  transform: scale(.7);
> 
> Why are you using scale(.7) here but scale(.8) in xul.css?
> 
> > #BMB_bookmarksPopup[arrowposition="after_start"][animate="cancel"],
> > #BMB_bookmarksPopup[arrowposition="before_end"][animate="cancel"] {
> >-  transform: scale(.1) skew(30deg, 20deg);
> >+  transform: scale(.7) skew(10deg, 10deg);
> > }
> 
> Why are you using scale(.7) here but scale(.6) in xul.css?

Sorry, that must be debris from trying a bunch of different values.
We can use scale(.7) everywhere.

(In reply to Tim Nguyen [:ntim] from comment #92)
> Ok, I just tested the patches. Here are the problems I ran into :
> - Menu Panel slides down but without a transition which feels bulky
> - The new animation gives headaches (it's too fast, but it's mostly the
> scale animation)
> - A scrollbar appears at the beginning of the transition
> - When items are placed in CTR add-on bar, panels are not anchored properly
> (they used to appear correctly before this was implemented).

The scrollbars are definitely a problem that needs to be addressed, even if it is only a Mac issue.
As for the timing: We need to test this on a couple of different platforms. Since this won't get uplifted until the panel gets the same treatment anyway, we have some time to let it bake.

Comment 95

[Tim Nguyen :ntim]

Its not mac only. I use windows.

Comment 96

[Neil Deakin]

Attachment: Part 5 - fix browser tests

Some tests open a popup notification then immediately try to click a button or something inside it. However, this fails because the popup now animates open and the button isn't yet in the expected location.

This patch adds a way to disable the animation for tests.

Comment 97

[Neil Deakin]

Attachment: Part 6 - adding cycle collection to the TransitionEnder

This object is used as a listener for AddSystemEventListener and holds a strong reference to a content node. Adding cycle collection prevents a leak on tests.

Comment 98

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8401844 [details] [diff] [review]
Part 5 - fix browser tests

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

::: browser/base/content/test/general/browser_customize_popupNotification.js
@@ +18,5 @@
>          let notification = PN.show(newWin.gBrowser.selectedBrowser, "some-notification", "Some message");
>          ok(notification, "showed the notification");
>          ok(PN.isPanelOpen, "panel is open");
>          is(PN.panel.anchorNode, newWin.gBrowser.selectedTab, "notification is correctly anchored to the tab");
> +        PN.panel.hidePopup();

doesn't this test need to reset transitionsEnabled to true?

::: toolkit/modules/PopupNotifications.jsm
@@ +154,5 @@
>    get iconBox() {
>      return this._iconBox;
>    },
>  
> +  // Enable or disable the opening/closing transition.

nit: functions docs on this file are all on the form of /** */

Comment 99

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f3408af361
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d79336c9d19
https://hg.mozilla.org/integration/mozilla-inbound/rev/c65c5250c1f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/127895063bb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fe80857423
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1702d62ee1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c661f15fcd

Comment 100

[Neil Deakin]

I checked in all the patches including the bookmarks menu ones. One additional test needed to be changed (browser_toolbarbutton_menu_context.js) to disable the animation due to this.

Here is the passing try build: https://tbpl.mozilla.org/?tree=Try&rev=c84be7f1565a

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4f3408af361
https://hg.mozilla.org/mozilla-central/rev/3d79336c9d19
https://hg.mozilla.org/mozilla-central/rev/c65c5250c1f2
https://hg.mozilla.org/mozilla-central/rev/127895063bb0
https://hg.mozilla.org/mozilla-central/rev/96fe80857423
https://hg.mozilla.org/mozilla-central/rev/0f1702d62ee1
https://hg.mozilla.org/mozilla-central/rev/c2c661f15fcd

Comment 102

[Andrei Vaida [:avaida]]

This feature seems to be covered by automated testing, is additional manual QA required?

Comment 103

[Neil Deakin]

It's only marginally covered by automated testing, and has been manually tested by several people. I think this is visible enough that we'll find out soon enough of any issues arise, so I don't think any additional testing is needed.

Comment 104

[[e-mail]]

Comment on attachment 8395267 [details] [diff] [review]
Animated arrow panels, v4

>+      nsCOMPtr<TransitionEnder> ender = new TransitionEnder(aPopup, aDeselectMenu);
Bah, how did I miss this? I'll fix it as part of bug 514280.

Comment 105

[Peter Henkel [:Terepin]]

Why is animation behavior inconsistent? "All bookmarks" lacks animation when you close it by clicking on it and "options" menu lacks it completely. Is this intended?

Comment 106

[Tim Nguyen :ntim]

(In reply to Peter Henkel [:Terepin] from comment #105)
> Why is animation behavior inconsistent? "All bookmarks" lacks animation when
> you close it by clicking on it and "options" menu lacks it completely. Is
> this intended?

Unecessary scrollbars were added with the new animation, which is why we decided to disable the animation on these 2 big panels.

Comment 107

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Since bug 994582, bug 994194 and bug 989991 aren't fixed yet, I'd still consider this effort a work in progress. Therefore it shouldn't move forward into Aurora (yet).
Forgive my ignorance about how patches move from channel to channel, but is there anything to do in particular to keep this patch from uplifting next week?

Comment 108

[Justin Dolske [:Dolske]]

It's probably simplest and cleanest to just back out the patches from this bug after uplift to Aurora.

Alternatively, if we expect the other bugs to be fixed soon, we could uplift those to Aurora. I don't think it hurts to have this half-finished for a while on Aurora.

Comment 109

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Justin Dolske [:Dolske] from comment #108)
> It's probably simplest and cleanest to just back out the patches from this
> bug after uplift to Aurora.

OK, then let's do that for now.

> Alternatively, if we expect the other bugs to be fixed soon, we could uplift
> those to Aurora. I don't think it hurts to have this half-finished for a
> while on Aurora.

I would generally agree, but since there sin' even a final design yet (now that we have to change it) it feels a little *too* unfinished to me ;)
I'm wondering if cases like these could be a good reason to revive the UX branch again.