Closed Bug 1180125 Opened 9 years ago Closed 9 years ago

Queue animation and transition events from Animation::Tick and dispatch as a separate step

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(10 files, 7 obsolete files)

part 0 - Fix an obvious bug in animation_utils.js
1.50 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 2 - Move AnimationCommon classes out of CSS namespace
25.46 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 4 - Move PseudoTypeAsString to AnimationCommon and reuse
9.97 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 5 - Move TransitionEventInfo to nsTransitionManager.h
5.06 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 6 - Use DelayedEventDispatcher in nsTransitionManager
4.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 8 - Dispatch transition events from refresh driver
4.13 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 1b - Hook nsAnimationManager's list of events up to the cycle collector
9.17 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 3 - Extract DelayedEventDispatcher
12.49 KB, patch
Details | Diff | Splinter Review
part 1 - Queue and dispatch CSS animation events as a separate step
21.09 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 7 - Queue transition events from CSSTransition::Tick
8.41 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
This is about implementing the following part from bug 1151731 comment 4:

> a) Make event queueing a separate step performed by Animation::Tick and dispatched in nsRefreshDriver::Tick

The reason this needs to be a separate step is that when we actually run these callbacks they should see a fully updated timing model so we need to make sure to update everything first *then* dispatch events.

I'd really like to reuse the work Xidorn did in bug 1168705 since it's very close to what we're doing here. However, there are two problems in doing that:

a) The nsRefreshDriver's PendingEvent setup expects an nsIDOMEvent. I'm not sure how expensive it is to turn all our internal events into nsIDOMEvents but even if we workaround that there's a bigger problem:

b) The way events are queued now is not in any particular order. We walk through the different collections in the linked list which is not sorted and then we add all the events for each element. As far as I can tell, there are no requirements on the ordering of events in the spec, but it seems like we should do something sensible: probably timeline order then document order.

With regards to (b), once we tick animations from their timelines (where they'll be stored in a hashmap) this ordering will be even more random so we'll probably want to introduce sorting to the events before dispatching them. That suggests we need to queue them (batch them might be a better word) then sort them just before dispatching. That's going to make it hard to reuse Xidorn's stuff unfortunately.
A very rough attempt at reworking the way CSS animation events are queued
and dispatched.

Applies on top of bug 1150810 (which needs bug 1171817) on central from
2015-07-02-ish (specifically bug 1171817 which just landed will conflict with
this).

TODO: Something similar to transitions
TODO: Rename 'queue' to 'batch'
Looking into a couple of test failures, I discovered this patch queues events with the pres context for its target element but:

  [pres context] != [pres context]->RefreshDriver()->PresContext()

so when we go to dispatch queued up events from the refresh driver we often don't find any to dispatch.

The easiest solution is probably to make nsAnimationManager::QueueEvent basically look for the root pres context and queue all events on its associated animation manager. I'm not sure if that's entirely valid, however.

It seems like:
* a refresh driver never updates it pres context until it is Disconnect-ed (which happens when the pres context is destroyed)
* likewise an animation manager never updates its pres context until it is Disconnected-ed (which happens when the pres context is destroyed)
* a pres context update its refresh driver:
  - when it is destroyed (set to null)
  - on init (set to 1 or 3 different possibilities including a new refresh driver)

So it probably is safe to just use the pres context associated with a refresh driver.
(In reply to Brian Birtles (:birtles) from comment #2)
> Looking into a couple of test failures, I discovered this patch queues
> events with the pres context for its target element but:
> 
>   [pres context] != [pres context]->RefreshDriver()->PresContext()
> 
> so when we go to dispatch queued up events from the refresh driver we often
> don't find any to dispatch.

We can't traverse child documents to find the right pres context? Something similar to RunFrameRequestCallbacks[1] or GetProfileTimelineSubDocShells[2].

[1] https://hg.mozilla.org/mozilla-central/file/951116f384da/layout/base/nsRefreshDriver.cpp#l1493
[2] https://hg.mozilla.org/mozilla-central/file/951116f384da/layout/base/nsRefreshDriver.cpp#l1436
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> We can't traverse child documents to find the right pres context? Something
> similar to RunFrameRequestCallbacks[1] or GetProfileTimelineSubDocShells[2].

That's a good idea. I think we could either:

a) Always store events on the animation manager for the pres context associated with the refresh driver (and have calls to DispatchEvents defer this animation manager)
b) Store events on the appropriate animation manager and have the refresh driver iterate over all the descendent animation managers

(a) seems simpler but we might encounter problems if we try to sort events from different documents
Hi David, what do you think of this approach? Does it make sense? Let me know if
you'd prefer I split this patch up a bit more finely before asking you to look
at it.

If you agree with this approach, I plan to do the same thing for transitions.

Furthermore, I expect to add a further patch that sorts the events before
dispatching them since we'll need that before long (and it's probably good
to have a logical and consistent order for these anywhere).
Attachment #8631462 - Flags: review?(dbaron)
Attachment #8629253 - Attachment is obsolete: true
There was a bug introduced in to animation_utils.js in
https://hg.mozilla.org/mozilla-central/rev/496e867cd2cd (bug 1070745). This
patch fixes this bug so we can be sure testing events correctly before messing
with them.
Attachment #8631989 - Flags: review?(dbaron)
This slightly simplifies the previous patch
Attachment #8631990 - Flags: review?(dbaron)
Attachment #8631462 - Attachment is obsolete: true
Attachment #8631462 - Flags: review?(dbaron)
For my own reference, I started working on removing the call to DispatchEvents in nsPresShell::FlushPendingNotifications because:

* There's nothing in the spec to say we should dispatch animationstart events in
  the same frame as an animation is generated. Rather, the only place this is
  really specified is HTML's processing model which says CSS animations and
  events are only done once per frame:
    https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-9
* When we make animationstart events not fire until the animation has actually
  started (bug 1134163), we won't be able to continue dispatching them in the
  same frame anyway.
* From an architectural point of view, flushing style is an animation function,
  but events are a timing function so we shouldn't be doing this when updating
  style.

However, without that call you have to wait *two* frames to get the animationstart event. Basically, what happens is:

Frame n:
- Update element style to create new animation
Frame n+1:
- Tick animations and queue events
- Dispatch events
- Process pending style changes
- Queue events <-- animationstart queued here
Frame n+2:
- Tick animations
- Dispatch events <-- animationstart dispatched here

If you trigger a style flush in frame n then you only have to wait one frame.

The call to call to DispatchEvents in nsPresShell::FlushPendingNotifications means that we dispatch the event in frame n+1 even if we don't trigger a style flush in frame n.

This is going to change in bug 1134163 anyway so I might just leave this alone for now.
This is a very rough first pass at this. I think I probably want to do
something like:

* Move CommonAnimationManager out of the css namespace (it's about time and it
  makes working on this code even more painful; one day I'll finally rename
  nsAnimationManager / nsTransitionManager to CSSAnimationManager
  / CSSTransitionManager and put them in the mozilla namespace too).
* Create a templated superclass for managing events that contains the QueueEvent
  / DispatchEvent etc. methods and make nsAnimationManager inherit from that
* Stick the method to convert a PseudoType to a string somewhere more sensible
* Make all the other changes bit by bit
Just a minor tweak to reverse the order in which animation events and fullscreen (pending) events are dispatched to match the HTML spec
Attachment #8633192 - Flags: review?(dbaron)
Attachment #8631990 - Attachment is obsolete: true
Attachment #8631990 - Flags: review?(dbaron)
The long-term plan is to drop the mozilla::css namespace altogether. Before we
go to much further with refactoring code in AnimationCommon, we should drop
usage of the mozilla::css namespace. Specifically, this patch moves the
CommonAnimationManager and AnimValuesStyleRule classes to the mozilla namespace.
Attachment #8633193 - Flags: review?(dbaron)
This patch extracts a utility class for queueing up a series of EventInfo
objects (of templated type) and then dispatching them. This covers the event
queuing behavior in nsAnimationManager so that we can reuse it in
nsTransitionManager.
Attachment #8633194 - Flags: review?(dbaron)
Prior to this patch, CSSAnimation defined a method for converting an
nsCSSPseudoElements::Type to a nsString (but only for the set of
pseudo-elements that can have animations). We would like to re-use this
when setting up transition events so this patch moves it to
AnimationCollection. Re-using this method more widely means we can make
a few further simplifications to the code.
Attachment #8633196 - Flags: review?(dbaron)
This is needed so we can allocate storage in nsTransitionManager for the
transition events we will queue for delayed dispatch.
Attachment #8633197 - Flags: review?(dbaron)
This simply uses the DelayedEventDispatcher in place of the previous array of
TransitionEventInfo objects. Doing the actual delayed dispatch is performed in
a separate patch.
Attachment #8633198 - Flags: review?(dbaron)
This patch moves the logic for queueing events out of the logic for flushing
transitions making it a separate step. It still doesn't delay the dispatch of
those events into a separate step yet. That is done in a subsequent patch.

This patch also makes sure to clear any queued events when the nsPresShell that
owns the transition manager is destroyed. We don't expect CSSTransition::Tick to
be called anywhere except nsTransitionManger::FlushTransitions so there
shouldn't be any orphaned events but for completeness it seems best to add this
now. (Later, when we tick transitions from their timeline we will need this.)
Attachment #8633199 - Flags: review?(dbaron)
This patch causes transition events to be dispatched as a separate step after
sampling the transitions. Eventually this will allow us to sample transitions
from their timeline (independently of where they came from and in potentially
any order) by separating the concepts of sampling and event dispatch.
Attachment #8633201 - Flags: review?(dbaron)
Attachment #8632022 - Attachment is obsolete: true
Comment on attachment 8633192 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

Is there a situation where we might queue events and then not run
the refresh driver tick shortly afterwards?  If so, it seems like
we might need to hook this up to the cycle collector.
Comment on attachment 8633194 [details] [diff] [review]
part 3 - Extract DelayedEventDispatcher

>+  void DispatchEvents(nsPresContext* const & aPresContext)

No need for reference here; nsPresContext* const should be fine.


r=dbaron with that
Attachment #8633194 - Flags: review?(dbaron) → review+
Comment on attachment 8633196 [details] [diff] [review]
part 4 - Move PseudoTypeAsString to AnimationCommon and reuse

>Bug 1180125 part 4 - Move PseudoTypeAsString to AnimationCommon and reuse

AnimationCollection, not AnimationCommon.

>+/*static*/ nsString
>+AnimationCollection::PseudoTypeAsString(nsCSSPseudoElements::Type aPseudoType)
>+{
>+  switch (aPseudoType) {
>+    case nsCSSPseudoElements::ePseudo_before:
>+      return NS_LITERAL_STRING("::before");
>+    case nsCSSPseudoElements::ePseudo_after:
>+      return NS_LITERAL_STRING("::after");
>+    default:

Probably good to assert here that it's ePseudo_NotPseudoElement.

>+      return EmptyString();
>+  }
>+}

r=dbaron with that
Attachment #8633196 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #20)
> Comment on attachment 8633192 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> Is there a situation where we might queue events and then not run
> the refresh driver tick shortly afterwards?  If so, it seems like
> we might need to hook this up to the cycle collector.

Yes, I think there could be such a situation. Even if it doesn't happen now, it
may well occur in the future so I think it's best to hook this up.

Here's a patch to do that. If this approach looks good to you, I'll apply the
same treatment to nsTransitionManager's list of events in a separate patch.
Attachment #8638366 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #21)
> Comment on attachment 8633194 [details] [diff] [review]
> part 3 - Extract DelayedEventDispatcher
> 
> >+  void DispatchEvents(nsPresContext* const & aPresContext)
> 
> No need for reference here; nsPresContext* const should be fine.

The intention was that this parameter should continue to reflect the mPresContext member of the calling manager which I figured could go null if the code executed by EventDispatcher::Dispatch caused the prescontext to be destroyed (therefore triggering a call to CommonAnimationManager::Disconnect). Does that sound possible?
Flags: needinfo?(dbaron)
Updated version of part 3 that moves the cycle-collection code to DelayedEventDispatcher
Attachment #8633194 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles) from comment #24)
> The intention was that this parameter should continue to reflect the
> mPresContext member of the calling manager which I figured could go null if
> the code executed by EventDispatcher::Dispatch caused the prescontext to be
> destroyed (therefore triggering a call to
> CommonAnimationManager::Disconnect). Does that sound possible?

Maybe.

So it's fine the way you had it *if* you add a comment explaining why.  Otherwise it's not clear.
Flags: needinfo?(dbaron)
Comment on attachment 8638366 [details] [diff] [review]
part 1b - Hook nsAnimationManager's list of events up to the cycle collector

>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Maybe good practice for this to be:
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIStyleRuleProcessor)

r=dbaron
Attachment #8638366 - Flags: review?(dbaron) → review+
Comment on attachment 8633192 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

So I guess one other concern about this patch:

we have logic to determine which documents share a refresh driver and which don't.  This seems to be overriding that logic by saying that if a refresh driver ticks, then all of its subdocuments will fire animation events right then, even if they're on a different refresh driver.  This seems at least odd, although I can't immediately describe harm that results from it.

Should this mechanism be more related to the way that refresh drivers currently relate to documents, or at least be structured not to descend into documents with different refresh drivers?
Flags: needinfo?(bbirtles)
Comment on attachment 8633199 [details] [diff] [review]
part 7 - Queue transition events from CSSTransition::Tick

The thing that seems a bit odd here is that it seems to be relying on side-effects in a rather unclear way in order to only fire the event once.  In particular, it's assuming that Tick() is going to be called exactly once between when GetComputedTiming() reports Computedtiming::AnimationPhase_After and when the transition's IsFinishedTransition() becomes true.  That seems rather sensitive.

Is there a better way to do things here?
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #28)
> Comment on attachment 8633192 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> So I guess one other concern about this patch:
> 
> we have logic to determine which documents share a refresh driver and which
> don't.  This seems to be overriding that logic by saying that if a refresh
> driver ticks, then all of its subdocuments will fire animation events right
> then, even if they're on a different refresh driver.  This seems at least
> odd, although I can't immediately describe harm that results from it.
> 
> Should this mechanism be more related to the way that refresh drivers
> currently relate to documents, or at least be structured not to descend into
> documents with different refresh drivers?

Yes, I think you're right. Events are queued during a call to:

1. nsAnimationManager::WillRefresh, i.e. when the manager is ticked by the refresh driver. In this case we will dispatch any queued events moments later within the same refresh driver tick.

2. PresShell::FlushPendingNotifications. In this same function we then call mPresContext->AnimationManager()->DispatchEvents() so any queued events will be dispatched immediately.

3. nsAnimationManager::CheckAnimationRule, which is called by nsStyleSet::GetContext. This case doesn't dispatch events immediately.

I had forgotten about 3. If it were only 1 and 2 it wouldn't matter if we descend into subdocuments on a different refresh driver since they wouldn't have any events queued.

Long-term bug 1134163 will probably remove (2), and I'd like to get this code to the point where we get exactly one call to Animation::Tick per refresh driver tick which would eliminate (3). Until then, we probably have to do the complex thing here. I'll update the patch with the refresh driver check.
Flags: needinfo?(bbirtles)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #29)
> Comment on attachment 8633199 [details] [diff] [review]
> part 7 - Queue transition events from CSSTransition::Tick
> 
> The thing that seems a bit odd here is that it seems to be relying on
> side-effects in a rather unclear way in order to only fire the event once. 
> In particular, it's assuming that Tick() is going to be called exactly once
> between when GetComputedTiming() reports
> Computedtiming::AnimationPhase_After and when the transition's
> IsFinishedTransition() becomes true.  That seems rather sensitive.
> 
> Is there a better way to do things here?

Yes, you're right. That's much less safe than I realized. I overlooked it because I already fixed it in bug 1181392 (attachment 8638412 [details] [diff] [review]). I'll bring that patch forward to this bug.
Attachment #8633192 - Attachment is obsolete: true
Attachment #8633192 - Flags: review?(dbaron)
Attachment #8633199 - Attachment is obsolete: true
Attachment #8633199 - Flags: review?(dbaron)
With only 5 jobs left to run, this may be my best try for 2015:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57be14991588
Comment on attachment 8639132 [details] [diff] [review]
part 1 - Queue and dispatch CSS animation events as a separate step

>+  virtual ~nsAnimationManager() {}

I don't see why you need this.  nsAnimationManager is final.

r=dbaron on the rest
Attachment #8639132 - Flags: review?(dbaron) → review+
Comment on attachment 8639133 [details] [diff] [review]
part 7 - Queue transition events from CSSTransition::Tick

>transitions making it a separate step. It still delay the dispatch of

Did you mean to leave "doesn't delay" as this was in the previous commit message?

>(mPreviousPhaseOrItertaion) which we may be able to unify at the same point.

Itertaion -> Iteration (I hope!)

>   virtual ~nsTransitionManager() {}

Hmmm.  This also shouldn't be here -- but it was introduced in something else that happened between the old patch and this one.


r=dbaron
Attachment #8639133 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> Comment on attachment 8639132 [details] [diff] [review]
> part 1 - Queue and dispatch CSS animation events as a separate step
> 
> >+  virtual ~nsAnimationManager() {}
> 
> I don't see why you need this.  nsAnimationManager is final.

Without this I was seeing the following warning on OSX/Android/B2G:

   error: static_assert failed "Reference-counted class nsAnimationManager should not have a public destructor. Make this class's destructor non-public"
(In reply to Brian Birtles (:birtles) from comment #37)
> Without this I was seeing the following warning on OSX/Android/B2G:
> 
>    error: static_assert failed "Reference-counted class nsAnimationManager
> should not have a public destructor. Make this class's destructor non-public"

I should add that this only became necessary when I specifically added the cycle-collection methods to nsAnimationManager (and later, nsTransitionManager). Both of these classes were ref-counted before that (via CommonAnimationManager) but the MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING wasn't being applied to them then.
sorry had to back this out for since one of this changes might have caused bug 1188799 at least started with this pushes
Flags: needinfo?(bbirtles)
Bug 1188799 confirmed fixed by this backout.
Depends on: 1188799
I'm having trouble working out what is going on with the failing browser_translation_exceptions.js test. There is something subtly different about the timing (or perhaps order) of dispatching transitionend events that means that sometimes when we get to the transitionend handler registered in translation-infobar.xml's init method this._getAnonElt is undefined where as othertimes it is filled in.

I don't know enough about XBL to know why this would happen and it's proving hard to debug. I can't reliably reproduce it and changing almost anything in translation-infobar.xml causes it to stop failing. (I guess this would be a good place for rr if I had a linux box.)

So far as I can tell, it's not the case that we're getting transitionend events for a different element although that could be the case if there's a nested <navigation> element firing events (again, I can't reproduce this once I add instrumentation to compare the event target with the <navigation> element).

I suspect if I replaced waiting for transitionend events with getting the animations running on the navigation element and waiting on their finished promise we might be able to fix this but first I need to dig into exactly why we're expecting a transitionend event.
Flags: needinfo?(bbirtles)
Ok, the transitions we're waiting on are here:
https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/toolkit/content/xul.css#l200

I've confirmed that we're getting two animation objects on the transition element for each of the transition properties (in both failure and success cases).

Beyond that I'm not sure what's happening.

* I don't *think* it's a problem that we have two transitions and, for example, we're getting processing one transitionend event after the other and having destroyed 'this'. Firstly, I don't think there's anything different with regards to the granularity with which we batch and dispatch events within the same element, and secondly I've never seen anything in the small amount of logging I've succeeded in doing during the fail case to indicate we're processing a second transitionend event.

* I don't *think* it's a problem that 'this' is not in scope during the transitionend event or points to a different element. I've succeeded in confirming that 'this' has the same identity when registering the event listener and inside the event callback.

* I don't *think* it's a problem that the order of the opacity and margin-left events is random. Firstly, it shouldn't be (these patches should preserve ordering) and secondly I've confirmed we dispatch them in the same order in both failure and success cases.

* I wondered if it was to do with having a nested element firing transitionend events that the parent was picking up and that something had changed with regards to the order in which we dispatch transitionend events between ancestors and descendents, particularly if, due to our use of XBL here, we were crossing subdocument boundaries. However, I have at least confirmed that in both success and failure cases the element target is a <navigation> element.

However, I think the last one might be close. I've just observed that in the failure case the event target's parent node is null, whilst in the success case it is navigationbox.

What could be happening is we have some other element listening for transitionend events in another subdocument and the handler for that event is doing some document surgery.

Before this bug (specifically part 8) we'd run FlushTransitions on all the TransitionManagers attached to the ticking refresh driver and dispatch events right away. Now, however, we'll queue them all then dispatch them beginning with the document of the pres context associated with the refresh driver and then visiting all the subdocuments. I suppose that will mean that we might process outer events before inner events. So if there is some ancestor document that does cleanup or tree surgery on transitionend then when we get to the transitionend for the inner <navigation> element it might have already been torn down.
I think I might have finally narrowed this down a bit.

In browser_translation_exceptions.js we get to the check "check shouldShowInfoBar initially returns true" then yield waiting for the pop-up to open.

In the success case, we dispatch transitionend events (opacity and margin-top) while waiting and pass them to the infobar and notification (infobar extends notification).

In the failure case, however, we resume the test (i.e. the Promise from openPopup resolves) before the transitionend events get dispatched. Then we proceed with the test to the point where we close the infobar.

Next we go to re-open the infobar and yield waiting for the pop-up. At this point the initial transitionend event is delivered but by this point the original notification is closed (and partially destroyed?) and we run into trouble.


Digging into this a bit further, the Promise-yielding openPopup() method we use to wait with is based on listening for the "popupshown" event from the notif._getAnonElt("options") element which is a <xul:button>.

So it sounds like there's a race between the popupshown event firing on the <xul:button> in the anonymous content of the infobar and the transitionend event firing on the parent <navigation>. We've come across this issue before in bug 994117 where we added code to nsMenuPopupFrame.cpp to delay the popupshown event until transitions had fired (https://hg.mozilla.org/mozilla-central/rev/0e9d11e5302a).

I notice this code is disabled on GTK which may be why bug 1188799 only fails on Ubuntu (there is one failure on OSX but it's a timeout that's incorrectly filed).

If that's correct, then the patches in this bug are probably just exposing an existing race (if I change just about anything in the test, the failure disappears so it's very time sensitive). Alternatively (and perhaps more likely), with these patches introduced, we may be ticking something else in between queueing transition events and dispatching them that causes the popupshown event to get in ahead of the transitionend event.

In either case, it would seem like simply enabling the code from bug 994117 on GTK as well would fix this.

Neil or Neil, do you know why the block of code in nsMenuPopupFrame::SetInitialChildList in https://hg.mozilla.org/mozilla-central/rev/0e9d11e5302a is disabled on GTK?
Flags: needinfo?(neil)
Flags: needinfo?(enndeakin)
Bug 1001234 and related bugs are probably relevant here.
Flags: needinfo?(neil)
I think you may be confusing the transition on the infobar with transitions on a panel. Transitions are disabled on panels on Linux due to graphical issues, so popup transitions don't come into play here. Also, it's a menu on a button which doesn't have a transition anyway (only arrow panels currently have them).

The test should wait for the infobar transition to finish before continuing I'm guessing, otherwise the button with the menu being opened isn't fully visible when it tries to open it.
Flags: needinfo?(enndeakin)
(In reply to Brian Birtles (:birtles) from comment #37)
> (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> > Comment on attachment 8639132 [details] [diff] [review]
> > part 1 - Queue and dispatch CSS animation events as a separate step
> > 
> > >+  virtual ~nsAnimationManager() {}
> > 
> > I don't see why you need this.  nsAnimationManager is final.
> 
> Without this I was seeing the following warning on OSX/Android/B2G:
> 
>    error: static_assert failed "Reference-counted class nsAnimationManager
> should not have a public destructor. Make this class's destructor non-public"

Does it work if you make it private and skip the "virtual"?
(In reply to Neil Deakin from comment #47)
> I think you may be confusing the transition on the infobar with transitions
> on a panel. Transitions are disabled on panels on Linux due to graphical
> issues, so popup transitions don't come into play here. Also, it's a menu on
> a button which doesn't have a transition anyway (only arrow panels currently
> have them).

The disabled code in question appears to be affecting more than it was intended to. We are seeing transitions on Linux for this infobar but we're following a codepath in Gecko that assumes we don't have transitions on Linux. How is that not a bug?

> The test should wait for the infobar transition to finish before continuing
> I'm guessing, otherwise the button with the menu being opened isn't fully
> visible when it tries to open it.

I thought the purpose of bug 994117 was that tests don't have to wait for *both* transitionend and popupshown to fire but could rely on popupshown happening later? That's how it works on all platforms except Linux. I'm happy to change that test but that doesn't seem to fix the root problem and it probably won't be long before someone else wastes 2 days debugging an "intermittent only on Linux" problem.
Flags: needinfo?(enndeakin)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #48)
> (In reply to Brian Birtles (:birtles) from comment #37)
> > (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> > > Comment on attachment 8639132 [details] [diff] [review]
> > > part 1 - Queue and dispatch CSS animation events as a separate step
> > > 
> > > >+  virtual ~nsAnimationManager() {}
> > > 
> > > I don't see why you need this.  nsAnimationManager is final.
> > 
> > Without this I was seeing the following warning on OSX/Android/B2G:
> > 
> >    error: static_assert failed "Reference-counted class nsAnimationManager
> > should not have a public destructor. Make this class's destructor non-public"
> 
> Does it work if you make it private and skip the "virtual"?

I expect so (this assertion doesn't run on Windows so I can't quickly check) but what's the issue with protected and explicit "virtual"? (The dtor is already virtual due to CommonAnimationManager.)
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #50)
> I expect so (this assertion doesn't run on Windows so I can't quickly check)
> but what's the issue with protected and explicit "virtual"? (The dtor is
> already virtual due to CommonAnimationManager.)

Oh, I missed the virtual destructor in CommonAnimationManager.  Probably fine then.  (Although maybe CommonAnimationManager doesn't need a virtual destructor anymore?)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #51)
> Oh, I missed the virtual destructor in CommonAnimationManager.  Probably
> fine then.  (Although maybe CommonAnimationManager doesn't need a virtual
> destructor anymore?)

That seems risky since if we add anything to ~nsAnimationManager it might not get called when deleting from a pointer to the base class.
(In reply to Brian Birtles (:birtles) from comment #49)
> The disabled code in question appears to be affecting more than it was
> intended to. We are seeing transitions on Linux for this infobar but we're
> following a codepath in Gecko that assumes we don't have transitions on
> Linux. How is that not a bug?

I think you might still be confusing the infobar with panels. The infobar isn't a panel and doesn't fire a popupshown event. It has a transition and fires the transitionend event when it is finished, which the test does not wait for.

The test then opens a menu popup from a button that is a child of the infobar, The popup does not itself have a transition but does fire the popupshown event when it is finished opening.

On Linux there shouldn't be any transition on any menus or panels. Your patch just changes the popup code to check if there is one, but there shouldn't be one.

I haven't tested this myself though, so it's possible I'm not understanding something about this specific notification being tested. If someone has added a transition to this specific menu, it shouldn't be there on Linux.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #53)
> I think you might still be confusing the infobar with panels. The infobar
> isn't a panel and doesn't fire a popupshown event. It has a transition and
> fires the transitionend event when it is finished, which the test does not
> wait for.
> 
> The test then opens a menu popup from a button that is a child of the
> infobar, The popup does not itself have a transition but does fire the
> popupshown event when it is finished opening.
> 
> On Linux there shouldn't be any transition on any menus or panels. Your
> patch just changes the popup code to check if there is one, but there
> shouldn't be one.

Thanks for that, that's a helpful explanation. The transition I'm seeing is this one:

  https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/toolkit/content/xul.css#l200

which appears to apply to the translationbar in this case.

At the same time, the menu popup's animate attribute is set to open and the menu popup's frame has current transitions on it.

I don't know how the XBL mapping works here but perhaps the code in nsMenuPopupFrame is picking up the transition on the infobar causing this to pass on non-Linux platforms, when it really shouldn't. That might be because we're now looking up whether we have animations by frame, instead of by content (https://hg.mozilla.org/mozilla-central/rev/2fd98d3d9258), I'm not sure.

It sounds like it's ok to change the test anyway.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: