transitioncancel is fired after transition has already fired transitionend

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: birtles, Assigned: mantaroh)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
Created attachment 8833863 [details]
Test case

According to CSS transitions, only running transitions are cancelled yet in the attached test we fire a transitioncancel event for a finished transition.

Admittedly, in CSS transitions level 2[1] this is less than clear where, "not idle or after → idle" should probably be written "not idle and not after → idle".

Basically this code fails to check for the after state:

  // Handle cancel events firts
  if (mPreviousTransitionPhase != TransitionPhase::Idle &&
      currentPhase == TransitionPhase::Idle) {
    TimeStamp activeTimeStamp = ElapsedTimeToTimeStamp(aActiveTime);
    events.AppendElement(TransitionEventParams{ eTransitionCancel,
                                                aActiveTime,
                                                activeTimeStamp });
  }

While we're at it can we should also fix the following nits:

1. s/firts/first/ in the comment above

2. One unnecessary space before = in:

  TimeStamp zeroTimeStamp   = AnimationTimeToTimeStamp(zeroDuration);
  TimeStamp startTimeStamp  = ElapsedTimeToTimeStamp(intervalStartTime);
  TimeStamp endTimeStamp    = ElapsedTimeToTimeStamp(intervalEndTime);

3. The following lines are not needed:

    intervalStartTime = zeroDuration;
    intervalEndTime   = zeroDuration;

[1] https://drafts.csswg.org/css-transitions-2/#event-dispatch
(Reporter)

Comment 1

8 months ago
Marking this as P1 since we really shouldn't ship transitioncancel with this bug.
Priority: -- → P1
(Assignee)

Comment 2

8 months ago
Thanks Brian,

Sorry, I misled the spec.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deddb0ab91aa2429f6ff8d91205ae86d3e3f3ce9
Comment hidden (mozreview-request)
(Reporter)

Comment 4

8 months ago
mozreview-review
Comment on attachment 8833875 [details]
Bug 1336899 - Should not fire the transitioncancel after ending CSS Transition.

https://reviewboard.mozilla.org/r/109982/#review110986

Thanks!
Attachment #8833875 - Flags: review?(bbirtles) → review+

Comment 5

8 months ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1895e4ebd7e8
Should not fire the transitioncancel after ending CSS Transition. r=birtles
(Reporter)

Comment 6

8 months ago
Updated spec to make this clearer: https://github.com/w3c/csswg-drafts/commit/d6695bb6f4f185ad12f6977aac6a07932e68800d

Comment 7

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1895e4ebd7e8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.