1179627 - Implement Animation.id

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This is really just for authors, it doesn't have any functionality. To that extent it seems like we could just rely on expando properties but, as currently specced, it should return an empty string as opposed to undefined when not set. It should also, I suppose, do error-checking on setting to something other than a string.

I wonder if there's a way to do that without adding a string member to every instance of Animation.

Comment 1

[Brian Birtles (:birtles)]

I spoke to heycam about this on IRC and we think it's probably not worth trying to avoid the extra string member. I was curious if there was a means of using the storage of [Cached] attributes to store the string but it seems like the wrapper won't be generated in time for this when constructing Animation objects using the constructor. If this does prove to be something we want to optimize we could have a static hashmap for associating Animations with ids.

Comment 2

[Takayoshi @hikaru__m]

Attachment: test_animation-identify.html

I wrote test file.
See the attachment files.

Comment 3

[Takayoshi @hikaru__m]

Attachment: file_animation-identify.html

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you, white-hawk-taka!
Next time you should upload files as diff files.
See https://developer.mozilla.org/ja/docs/Creating_a_patch

Comment 5

[Takayoshi @hikaru__m]

Attachment: file_animation-identify.html

Comment 6

[Takayoshi @hikaru__m]

Attachment: file_animation-identify.html

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8695636 [details]
file_animation-identify.html

Thanks for looking into this!

Here are a few suggestions for the test.

>test(function(t) {
>  var div = addDive(t);

addDiv

>  div.style.animation = 'anim 100s';

'abc 100s'
(The animation-name part, 'anim' here, should refer to the name of the @keyframes rule)

>  var animation = div.getAnimation()[0];

getAnimations (with an 's' at the end)

>  animation.id = 'anim'
>
>  assert_equals(animation.id, 'anim',
>    'animation.id is the show it');

"animation.id reflects the value set"

We should also add a test for the initial value.

Comment 8

[Takayoshi @hikaru__m]

Attachment: bug1179627.patch

I have fix and make the patch.

Comment 9

[Brian Birtles (:birtles)]

(In reply to white-hawk-taka from comment #8)
> Created attachment 8695731 [details] [diff] [review]
> bug1179627.patch
> 
> I have fix and make the patch.

Looks good!

It would be good to also add a test that animation.id is initially ''.

Once you have done that, please request review on the patch. You can do that when you attach the patch or from the "Details" page later. Set the 'review' section to '?' and then choose either Hiro (type ':hiro' and it will autocomplete his email address) or me (type ':birtles').

Comment 10

[chikoski]

(In reply to white-hawk-taka from comment #8)
> Created attachment 8695731 [details] [diff] [review]
> bug1179627.patch
> 
> I have fix and make the patch.

I have created a patch to implement Animation.id. 

By the way, your patch causes a build error. 
Could you fix it and finish your code?
Or can I do that?

Comment 11

[chikoski]

Attachment: animationIdImpl.patch

This patch implemented Animation.id

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8697304 [details] [diff] [review]
animationIdImpl.patch

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

This looks good but I'd like to have another look after adding the mutation observer notifications.

::: dom/animation/Animation.cpp
@@ +48,5 @@
>  //
>  // ---------------------------------------------------------------------------
>  
>  void
> +Animation::GetId(nsAString& aResult)

I think we could just write GetId these as an inline method in the header file. It would make maintaining this code a little easier.

@@ +56,5 @@
> +
> +void
> +Animation::SetId(const nsAString& aId)
> +{
> +  mId = aId;

I think this needs to notify mutation observers (and we need a test for this too).

i.e. nsNodeUtils::AnimationChanged(this)

We should probably compare if mId and aId are equal as well, in order to prevent redundant notifications.

@@ +57,5 @@
> +void
> +Animation::SetId(const nsAString& aId)
> +{
> +  mId = aId;
> +}  

This line adds trailing whitespace.

::: dom/animation/Animation.h
@@ +59,5 @@
>      , mPendingState(PendingState::NotPending)
>      , mAnimationIndex(sNextAnimationIndex++)
>      , mFinishedAtLastComposeStyle(false)
>      , mIsRelevant(false)
> +    , mFinishedIsResolved(false)      

This line adds trailing white whitespace. Please fix.

@@ +88,5 @@
>      Continue
>    };
>  
>    // Animation interface methods
> +  void GetId(nsAString& aResult);

I think this can be a const method.

::: dom/webidl/Animation.webidl
@@ +14,5 @@
>  
>  [Func="nsDocument::IsWebAnimationsEnabled"]
>  interface Animation : EventTarget {
> +           attribute DOMString          id;
> +  // Bug 1049975: Make 'effect' writeable  

This line adds extra trailing whitespace. Please fix.

This also needs review from a DOM peer (e.g. smaug).

Comment 13

[chikoski]

Attachment: animationIdImpl.patch (updated with review comment)

Thank you for reviewing. 

This new patch includes:

- Animation::SetId modification to call nsNodeUtils::AnimationChanged when Animation.id is changed
- Test code for mutation observer call in Animation::SetId
- removal several trail spaces

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8697354 [details] [diff] [review]
animationIdImpl.patch (updated with review comment)

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

I don't understand how the tests pass without generating an added record.

::: dom/animation/Animation.cpp
@@ +54,5 @@
> +    return;
> +  }
> +  mId = aId;
> +  nsNodeUtils::AnimationChanged(this);
> +}  

Trailing space here.

::: dom/animation/test/chrome/test_animation_observers.html
@@ +802,5 @@
>    childA.remove();
>    childB.remove();
>  });
>  
> +addAsyncAnimTest("chaging id causes mutation observer notification", {}, function*() {

This line is over 80 chars long.

@@ +804,5 @@
>  });
>  
> +addAsyncAnimTest("chaging id causes mutation observer notification", {}, function*() {
> +  // Start an animation that should already be finished.
> +  div.style = "animation: anim 1s;";

The comment doesn't match the code here. Also, why not just have:

 div.style.animation = "anim 1s";

@@ +807,5 @@
> +  // Start an animation that should already be finished.
> +  div.style = "animation: anim 1s;";
> +  
> +  // The animation should cause no Animations to be created.
> +  var animation = div.getAnimations()[0];

The comment doesn't match the code here.

@@ +810,5 @@
> +  // The animation should cause no Animations to be created.
> +  var animation = div.getAnimations()[0];
> +  animations.id = "new id";
> +  assert_records([{ added: [], changed: [animation], removed: []}],
> +                 "records after id is changed");

Shouldn't this create an added record?

@@ +815,5 @@
> +
> +  div.style = "";
> +});
> +
> +addAsyncAnimTest("asigning same value with id does not cause mutation observer notification", {}, function*() {

This line is longer than 80 characters.

@@ +817,5 @@
> +});
> +
> +addAsyncAnimTest("asigning same value with id does not cause mutation observer notification", {}, function*() {
> +  // Start an animation that should already be finished.
> +  div.style = "animation: anim 1s;";

Again, the comment doesn't match the code.

@@ +826,5 @@
> +  assert_records([{ added: [], changed: [animation], removed: []}],
> +                 "records after id is changed");
> +  animations.id = "new id";
> +  assert_records([{ added: [], changed: [], removed: []}],
> +                 "records after assigning same value with id");  

I think we may as well just combine this with the previous test.

@@ +829,5 @@
> +  assert_records([{ added: [], changed: [], removed: []}],
> +                 "records after assigning same value with id");  
> +  div.style = "";
> +});
> +  

A lot of trailing space in this file.

Comment 15

[chikoski]

Attachment: animationIdImpl.patch

Thank you for reviewing. As your comment, my test did not work well. I updated my patch to fix broken test cases.

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8697643 [details] [diff] [review]
animationIdImpl.patch

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

r=me with comments addressed.

adding r?smaug for WebIDL changes

::: dom/animation/test/chrome/test_animation_observers.html
@@ +802,5 @@
>    childA.remove();
>    childB.remove();
>  });
>  
> +addAsyncAnimTest("chaging id", { observe: div, subtree: true }, function*() {

"change_id"

We should move this test inside the big loop that tests observing the div or its parent (starting on line 168). Perhaps right before, "change_currentTime_while_pause_pending", at about line 765.

@@ +814,5 @@
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [animation], removed: []}],
> +                 "records after id is changed");
> +
> +  div.style.animation = "";

We should wait for the removal record too.

@@ +817,5 @@
> +
> +  div.style.animation = "";
> +});
> +
> +addAsyncAnimTest("asigning same value with id", { observe: div, subtree: true }, function*() {

"assigning"

This line is over 80 characters.

@@ +833,5 @@
> +  yield await_frame();
> +  assert_records([],
> +                 "records after assigning same value with id");
> +  div.style.animation = "";
> +});

I think we can combine these two tests into one since the first half is identical.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8697643 [details] [diff] [review]
animationIdImpl.patch

+           attribute DOMString          id;
We seem to not follow the specs whitespace handling anyway here, so just don't
add those extra spaces.

Comment 18

[chikoski]

Attachment: animationIdImpl.patch

r=[e-mail]

Thank you for reviewing. I updated my patch.

> We should move this test inside the big loop that tests observing the div or > its parent (starting on line 168). Perhaps right before, 
> "change_currentTime_while_pause_pending", at about line 765.

I could not find the test you mentioned, so I moved "change_id" to the end of the big loop (L739).

Comment 19

[Hiroyuki Ikezoe (:hiro)]

chiko, you don't need to ask to review anymore because your patch has got review+ by Brian and Olli.

(In reply to chikoski from comment #18)

> > We should move this test inside the big loop that tests observing the div or > its parent (starting on line 168). Perhaps right before, 
> > "change_currentTime_while_pause_pending", at about line 765.
> 
> I could not find the test you mentioned, so I moved "change_id" to the end
> of the big loop (L739).

Your local tree must be older than the current tip.
The position is here.
https://dxr.mozilla.org/mozilla-central/source/dom/animation/test/chrome/test_animation_observers.html#765

Comment 20

[Hiroyuki Ikezoe (:hiro)]

white-hawk-taka, can you please finish attachment 8695731 [details] [diff] [review] up?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8698357 [details] [diff] [review]
animationIdImpl.patch

r+ for the .webidl

Comment 22

[Takayoshi @hikaru__m]

Attachment: bug1179627.patch

This patch has id init test. Please review it.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8699845 [details] [diff] [review]
bug1179627.patch

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

Please include corresponding bug number in commit message like this:
Bug 1179627 - Part 2: Automation tests for animation.id.

::: dom/animation/test/css-animations/file_animation-identify.html
@@ +15,5 @@
> +  var animation = div.getAnimations()[0];
> +
> +  //initial
> +  assert_equals(animation.id, '',
> +    'animation.id has initial value');  

Please remove trailing space.

@@ +20,5 @@
> +
> +  animation.id = 'anim'
> +
> +  assert_equals(animation.id, 'anim',
> +    'animation.id relects the value set');

reflects.

::: dom/animation/test/mochitest.ini
@@ +40,5 @@
>  skip-if = buildapp == 'mulet'
>  support-files = css-animations/file_element-get-animations.html
>  [css-animations/test_timeline-get-animations.html]
> +[css-animations/test_animation-identify.html]
> +support-files = css-animations/file_animation-identify.html

These two lines should not be between test_timeline-get-animations.html and file_timeline-get-animations.html.

Comment 24

[Takayoshi @hikaru__m]

Attachment: bug1179627.patch

Thanks review. I fix it. Please review again.

Comment 25

[chikoski]

Attachment: animationIdImpl.patch

r=[e-mail]
r=[e-mail]

Thank you all. I updated my patch to 

- move added test case("change_id") to appropriate position in test_animation_observer.html
- remove bug fix of mochitest.ini because it will be contained in bug1179627.patch

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8700264 [details] [diff] [review]
bug1179627.patch

r=me with the commit log change as I commented in comment#23.
I'd actually prefer that file name matches to API name, say, file_animation-id.html, but I'd like to hear Brian's opinion.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8700283 [details] [diff] [review]
animationIdImpl.patch

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

chiko, once you got r+, you can set r+ flag by yourself when you attach revised patches.

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1392,1 @@
>  });

Here is an extra line. You should remove it.

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8700264 [details] [diff] [review]
bug1179627.patch

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

As Hiro suggests, please rename this to test_animation-id.html and file_animation-id.html.

r=birtles with comments addressed and the files renamed.

::: dom/animation/test/css-animations/file_animation-identify.html
@@ +15,5 @@
> +  var animation = div.getAnimations()[0];
> +
> +  //initial
> +  assert_equals(animation.id, '',
> +    'animation.id has initial value');

No need for the comment, and I think this fits on one line within 80 characters so you can replace these 3 lines with just:

  assert_equals(animation.id, '', 'id for CSS Animation is initially empty');

@@ +21,5 @@
> +  animation.id = 'anim'
> +
> +  assert_equals(animation.id, 'anim',
> +    'animation.id reflects the value set');
> +  }, 'Returned CSS animations have the correct Animation.id');

'animation.id for CSS Animations'

Comment 29

[Takayoshi @hikaru__m]

Attachment: bug1179627.patch

Thanks review. File name has changed, exclude comment and fix lines under 80 characters.
Please review again.

Comment 30

[Brian Birtles (:birtles)]

Comment on attachment 8700447 [details] [diff] [review]
bug1179627.patch

Looks perfect! Thanks!

By the way, when you receive a comment like "r=me with comments addressed", you can just make the changes and then mark the patch as review+ by yourself. You don't need to request review again unless it has changed a lot or there is something you are uncertain about.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Thank you Shirotaka_Pro and chiko!

I just pushed a try with a bug number in part 1:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8a1e831ee5

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Thanks for the test!  test_animation_observers.html failed on the try.

https://treeherder.mozilla.org/logviewer.html#?job_id=14769756&repo=try

I think we should use longer duration instead of 1s.
chiko, please change the duration and update the patch?

Comment 33

[chikoski]

Attachment: animationIdImpl.patch

Thanks, Hiro.

I updated my patch with your comment.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8701355 [details] [diff] [review]
animationIdImpl.patch

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

Thanks chiko!

I just push a new try with 100s duration.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ade35449e0

Please update your patch again!

::: dom/animation/test/chrome/test_animation_observers.html
@@ +762,5 @@
>                       "records after animation end");
>      });
>    });
>  
> +  addAsyncAnimTest("chage_id", aOptions, function*() {

chage -> change.

@@ +763,5 @@
>      });
>    });
>  
> +  addAsyncAnimTest("chage_id", aOptions, function*() {
> +    e.style.animation = "anim 1s";

You still need to change the duration to 100s to avoid intermittent failures on slower platforms.

Comment 35

[chikoski]

Attachment: part 1 - Implement Animation.id

Thanks Hiro.

I have just updated my patch.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: part 2 - add animation.id for CSS animations test files

Added bug number.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Thank you, everyone!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f40f36d220

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef66db4c4e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/be55d584f00a

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ef66db4c4e9
https://hg.mozilla.org/mozilla-central/rev/be55d584f00a