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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mozilla, Assigned: boris, NeedInfo)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla48
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(URL)

Attachments

(7 attachments, 12 obsolete attachments)

3.61 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.19 KB, patch
boris
: review+
Details | Diff | Splinter Review
5.52 KB, patch
boris
: review+
Details | Diff | Splinter Review
2.69 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.87 KB, patch
boris
: review+
Details | Diff | Splinter Review
12.17 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.48 KB, patch
boris
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
Flags: needinfo?(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

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

Updated

3 years ago
Version: 41 Branch → Trunk
(Assignee)

Updated

3 years ago
Assignee: nobody → boris.chiou
(Assignee)

Updated

3 years ago
See Also: → bug 962594
(Assignee)

Comment 3

3 years ago
Created attachment 8737498 [details] [diff] [review]
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().
(Assignee)

Comment 4

3 years ago
Created attachment 8737499 [details] [diff] [review]
Part 2: Cancel all transitions if display:none
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
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.
Flags: needinfo?(cam)
(Assignee)

Comment 6

3 years ago
(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.
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?
Flags: needinfo?(cam)
(Assignee)

Comment 8

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

Updated

3 years ago
See Also: → bug 1197620
(Assignee)

Comment 9

3 years ago
Created attachment 8738922 [details] [diff] [review]
Part 1: Add StopTransitionsForElement

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

Updated

3 years ago
Attachment #8737498 - Attachment is obsolete: true
Attachment #8737499 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8738923 [details] [diff] [review]
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.
(Assignee)

Comment 11

3 years ago
Created attachment 8738924 [details] [diff] [review]
Part 3: Cancel transitions for destroy frames

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

Comment 12

3 years ago
Created attachment 8738926 [details] [diff] [review]
Part 4: Refactor code in nsTransitionManager::StyleContextChanged()
(Assignee)

Comment 13

3 years ago
Created attachment 8738927 [details] [diff] [review]
Part 5: Avoid unnecessary transition update if display:none
(Assignee)

Comment 14

3 years ago
Created attachment 8738928 [details] [diff] [review]
Part 6: Remove unnecessary tests for display:none
Comment hidden (obsolete)
(Assignee)

Comment 16

3 years ago
Created attachment 8738934 [details] [diff] [review]
Part 7: Test (v2)
(Assignee)

Updated

3 years ago
Attachment #8738929 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8738922 - Flags: review?(cam)
(Assignee)

Comment 17

3 years ago
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.
Attachment #8738923 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8738924 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8738926 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8738927 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8738928 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8738934 - Flags: review?(cam)
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/
Attachment #8738922 - Flags: review?(cam) → review+
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?
Attachment #8738923 - Flags: review?(cam) → review+
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/
Attachment #8738924 - Flags: review?(cam) → review+
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?
Attachment #8738926 - Flags: review?(cam) → review+
Attachment #8738927 - Flags: review?(cam) → review+
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?
Attachment #8738928 - Flags: review?(cam)
Attachment #8738934 - Flags: review?(cam) → review+
(Assignee)

Comment 23

3 years ago
Created attachment 8739838 [details] [diff] [review]
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.
Attachment #8739838 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8738922 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8738923 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8739840 [details] [diff] [review]
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.
Attachment #8739840 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8739839 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8739841 [details] [diff] [review]
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.
Attachment #8739841 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8738924 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8739842 [details] [diff] [review]
Part 4: Refactor code in nsTransitionManager::StyleContextChanged(). (v2, carry heycam's r+)

Updated: Add a description for UpdateTransitions() and some asserts.
Attachment #8739842 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8738926 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8739850 [details] [diff] [review]
Part 6: Revise tests for display:none in test_transitions.html (v2)
(Assignee)

Updated

3 years ago
Attachment #8738928 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
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.
Attachment #8739850 - Flags: review?(cam)
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".
Attachment #8739850 - Flags: review?(cam) → review+
(Assignee)

Comment 31

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

Comment 32

3 years ago
Created attachment 8739859 [details] [diff] [review]
Part 6: Revise tests for display:none in test_transitions.html. (v3, carry heycam' r+)

Updated: Update the comment.
Attachment #8739859 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8739850 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1263534
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 34

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

try looks good.
We need to get this edited into the transitions spec. Boris, can you send a mail to www-style?
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 36

3 years ago
(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 "www-style@w3.org"? Is there any format for the title? Thanks.
Flags: needinfo?(boris.chiou) → needinfo?(bbirtles)
(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 "www-style@w3.org"? 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
Flags: needinfo?(bbirtles)
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
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
(Assignee)

Comment 39

3 years ago
Created attachment 8740314 [details] [diff] [review]
Part 4: Refactor code in nsTransitionManager::StyleContextChanged(). (v2, carry heycam's r+)

Rebased.
Attachment #8740314 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8739842 - Attachment is obsolete: true
(Assignee)

Comment 40

3 years ago
Created attachment 8740316 [details] [diff] [review]
Part 5: Avoid unnecessary transition update if display:none. (v1, carry heycam's r+)

Rebased.
Attachment #8740316 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8738927 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
Rebased part 4 and part 5.
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 43

3 years ago
(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
(Assignee)

Comment 44

3 years ago
(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...
(Assignee)

Comment 51

3 years ago
(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
(Assignee)

Updated

3 years ago
Flags: needinfo?(cam)
(Assignee)

Comment 52

3 years ago
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.
Flags: needinfo?(cam)
(Reporter)

Updated

3 years ago
Blocks: 1264125
(Assignee)

Comment 54

3 years ago
(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.
(Reporter)

Updated

2 years ago
Blocks: 1230801
You need to log in before you can comment on or make changes to this bug.