Closed Bug 1067701 Opened 9 years ago Closed 9 years ago

Implement Animation.target

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

      No description provided.
Blocks: 1033114
Attached patch PatchSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #8489753 - Flags: review?(birtles)
Comment on attachment 8489753 [details] [diff] [review]
Patch

Review of attachment 8489753 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with feedback addressed

::: dom/animation/Animation.h
@@ +164,5 @@
> +  }
> +
> +  void SetTarget(Element* aTarget) {
> +    mTarget = aTarget;
> +  }

I'd rather we left this out and tackled it in a separate bug. The steps involved in correctly switching targets are pretty complicated. For a start, we need to make sure the animation effect gets cleared from the old target. Then we also need to do all these steps:

  http://w3c.github.io/web-animations/#setting-the-source-content

Just make the IDL readonly and file a bug for making it writeable.

::: dom/animation/test/css-integration/test_element-get-animation-players.html
@@ +41,5 @@
>    var players = div.getAnimationPlayers();
>    assert_equals(players.length, 1,
>      'getAnimationPlayers returns a player running CSS Animations');
> +  assert_equals(players[0].source.target, div,
> +    'Animation.target is the animatable div');

We try to keep these tests as targeted as possible or at least that's the feedback I got when I submitted some tests to web-platform-tests. So I'd prefer if we just factored out another test method just to check this.

::: dom/webidl/Animation.webidl
@@ +15,5 @@
>    // FIXME: |effect| should have type (AnimationEffect or EffectCallback)?
>    // but we haven't implemented EffectCallback yet.
> +  [Cached,Pure]
> +  readonly attribute AnimationEffect? effect;
> +           attribute Element?         target;

As mentioned, make target readonly and add a FIXME/bug reference.
Attachment #8489753 - Flags: review?(birtles) → review+
Blocks: 998245
Attached patch Updated patch (obsolete) — Splinter Review
Hi David, I went ahead and applied my review feedback to your patch because I'm building a lot of stuff that depends on this patch.

If this looks good to you, feel free to transfer my r+ and land it.
Comment on attachment 8497950 [details] [diff] [review]
Updated patch

Boris, would you be able to review the WebIDL parts of this patch?

(It's David's patch, but I incorporated my own review feedback since I want to land it soon. I'll let David check those changes, but I'd like to get your review of this version of the patch since the WebIDL parts of this patch are slightly different to the one I reviewed.)
Attachment #8497950 - Flags: review?(bzbarsky)
Comment on attachment 8497950 [details] [diff] [review]
Updated patch

r=me, but the pseudotype version of GetTarget doesn't seem relevant to this patch.
Attachment #8497950 - Flags: review?(bzbarsky) → review+
Attached patch Updated patch (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8497950 [details] [diff] [review]
> Updated patch
> 
> r=me, but the pseudotype version of GetTarget doesn't seem relevant to this
> patch.

Removed. I'll add it in a later bug where it's needed.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6a83f86c666b
Attachment #8497950 - Attachment is obsolete: true
Attached patch Updated patchSplinter Review
Got the initialization order wrong in the previous patch (and MSVC doesn't care about this).

Running this through try with some other changesets that will fail a few newly added tests:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f9fb59908912
Attachment #8497968 - Attachment is obsolete: true
Comment on attachment 8497999 [details] [diff] [review]
Updated patch

Review of attachment 8497999 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/Animation.h
@@ +169,5 @@
> +    // returns animations targetting Element so we should this should never
> +    // be called for an animation that targets a pseudo-element.
> +    MOZ_ASSERT(mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement,
> +               "Requesting the target of an Animation that targets a"
> +               " pseudo-element is not yet supported.");

So this is OK because we will call GetTarget from Play() (for restyling) but not from PlayInternal, right?

::: dom/webidl/Animation.webidl
@@ +15,5 @@
>    // FIXME: |effect| should have type (AnimationEffect or EffectCallback)?
>    // but we haven't implemented EffectCallback yet.
> +  [Cached,Pure]
> +  readonly attribute AnimationEffect? effect;
> +  // FIXME: This should be writeable

reference bug 1067769?
Attachment #8497999 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/28519d825a23
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.