Closed Bug 1353987 Opened 6 years ago Closed 6 years ago

cancel() should create a new resolved ready promise

Categories

(Core :: DOM: Animation, enhancement, P3)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

Details

Attachments

(1 file)

Bug 1353987 - Clear ready promise when animation is canceled.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
According to the spec, when we cancel an animation we should "reset an animation's pending tasks"[1] which has the following steps:

  ...
  4. Reject animation’s current ready promise with a DOMException named
     "AbortError".
  5. Let animation’s current ready promise be the result of creating a new resolved
     Promise object.

However, in our implementation we simply have:

  void
  Animation::ResetPendingTasks()
  {
    ...
    if (mReady) {
      mReady->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
    }
  }

Since we create the ready promise lazily and, if neededn resolve it when we create it (see Animation::GetReady) perhaps all we need to do here is set mReady = nullptr;

Fortunately we're not shipping the ready promise yet so we don't need to uplift this.

Note that there is a test as part of https://github.com/w3c/web-platform-tests/pull/3330 that fails due to this. We might want to wait for that test for be merged to m-c or simply merge it manually as part of this bug.

[1] https://w3c.github.io/web-animations/#reset-an-animations-pending-tasks
Assignee: nobody → mantaroh
The test for this has now been merged into mozilla central:

  testing/web-platform/tests/web-animations/timing-model/animations/canceling-an-animation.html
Comment on attachment 8874722 [details]
Bug 1353987 - Clear ready promise when animation is canceled.

https://reviewboard.mozilla.org/r/146030/#review150022

::: commit-message-2c628:1
(Diff revision 1)
> +Bug 1353987 - Set null to pending promise when animation is canceled in order to follow the spec. r?birtles

For the commit message, how about the following:


Clear ready promise when animation is canceled

According to the spec, when we cancel an animation we should "reset an animation's pending tasks"[1] which has the following steps:

  ...
  4. Reject animation’s current ready promise with a DOMException named
     "AbortError".
  5. Let animation’s current ready promise be the result of creating a new resolved
     Promise object.

Since we we create the ready promise lazily and, if needed resolve it when we create it (see Animation::GetReady), this patch simply clears the ready promise when the animation is canceled.

[1] https://w3c.github.io/web-animations/#reset-an-animations-pending-tasks
Attachment #8874722 - Flags: review?(bbirtles) → review+
Comment on attachment 8874722 [details]
Bug 1353987 - Clear ready promise when animation is canceled.

https://reviewboard.mozilla.org/r/146030/#review150022

Thanks, Brian.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Comment on attachment 8874722 [details]
> Bug 1353987 - Clear ready promise when animation is canceled.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/146030/diff/2-3/

Rebaed.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 74d6e1488e98 -d 19bc87470994: rebasing 400979:74d6e1488e98 "Bug 1353987 - Clear ready promise when animation is canceled. r=birtles" (tip)
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8047b74cb022
Clear ready promise when animation is canceled. r=birtles
https://hg.mozilla.org/mozilla-central/rev/8047b74cb022
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.