1353987 - cancel() should create a new resolved ready promise

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1353987 - Clear ready promise when animation is canceled.

Review commit: https://reviewboard.mozilla.org/r/146030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146030/

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

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/1-2/

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8874722 [details]
Bug 1353987 - Clear ready promise when animation is canceled.

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

Thanks, Brian.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

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/

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(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.

Comment 8

[Mozilla Autoland]

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)

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

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/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8047b74cb022
Clear ready promise when animation is canceled. r=birtles

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8047b74cb022