Closed Bug 994117 Opened 6 years ago Closed 5 years ago

Consider delaying the popupshown event until after the transition finishes

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

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.
Still to do: fix leaks and tests
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Blocks: 989991
Attached patch delayps-style (obsolete) — Splinter Review
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.
Attachment #8423824 - Attachment is obsolete: true
Attachment #8428680 - Flags: review?(dbaron)
I had to do this due to bug 1015952.
Attachment #8428682 - Flags: review?(mixedpuppy)
Also enables the animation on the main panel.
Attachment #8428683 - Flags: review?(neil)
Attachment #8428683 - Flags: review?(mconley)
Attachment #8428682 - Flags: review?(mixedpuppy) → review+
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?
> >+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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
I can remove the 'else' bit and just call CancelListener unconditionally. I assume that's what you mean?
Comment on attachment 8428680 [details] [diff] [review]
delayps-style

dbaron's status says busy for a while so let's try someone else
Attachment #8428680 - Flags: review?(dbaron) → review?(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.
Attachment #8428680 - Flags: review?(birtles)
(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.
(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 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.
Attachment #8428683 - Flags: review?(neil) → review+
(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?
(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.
(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.
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.
Attachment #8428680 - Attachment is obsolete: true
Attachment #8439290 - Flags: review?(birtles)
Add a test, address comments, and change method called from the other patch.
Attachment #8428683 - Attachment is obsolete: true
(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.
Flags: needinfo?(enndeakin)
Oops. This should be the correct patch.
Attachment #8439290 - Attachment is obsolete: true
Attachment #8439290 - Flags: review?(birtles)
Attachment #8439642 - Flags: review?(birtles)
Flags: needinfo?(enndeakin)
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.
Attachment #8439642 - Flags: review?(birtles)
Attached patch delayps-animsSplinter Review
Attachment #8439642 - Attachment is obsolete: true
Attachment #8440445 - Flags: review?(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.
Attachment #8440445 - Flags: review?(birtles) → review+
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.
I don't think this patch is upliftable to beta.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
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.
(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?
Flags: needinfo?(enndeakin)
Depends on: 1027014
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.
Flags: needinfo?(enndeakin)
Depends on: 1028519
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.
Flags: needinfo?(enndeakin)
(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.
Flags: needinfo?(enndeakin)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
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?
(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?
This patch reverts bug 610545 and goes back to the old animation, which would be a temporary change until other bugs are addressed.
Flags: needinfo?(enndeakin)
(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).
Attachment #8449486 - Flags: review?(dao)
Attachment #8449486 - Flags: review?(dao) → review+
I filed bug 1034067 for disabling the animation.
Depends on: 1088637
Blocks: 1189630
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.