994117 - Consider delaying the popupshown event until after the transition finishes

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Now that bug 610545 has added arrow panel transitions, the event flow is as follows:

popupshowing before making it visible
popupshown immediately after making it visible
transitionend once the transition is finished

popuphiding before making it hidden
transitionend once the transition is finished
popuphidden at the end

Tests often want to click on the popup after opening it. Currently, tests disable the animation. They should do this anyway to speed up the test.

However, if they don't do this for whatever reason, as popupshown is fired once it is only partially visible, they may fail. Instead, consider delaying the popupshown event so it fires after the transition is finished much like is done with popuphidden.

Comment 1

[Neil Deakin]

Attachment: Delay popupshown until transition has finished

Still to do: fix leaks and tests

Comment 2

[Neil Deakin]

Attachment: delayps-style

Need a way to know whether a transition is currently occurring in order to tell whether I should hook up a transitionend listener or not.

I only need it for transitions, not animations. It seemed easier to do what I've done here than add a set of additional methods for this one case.

Comment 3

[Neil Deakin]

Attachment: Disable transition for popups in social tests

I had to do this due to bug 1015952.

Comment 4

[Neil Deakin]

Attachment: Delay popupshown until transition has finished

Also enables the animation on the main panel.

Comment 5

[[e-mail]]

Comment on attachment 8428683 [details] [diff] [review]
Delay popupshown until transition has finished

Sorry for the delay; when I first applied the patch I wondered where the animations had gone and I didn't get around to finding out that bug 994582 had removed them until now.

>+NS_IMETHODIMP nsXULPopupShownEvent::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsMenuPopupFrame* popup = do_QueryFrame(mPopup->GetPrimaryFrame());
>+  if (popup) {
>+    // ResetPopupShownDispatcher will delete the reference to this, so keep
>+    // another one until Run is finished.
>+    nsRefPtr<nsXULPopupShownEvent> event = this;
>+    // Only call Run if it the dispatcher was assigned. This avoids calling the
>+    // Run method if the transitionend event fires multiple times.
>+    if (popup->ClearPopupShownDispatcher()) {
>+      return Run();
>+    }
>+  }
>+  else {
>+    CancelListener();
>   }
When wouldn't you want to cancel the listener? The common case where the popup is waiting for the dispatcher cancels the listener of course, as does the case where the popup frame no longer exists, but what about the case where the popup frame exists but isn't waiting?

Comment 6

[Neil Deakin]

> >+NS_IMETHODIMP nsXULPopupShownEvent::HandleEvent(nsIDOMEvent* aEvent)
> >+{
> >+  nsMenuPopupFrame* popup = do_QueryFrame(mPopup->GetPrimaryFrame());
> >+  if (popup) {
> >+    // ResetPopupShownDispatcher will delete the reference to this, so keep
> >+    // another one until Run is finished.
> >+    nsRefPtr<nsXULPopupShownEvent> event = this;
> >+    // Only call Run if it the dispatcher was assigned. This avoids calling the
> >+    // Run method if the transitionend event fires multiple times.
> >+    if (popup->ClearPopupShownDispatcher()) {
> >+      return Run();
> >+    }
> >+  }

Are you referring to the else case here? ClearPopupShownDispatcher will call ClearListener if mPopupShownDispatcher is set. mPopupShownDispatcher is only set in one place just before the listener is added, and cleared in one place in ClearPopupShownDispatcher just after the listener is removed. So I don't think it is needed to call ClearListener there.

Comment 7

[[e-mail]]

(In reply to Neil Deakin from comment #6)
> Are you referring to the else case here? ClearPopupShownDispatcher will call
> ClearListener if mPopupShownDispatcher is set. mPopupShownDispatcher is only
> set in one place just before the listener is added, and cleared in one place
> in ClearPopupShownDispatcher just after the listener is removed. So I don't
> think it is needed to call ClearListener there.

Well, if that was the case, then it would seem that neither of the else clauses could ever happen. In fact thinking about it more, unless it finds a frame waiting specifically for it, it should just remove itself and go away.

Comment 8

[Neil Deakin]

I can remove the 'else' bit and just call CancelListener unconditionally. I assume that's what you mean?

Comment 9

[Neil Deakin]

Comment on attachment 8428680 [details] [diff] [review]
delayps-style

dbaron's status says busy for a while so let's try someone else

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8428680 [details] [diff] [review]
delayps-style

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

::: layout/style/nsAnimationManager.cpp
@@ +332,5 @@
>  {
> +  if (aProperty == eCSSProperty_UNKNOWN) {
> +    return mAnimations.Length() > 0;
> +  }
> +

Unfortunately this doesn't really work since:

a) All the animations in mAnimations may point to empty keyframes rules, i.e. you'd return true even when you're not actually modifying any properties. That might be ok, but it doesn't match the description in the header file.

b) Animations can remain in mAnimations even after they have finished their animation interval. Whilst transitions get removed one tick after they finish, animations hang around.

I think what you are really trying to determine here is if you have animations that have yet to finish?

I think what you want is "HasCurrentAnimationsAt" which takes a refresh driver timestamp, goes through mAnimations, calls GetComputedTimingAt and returns true if at least one animations has phase before or active.

("current" is a Web Animations term that roughly corresponds to animations that are in play or are waiting to start.)

If you define it on CommonElementAnimationData in AnimationCommon.cpp you can do it just once for both animations and transitions.

That would still return true if the animations/transitions are not animating any properties but I think that matches the name of the method and won't be a problem for your particular use case since transitions always animate exactly one property.

Also this bug needs tests since I'm doing a lot of refactoring of this code at the moment and I don't want to break your changes here.

Comment 11

[Neil Deakin]

(In reply to Brian Birtles (:birtles) from comment #10)
> I think what you are really trying to determine here is if you have
> animations that have yet to finish?

I don't care about animations for the purposes of this bug right now. I only care about transitions. I want to know if a transition is currently occuring that has not yet fired its transitionend, so that I know whether to listen for the event or not.

Comment 12

[[e-mail]]

(In reply to Neil Deakin from comment #8)
> I can remove the 'else' bit and just call CancelListener unconditionally. I
> assume that's what you mean?

I would be happy with that, since currently it's unclear why it's written the way it is.

Comment 13

[[e-mail]]

Comment on attachment 8428683 [details] [diff] [review]
Delay popupshown until transition has finished

>+NS_IMETHODIMP nsXULPopupShownEvent::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsMenuPopupFrame* popup = do_QueryFrame(mPopup->GetPrimaryFrame());
>+  if (popup) {
>+    // ResetPopupShownDispatcher will delete the reference to this, so keep
>+    // another one until Run is finished.
>+    nsRefPtr<nsXULPopupShownEvent> event = this;
>+    // Only call Run if it the dispatcher was assigned. This avoids calling the
>+    // Run method if the transitionend event fires multiple times.
>+    if (popup->ClearPopupShownDispatcher()) {
>+      return Run();
>+    }
>+  }
>+  else {
>+    CancelListener();
>   }
r=me with all failure cases cancelling the listener as discussed.

Comment 14

[Brian Birtles (:birtles)]

(In reply to Neil Deakin from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > I think what you are really trying to determine here is if you have
> > animations that have yet to finish?
> 
> I don't care about animations for the purposes of this bug right now. I only
> care about transitions. I want to know if a transition is currently occuring
> that has not yet fired its transitionend, so that I know whether to listen
> for the event or not.

Transitions are a subset of animations.

Does the approach I outlined make sense? Or are you concerned about the performance of it?

Comment 15

[Neil Deakin]

(In reply to Brian Birtles (:birtles) from comment #14)
> Does the approach I outlined make sense? Or are you concerned about the
> performance of it?

It makes sense and I have it almost working. My concern was only that I only add a transition listener and don't want to have to handle animations if I don't need to.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Neil Deakin from comment #15)
> It makes sense and I have it almost working. My concern was only that I only
> add a transition listener and don't want to have to handle animations if I
> don't need to.

If it's not too much trouble, I'd rather make something that makes sense for the general-case of animations. We're currently trying to limit the amount of special-case transitions code and I think a HasCurrentAnimationsAt method will be useful elsewhere.

If this proves too much hassle though, we could adapt your original approach and make it check that all the keyframes are not empty in the general case.

Comment 17

[Neil Deakin]

Attachment: Part 1 - add HasCurrentAnimations method

OK, I hope I understood your comments above. It seems to work for a transition. I added a test to the other patch for my usage. I'm not sure how to test animations.

Comment 18

[Neil Deakin]

Attachment: Part 2.1 - Delay popupshown until transition has finished

Add a test, address comments, and change method called from the other patch.

Comment 19

[Brian Birtles (:birtles)]

(In reply to Neil Deakin from comment #17)
> Created attachment 8439290 [details] [diff] [review]
> Part 1 - add HasCurrentAnimations method
> 
> OK, I hope I understood your comments above. It seems to work for a
> transition. I added a test to the other patch for my usage. I'm not sure how
> to test animations.

Did you forget to qrefresh? It looks the same as the last patch.

Comment 20

[Neil Deakin]

Attachment: Part 1 - add HasCurrentAnimations method, real patch

Oops. This should be the correct patch.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8439642 [details] [diff] [review]
Part 1 - add HasCurrentAnimations method, real patch

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

::: layout/style/AnimationCommon.cpp
@@ +679,5 @@
> +CommonElementAnimationData::HasCurrentAnimationsAt(TimeStamp aTime)
> +{
> +  for (uint32_t animIdx = mAnimations.Length(); animIdx-- != 0; ) {
> +    ElementAnimation* anim = mAnimations[animIdx];
> +    if (!anim->mProperties.IsEmpty() && anim->IsRunningAt(aTime)) {

Unfortunately IsRunningAt returns false for animations in their delay phase. So if you have a transition with a delay this won't return true.

Perhaps you could add ElementAnimation::IsCurrentAt which just checks for before/active but also returns false if mStartTime is null (needed for finished transitions). It should probably return true for paused animations too unlike IsRunningAt.

Also, I don't think we need the check for empty keyframes here. "Current animations" should return true even if all the animations have empty keyframes. But I think "Has animation of property unknown" should not which was my main concern with the original patch.

(And by the way, provided we add the check for empty keyframes, I'd be ok with landing your original patch too if that seems simpler to you. My main concern with it was that it wouldn't do give you what you need for animations but since you are only interested in transitions that's not a problem.)

The rest looks good.

Comment 22

[Neil Deakin]

Attachment: delayps-anims

Comment 23

[Brian Birtles (:birtles)]

Comment on attachment 8440445 [details] [diff] [review]
delayps-anims

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

Looks great. Thanks Neil. r=me with those two comments addressed.

::: layout/base/nsLayoutUtils.h
@@ +1877,5 @@
>     */
>    static bool HasAnimations(nsIContent* aContent, nsCSSProperty aProperty);
>  
>    /**
> +   * Returns true if the content node has any current animations or transitions.

Perhaps add:

"A current animation is any animation that has not yet finished playing including paused animations."

::: layout/style/AnimationCommon.cpp
@@ +383,5 @@
>  
>  bool
> +ElementAnimation::IsCurrentAt(TimeStamp aTime) const
> +{
> +  if (IsPaused()) {

I'd drop this part. It's possible to pause an animation that has already finished.

Comment 24

[Neil Deakin]

https://tbpl.mozilla.org/?tree=Try&rev=6fc5a34a33ee

Checked in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0404eba196
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9d11e5302a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e49b9b55cbb

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff0404eba196
https://hg.mozilla.org/mozilla-central/rev/0e9d11e5302a
https://hg.mozilla.org/mozilla-central/rev/9e49b9b55cbb

Comment 26

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

Once this has baked for a couple of days, we should uplift it to Aurora and Beta so we have a complete Arrowpanel implementation in 31.

Comment 27

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

I don't think this patch is upliftable to beta.

Comment 28

[Tim Nguyen :ntim]

(In reply to :Gavin Sharp (email [e-mail]) from comment #27)
> I don't think this patch is upliftable to beta.

I don't think we want to ship something weirdly inconsistent as panel animation. This bug is necessary, otherwise the main panel will be inconsistent with other panels, which will cause a major feedback buzz about it, and possibly many duplicate bugs.

Comment 29

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

What we want and what we can have don't always line up.

We can disable the animations across the board relatively easily if we think that's better than inconsistency.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #28)
> will cause a major feedback buzz about
> it, and possibly many duplicate bugs.

This sounds like a vast exaggeration. We may get some duplicate bugs filed, but that's no big deal.

Neil, do you think this is low-risk enough to be uplifted to aurora?

Comment 31

[Neil Deakin]

This has caused a number of regressions and some intermittent test failures, so I don't think this is ready to be checked in anywhere else.

Comment 32

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

So it sounds to me that we can't ship the full version of the new panel animation (which includes this patch) with 31 and possibly not even with 32.

If that's the case, can we move back to the old behavior for these two versions?

Shipping half-implemented animations or disabling them completely are both pretty noticeable regressions that shouldn't be in the release version.

Comment 33

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

(In reply to Philipp Sackl [:phlsa] from comment #32)
> So it sounds to me that we can't ship the full version of the new panel
> animation (which includes this patch) with 31 and possibly not even with 32.
> 
> If that's the case, can we move back to the old behavior for these two
> versions?

I just want to clarify so we're on the same page here:
- in Firefox 30, we have no arrow panel animations
- in Firefox 31 beta, we currently have animations for all arrow panels, except for:
  - on linux (disabled completely for all panels)
  - the primary toolbar menu panel

- in Firefox 33, we have animations for all arrow panels, except for on linux (disabled completely for all panels)

This patch is required to enable the menu panel animation, and uplifting it to Firefox 31 is not feasible (there are outstanding regressions, it's too large of a change).

> Shipping half-implemented animations or disabling them completely are both
> pretty noticeable regressions that shouldn't be in the release version.

Not shipping the animations is not a regression for users currently using Firefox 30, right?

I don't have a strong preference for what we should do - if you're proposing disabling all animations instead of shipping a partial implementation, I would support that.

Comment 34

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #33)
> I just want to clarify so we're on the same page here:
> - in Firefox 30, we have no arrow panel animations

Not true. We have a simpler animation there: bug 767133, bug 767861

Comment 35

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #33)
> I just want to clarify so we're on the same page here:
> - in Firefox 30, we have no arrow panel animations

As Dão said, we do have some animations in release today (slide down plus alpha).
It was my understanding that disabling animations altogether would not revert us to that state but would just remove all animations. That would be the regression I was talking about.

If we really have to chose between having partial animations or no animations at all, I'd lean towards having no animation at all.

Comment 36

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

Re-creating the needinfo? to Neil:
Is there any chance we can revert to the behavior we have in release now (slide+fade) for all panels until we have a version with all the patches related to the new animation?

Comment 37

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

(In reply to Philipp Sackl [:phlsa] from comment #35)
> It was my understanding that disabling animations altogether would not
> revert us to that state but would just remove all animations.

I don't think that's necessarily true - we would need to also re-introduce the CSS from bug 767133/bug 767861, but I think that should be doable.

Neil?

Comment 38

[Neil Deakin]

Attachment: Disable animation

This patch reverts bug 610545 and goes back to the old animation, which would be a temporary change until other bugs are addressed.

Comment 39

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

(In reply to Neil Deakin from comment #38)
> Created attachment 8449486 [details] [diff] [review]
> Disable animation
> 
> This patch reverts bug 610545 and goes back to the old animation, which
> would be a temporary change until other bugs are addressed.

That's great! Can we uplift this patch to Aurora and Beta? All new animations work in Nightly, so we don't need it there (unless this needs testing).

Comment 40

[Neil Deakin]

I filed bug 1034067 for disabling the animation.

Comment 41

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336