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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mozilla, Assigned: boris, NeedInfo)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

()

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

4 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

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

Updated

4 years ago
Version: 41 Branch → Trunk
Assignee

Updated

3 years ago
Assignee: nobody → boris.chiou
Assignee

Updated

3 years ago
See Also: → 962594
Assignee

Comment 3

3 years ago
Add UpdateTransitions(), by using the same naming rule of UpdateAnimations(),
to wrap the code in StyleContextChanged().
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: → 1197620
Assignee

Comment 9

3 years ago
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
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
Add HasCSSTransition() and try to cancel transitions for destroy frames.
Comment hidden (obsolete)
Assignee

Comment 16

3 years ago
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
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
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
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
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

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

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

Updated

3 years ago
Attachment #8739842 - Attachment is obsolete: true
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

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