1182856 - transitionend should not be fired after element is made display:none

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Chris Rebert]

Per the first RESOLUTION in the "AnimationEnd events and display: none" section of https://lists.w3.org/Archives/Public/www-style/2015Apr/0405.html concerning clarification of CSS Transitions Level 1, if an element is made display:none while a transition is in progress, no transitionend event should be fired for that aborted transition. Chrome, Safari, and IE11 comply with this. Firefox currently doesn't; it still fires a transitionend event in said scenario.

Testcase: http://jsfiddle.net/cvrebert/ch414jjn/show

Comment 1

[David Baron :dbaron:]

Bug 962594 fixed this for CSS animations, although transitions are a bit different.

When we fix this we can probably mark both bug 537143 and bug 1158431 as WONTFIX or INVALID.

Comment 2

[Chris Rebert]

There is also a W3C CSS Test for the spec'd behavior that is awaiting review (hint, hint)

Comment 3

[Boris Chiou [:boris]]

Attachment: Part 1: Add a helper function for updating transitions

Add UpdateTransitions(), by using the same naming rule of UpdateAnimations(),
to wrap the code in StyleContextChanged().

Comment 4

[Boris Chiou [:boris]]

Attachment: Part 2: Cancel all transitions if display:none

Comment 5

[Boris Chiou [:boris]]

Hi, Cameron,

I'm stuck with this problem for a while because I got a problem when setting the ancestor of the target element to be display:none. 
In my patches (you can check part 2), I will cancel all transitions if the target element of transitions is display:none. However, there is still a bug if we set its parent element display:none:
e.g.

#target {
  transition: margin-left 10s;
}

<div id='parent'>
  <div id='target'></div>
</div>

If I set |target| to be display:none, we will restyle the context of this element, so I can cancel all transitions targeting it. However, if I set |parent| to be display:none, we restyle the context of |parent|, instead of the context of |target|, so I cannot cancel all transitions targeting |target|. (Maybe I'm wrong because I just checked my logs in RestyleSelf()). Do you have any advice for this case? Or just add some restyle hints while restyling |parent|? Thanks.

Comment 6

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #5)
> e.g.
> 
> #target {
>   transition: margin-left 10s;
> }
> 
> <div id='parent'>
>   <div id='target'></div>
> </div>
> 
> If I set |target| to be display:none, we will restyle the context of this
> element, so I can cancel all transitions targeting it. However, if I set
> |parent| to be display:none, we restyle the context of |parent|, instead of
> the context of |target|, so I cannot cancel all transitions targeting
> |target|. (Maybe I'm wrong because I just checked my logs in RestyleSelf()).
> Do you have any advice for this case? Or just add some restyle hints while
> restyling |parent|? Thanks.

An alternative way is to try to cancel all transitions of descendants of |parent|, but I have to traverse the subtree of |parent| in nsTransitionManager::StyleContextChanged() when NS_STYLE_IN_DISPLAY_NONE_SUBTREE bit is changed and aNewStyleContext->IsInDisplayNoneSubtree() is true. Fortunately, If we'd not restyle the elements in the subtree of |parent| (and I guess ignoring descendants of display:none elements is ok to us), traversing subtrees of descendants of |parent| could be avoided. However, I'm not sure if the assumption is enough.

Comment 7

[Cameron McCormack (:heycam)]

It would be good to avoid new tree traversal code.  We have code in nsFrame::DestroyFrom that cancels CSS Animations for elements whose frames are being destroyed.  Can we use this to handle Transitions too?

Comment 8

[Boris Chiou [:boris]]

Thanks, Cameron. It's a good hint. I have to check Bug 1197620 first (which stops all animations when disply:none).

Comment 9

[Boris Chiou [:boris]]

Attachment: Part 1: Add StopTransitionsForElement

Add a new API in nsTransitionManager, so we can cancel transitions for a
specific element easily.

Comment 10

[Boris Chiou [:boris]]

Attachment: Part 2: Let AnimationsWithDestroyFrame destroy transitions

We also want to cancel transitions with destroyed frames, so the simplest way
is to extend the ability of AnimationsWithDestroyFrame to cancel transitions as
well.

Comment 11

[Boris Chiou [:boris]]

Attachment: Part 3: Cancel transitions for destroy frames

Add HasCSSTransition() and try to cancel transitions for destroy frames.

Comment 12

[Boris Chiou [:boris]]

Attachment: Part 4: Refactor code in nsTransitionManager::StyleContextChanged()

Comment 13

[Boris Chiou [:boris]]

Attachment: Part 5: Avoid unnecessary transition update if display:none

Comment 14

[Boris Chiou [:boris]]

Attachment: Part 6: Remove unnecessary tests for display:none

Comment 15

[Boris Chiou [:boris]]

Attachment: Part 7: Test

Comment 16

[Boris Chiou [:boris]]

Attachment: Part 7: Test (v2)

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8738923 [details] [diff] [review]
Part 2: Let AnimationsWithDestroyFrame destroy transitions

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

I'm unsure if I have to change the struct name of AnimationsWithDestroyFrame because it also handle transitions. However, I think "Animation" could represent both "CSS Animation" and "CSS Transition", so I keep the original struct name.

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8738922 [details] [diff] [review]
Part 1: Add StopTransitionsForElement

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

Maybe mention in the commit message that it is for cancelling transitions without dispatching the event.

::: layout/style/nsTransitionManager.h
@@ +339,5 @@
>    void SortEvents()      { mEventDispatcher.SortEvents(); }
>    void ClearEventQueue() { mEventDispatcher.ClearEventQueue(); }
>  
> +  // Stop transitions on the element. This method takes the real element
> +  // rather than the element for the generated content for animations on

s/animations/transitions/

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8738923 [details] [diff] [review]
Part 2: Let AnimationsWithDestroyFrame destroy transitions

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

::: layout/base/RestyleManager.h
@@ +266,5 @@
>      // ensure that aRestyleManager lives at least as long as the
>      // object.  (This is generally easy since the caller is typically a
>      // method of RestyleManager.)
>      explicit AnimationsWithDestroyedFrame(RestyleManager* aRestyleManager);
> +    ~AnimationsWithDestroyedFrame() = default;

I think you can just delete this line altogether, yeah?

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8738924 [details] [diff] [review]
Part 3: Cancel transitions for destroy frames

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

In the commit message, s/destroy/destroyed/g.  Also s/try to // since there is no way that we will fail to cancel them.

::: layout/generic/nsFrame.cpp
@@ +704,2 @@
>      // If no new frame for this element is created by the end of the
> +    // restyling process, stop animations or transitions for this frame

s/animations or transitions/animations and transitions/

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8738926 [details] [diff] [review]
Part 4: Refactor code in nsTransitionManager::StyleContextChanged()

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

::: layout/style/nsTransitionManager.h
@@ +351,5 @@
>    typedef nsTArray<RefPtr<mozilla::dom::CSSTransition>>
>      OwningCSSTransitionPtrArray;
>  
> +  bool
> +  UpdateTransitions(const nsStyleDisplay* aDisp,

Can you add a comment above this declaration describing what this helper method does, including how aElementTransitions can get set to null?

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8738928 [details] [diff] [review]
Part 6: Remove unnecessary tests for display:none

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

I think we want to keep tests for transitions not running in display:none subtrees.  Can you keep the tests and make them explicitly test that?

Comment 23

[Boris Chiou [:boris]]

Attachment: Part 1: Add StopTransitionsForElement. (v2, carry heycam's r+)

Add a new API in nsTransitionManager, so we can cancel transitions for a
specific element easily. The purpose of this API is for cancelling transitions
without dispatching the event.

Updated: Fix the commit message and comments.

Comment 24

[Boris Chiou [:boris]]

Attachment: Part 2: Let AnimationsWithDestroyFrame destroy transitions. (v2, carry heycam's r+)

We also want to cancel transitions with destroyed frames, so the simplest way
is to extend the ability of AnimationsWithDestroyFrame to cancel transitions as
well.

Comment 25

[Boris Chiou [:boris]]

Attachment: Part 2: Let AnimationsWithDestroyFrame destroy transitions. (v2, carry heycam's r+)

We also want to cancel transitions with destroyed frames, so the simplest way
is to extend the ability of AnimationsWithDestroyFrame to cancel transitions as
well.

Updated: Remove the destructor.

Comment 26

[Boris Chiou [:boris]]

Attachment: Part 3: Cancel transitions for destroy frames. (v2, carry heycam's r+)

Add HasCSSTransition() and cancel transitions for destroyed frames.

Updated: Fix typos in the commit message and comments.

Comment 27

[Boris Chiou [:boris]]

Attachment: Part 4: Refactor code in nsTransitionManager::StyleContextChanged(). (v2, carry heycam's r+)

Updated: Add a description for UpdateTransitions() and some asserts.

Comment 28

[Boris Chiou [:boris]]

Attachment: Part 6: Revise tests for display:none in test_transitions.html (v2)

Comment 29

[Boris Chiou [:boris]]

Comment on attachment 8739850 [details] [diff] [review]
Part 6: Revise tests for display:none in test_transitions.html (v2)

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

How about this? I revised the tests to make sure the computed value is always the final value between 0~8s, so we can make sure there is no transition for this element. Thanks.

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8739850 [details] [diff] [review]
Part 6: Revise tests for display:none in test_transitions.html (v2)

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

::: layout/style/test/test_transitions.html
@@ +741,4 @@
>      for (var i in display_tests) {
>          var p = display_tests[i];
>  
> +        // There is no transition if we set display:none, so the computed value

Maybe say "if the old or new style is display:none".

Comment 31

[Boris Chiou [:boris]]

Thanks, Cameron. If there is no other concerns (e.g. your discussion in the irc), I will land these patches after getting try success.

Comment 32

[Boris Chiou [:boris]]

Attachment: Part 6: Revise tests for display:none in test_transitions.html. (v3, carry heycam' r+)

Updated: Update the comment.

Comment 33

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d36c99ce317

Comment 34

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #33)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d36c99ce317

try looks good.

Comment 35

[Brian Birtles (:birtles)]

We need to get this edited into the transitions spec. Boris, can you send a mail to www-style?

Comment 36

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 13 April) from comment #35)
> We need to get this edited into the transitions spec. Boris, can you send a
> mail to www-style?

Sure. Is the email address "[e-mail]"? Is there any format for the title? Thanks.

Comment 37

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #36)
> (In reply to Brian Birtles (:birtles, away 13 April) from comment #35)
> > We need to get this edited into the transitions spec. Boris, can you send a
> > mail to www-style?
> 
> Sure. Is the email address "[e-mail]"? Is there any format for the
> title? Thanks.

You'll need to join the list and prefix the subject with [css-transitions].

Guidelines for posting are here: https://wiki.csswg.org/tools/www-style

Comment 38

[Carsten Book [:Tomcat]]

part 4 has problems :

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 6
adding 1182856 to series file
renamed 1182856 -> Bug-1182856---Part-4-Refactor-code-in-nsTransition.patch
applying Bug-1182856---Part-4-Refactor-code-in-nsTransition.patch
patching file layout/style/nsTransitionManager.cpp
Hunk #3 succeeded at 471 with fuzz 2 (offset 9 lines).
Hunk #4 FAILED at 481
1 out of 4 hunks FAILED -- saving rejects to file layout/style/nsTransitionManager.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1182856---Part-4-Refactor-code-in-nsTransition.patch

Comment 39

[Boris Chiou [:boris]]

Attachment: Part 4: Refactor code in nsTransitionManager::StyleContextChanged(). (v2, carry heycam's r+)

Rebased.

Comment 40

[Boris Chiou [:boris]]

Attachment: Part 5: Avoid unnecessary transition update if display:none. (v1, carry heycam's r+)

Rebased.

Comment 41

[Boris Chiou [:boris]]

Rebased part 4 and part 5.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd79b8c38ccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8278f70e662c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60ba2b201ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf35697f257
https://hg.mozilla.org/integration/mozilla-inbound/rev/d456cd95c7bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc53ebd6d2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a627b8fce007

Comment 43

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 13 April) from comment #35)
> We need to get this edited into the transitions spec. Boris, can you send a
> mail to www-style?

https://lists.w3.org/Archives/Public/www-style/2016Apr/0202.html

Comment 44

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #43)
> (In reply to Brian Birtles (:birtles, away 13 April) from comment #35)
> > We need to get this edited into the transitions spec. Boris, can you send a
> > mail to www-style?
> 
> https://lists.w3.org/Archives/Public/www-style/2016Apr/0202.html

Oops. I wrote a wrong title...

Comment 45

[Boris Chiou [:boris]]

An assertion on Android:

https://treeherder.mozilla.org/logviewer.html#?job_id=25602155&repo=mozilla-inbound

Comment 46

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2873e22def14

Comment 47

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=852be8cbd06d

Comment 48

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7203c76d599

Comment 49

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab09426e2c16

Comment 50

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f51ecf7b5770

Comment 51

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #45)
> An assertion on Android:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=25602155&repo=mozilla-
> inbound

Assertion: MOZ_ASSERT(!animation.startTime().IsNull(),
                      "Failed to resolve start time of pending animations"); [1]

This assertion happened before:
"change_duration_and_currenttime:subtree] records after animation restarted - number of records"
in test_animation_observers.html

In the test case, we change the duration of this animation to a longer duration, so we should restart the animation [2], but got this assertion after applying these patches of this bug. I have no idea now, so Cameron, do you have any hint for finding the possible reasons causing this assertion? Thanks.


[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/gfx/layers/composite/AsyncCompositionManager.cpp#568
[2] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/animation/test/chrome/test_animation_observers.html#1548

Comment 52

[Boris Chiou [:boris]]

Remove the ni because it looks like that the assertion is not caused by these patches. (It happened after previous patches.)

However, it might be a potential bug.

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd79b8c38ccb
https://hg.mozilla.org/mozilla-central/rev/8278f70e662c
https://hg.mozilla.org/mozilla-central/rev/c60ba2b201ca
https://hg.mozilla.org/mozilla-central/rev/4cf35697f257
https://hg.mozilla.org/mozilla-central/rev/d456cd95c7bd
https://hg.mozilla.org/mozilla-central/rev/fdc53ebd6d2e
https://hg.mozilla.org/mozilla-central/rev/a627b8fce007

Comment 54

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #52)
> Remove the ni because it looks like that the assertion is not caused by
> these patches. (It happened after previous patches.)
> 
> However, it might be a potential bug.

We can check bug 1264107 for more detail.