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
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
There is also a W3C CSS Test for the spec'd behavior that is awaiting review (hint, hint)
Version: 41 Branch → Trunk
Assignee: nobody → boris.chiou
See Also: → 962594
Add UpdateTransitions(), by using the same naming rule of UpdateAnimations(),
to wrap the code in StyleContextChanged().
Status: NEW → ASSIGNED
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)
(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)
Thanks, Cameron. It's a good hint. I have to check Bug 1197620 first (which stops all animations when disply:none).
See Also: → 1197620
Add a new API in nsTransitionManager, so we can cancel transitions for a
specific element easily.
Attachment #8737498 - Attachment is obsolete: true
Attachment #8737499 - Attachment is obsolete: true
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.
Add HasCSSTransition() and try to cancel transitions for destroy frames.
Attachment #8738929 - Attachment is obsolete: true
Attachment #8738922 - Flags: review?(cam)
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)
Attachment #8738924 - Flags: review?(cam)
Attachment #8738926 - Flags: review?(cam)
Attachment #8738927 - Flags: review?(cam)
Attachment #8738928 - Flags: review?(cam)
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+
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+
Attachment #8738922 - Attachment is obsolete: true
Attachment #8738923 - Attachment is obsolete: true
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+
Attachment #8739839 - Attachment is obsolete: true
Add HasCSSTransition() and cancel transitions for destroyed frames.

Updated: Fix typos in the commit message and comments.
Attachment #8739841 - Flags: review+
Attachment #8738924 - Attachment is obsolete: true
Updated: Add a description for UpdateTransitions() and some asserts.
Attachment #8739842 - Flags: review+
Attachment #8738926 - Attachment is obsolete: true
Attachment #8738928 - Attachment is obsolete: true
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+
Thanks, Cameron. If there is no other concerns (e.g. your discussion in the irc), I will land these patches after getting try success.
Attachment #8739850 - Attachment is obsolete: true
Blocks: 1263534
Keywords: checkin-needed
(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)
(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
Attachment #8739842 - Attachment is obsolete: true
Attachment #8738927 - Attachment is obsolete: true
Rebased part 4 and part 5.
Flags: needinfo?(boris.chiou)
(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
(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...
(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
Flags: needinfo?(cam)
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)
Blocks: 1264125
(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.
Blocks: 1230801
You need to log in before you can comment on or make changes to this bug.