Closed
Bug 1067769
Opened 11 years ago
Closed 9 years ago
Allow setting KeyframeEffect.target
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: dzbarsky, Assigned: boris)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(15 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
No description provided.
Reporter | ||
Updated•11 years ago
|
Blocks: web-animations
Updated•10 years ago
|
No longer blocks: web-animations
Updated•10 years ago
|
Summary: Allow setting Animation.target → Allow setting KeyframeEffect.target
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Comment 1•9 years ago
|
||
I'd like to split this bug into three parts:
1. Support nullable target
* Part of our code handles null target properly, so I have to make sure the other part does as well.
2. Allow setting KeyframeEffect.target
* Support this API, as the bug title says.
3. Test cases.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 5•9 years ago
|
||
Hi, Brian,
Part 1 ~ Part 3 are only for supporting null target. I'm still working on setting target, but I think it's better to review them first. Thanks.
Comment 6•9 years ago
|
||
Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles
https://reviewboard.mozilla.org/r/46387/#review43253
r=me with comments addressed.
::: dom/animation/KeyframeEffect.cpp:767
(Diff revision 1)
> + // Use doc as the parent if targetElement is null.
> RefPtr<KeyframeEffectType> effect =
> - new KeyframeEffectType(targetElement->OwnerDoc(), targetElement,
> - pseudoType, timingParams);
> + new KeyframeEffectType(targetElement ? targetElement->OwnerDoc() : doc,
> + targetElement, pseudoType, timingParams);
This obviously needs to be specced [1] but I think it's probably best to just use |doc| here. At least that makes sense to me that the KeyframeEffect has a consistent global regardless of the target element used (or lack of target element) when it is initialized.
[1] https://github.com/w3c/web-animations/issues/145
Attachment #8741608 -
Flags: review?(bbirtles) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8741609 [details]
MozReview Request: Bug 1067769 - Part 1: Avoid doing RequestRestyle and mutation batch for null target. r=birtles
https://reviewboard.mozilla.org/r/46389/#review43257
r=me but I really want to know why that assertion is valid (and be sure we can never be in a situation where that might not hold)
::: dom/animation/KeyframeEffect.cpp:577
(Diff revision 1)
> }
> }
>
> if (mAnimation) {
> nsPresContext* presContext = GetPresContext();
> - if (presContext) {
> + if (presContext && mTarget) {
Why not move this test up to where we check mAnimation?
Or, perhaps better still, move this whole block beginning with "if (mAnimation) {" inside the "if (mTarget) {" block above?
::: dom/animation/KeyframeEffect.cpp:1125
(Diff revision 1)
> }
>
> bool
> KeyframeEffectReadOnly::CanThrottleTransformChanges(nsIFrame& aFrame) const
> {
> + MOZ_ASSERT(mTarget);
We should have a comment or assertion description saying why we think this invariant holds.
::: dom/animation/KeyframeEffect.cpp:1382
(Diff revision 1)
>
> void KeyframeEffect::NotifySpecifiedTimingUpdated()
> {
> // Use the same document for a pseudo element and its parent element.
> - nsAutoAnimationMutationBatch mb(mTarget->OwnerDoc());
> + // Use nullptr if we don't have mTarget, so disable the mutation batch.
> + nsAutoAnimationMutationBatch mb(mTarget ? mTarget->OwnerDoc() : nullptr);
(This is a somewhat minor point, but the order of these patches is not quite right. In part 1 we stop throwing when null is passed into the constructor--that means mTarget could be nullptr. If we just apply part 1 and this method gets called, we will end up crashing.
Sometimes it can be quite difficult, but as far as possible we should try to make each patch able to be applied in turn and compile, pass all tests, and not crash.
So in this bug it would be better if we made this change first, and only after it is safe for mTarget to be nullptr, update the constructor to allow it.
It's a minor point but it is a good goal in future. It makes bisecting easier, for example.)
Attachment #8741609 -
Flags: review?(bbirtles) → review+
Updated•9 years ago
|
Attachment #8741610 -
Flags: review?(bbirtles)
Comment 8•9 years ago
|
||
Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles
https://reviewboard.mozilla.org/r/46435/#review43265
This is good but I want to just check the test for the finished promise and the test for the finish event.
We should probably also add a test that an animation observer record is *not* created for a new animation with a null target.
::: testing/web-platform/tests/web-animations/animation/constructor.html:67
(Diff revision 1)
> + anim.currentTime += 2500;
> + assert_equals(effect.getComputedTiming().progress, 0.25);
> + anim.currentTime += 2500;
> + assert_equals(effect.getComputedTiming().progress, 0.5);
I don't think that checking at both 0.25 and 0.5 adds any value to this test. Just checking at 0.5 would be fine.
We should also test that the finished promise is resolved and finish events are fired since that will be the primary way in which effects with a null target are used: as a means of synchronization.
::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:712
(Diff revision 1)
> + { left: ["10px", "20px"] },
> + { duration: 10000,
> + fill: "forwards" });
> + assert_true(!!effect, "Create effect with null target successfully");
> +
> + var anim = new Animation(effect, document.timeline);
We need to actually play() the animation before it would be even considered by document.getAnimations().
Comment 9•9 years ago
|
||
Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles
https://reviewboard.mozilla.org/r/46387/#review43269
::: dom/animation/KeyframeEffect.cpp:749
(Diff revision 1)
> return nullptr;
> }
>
> - if (aTarget.IsNull()) {
> - // We don't support null targets yet.
> - aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
> + RefPtr<Element> targetElement;
> + CSSPseudoElementType pseudoType = CSSPseudoElementType::NotPseudo;
> + if (!aTarget.IsNull()) {
We also need to update dom/base/domerr.msg and xpcom/base/ErrorList.h to drop NS_ERROR_DOM_ANIM_NO_TARGET_ERR.
Attachment #8741608 -
Flags: review+
Comment 10•9 years ago
|
||
Oh I don't understand why MozReview is so hard to use. I just wanted to add a comment and it cleared the review flag... Anyway part 1 is still r+
Assignee | ||
Comment 11•9 years ago
|
||
https://reviewboard.mozilla.org/r/46387/#review43253
> This obviously needs to be specced [1] but I think it's probably best to just use |doc| here. At least that makes sense to me that the KeyframeEffect has a consistent global regardless of the target element used (or lack of target element) when it is initialized.
>
> [1] https://github.com/w3c/web-animations/issues/145
Got it. I will |doc| directly.
Assignee | ||
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/46387/#review43269
> We also need to update dom/base/domerr.msg and xpcom/base/ErrorList.h to drop NS_ERROR_DOM_ANIM_NO_TARGET_ERR.
Got it. I will update them soon.
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/46389/#review43257
> Why not move this test up to where we check mAnimation?
>
> Or, perhaps better still, move this whole block beginning with "if (mAnimation) {" inside the "if (mTarget) {" block above?
Yes, it's much better.
> We should have a comment or assertion description saying why we think this invariant holds.
Indeed, if mTarget is nullptr, we will not call CanThrottleTransformChanges() from CanThrottle(). However, I think it is a fuction block and we should let people know not to call this if your mTarget is nullptr in the future. In the current implementation, we can drop this assertion, so I will remove it.
> (This is a somewhat minor point, but the order of these patches is not quite right. In part 1 we stop throwing when null is passed into the constructor--that means mTarget could be nullptr. If we just apply part 1 and this method gets called, we will end up crashing.
>
> Sometimes it can be quite difficult, but as far as possible we should try to make each patch able to be applied in turn and compile, pass all tests, and not crash.
>
> So in this bug it would be better if we made this change first, and only after it is safe for mTarget to be nullptr, update the constructor to allow it.
>
> It's a minor point but it is a good goal in future. It makes bisecting easier, for example.)
Yes, you are right. I'd change the order of part 1 and part 2, so they will be better.
Assignee | ||
Comment 14•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review43265
> I don't think that checking at both 0.25 and 0.5 adds any value to this test. Just checking at 0.5 would be fine.
>
> We should also test that the finished promise is resolved and finish events are fired since that will be the primary way in which effects with a null target are used: as a means of synchronization.
Got it. I will add one more test for it.
> We need to actually play() the animation before it would be even considered by document.getAnimations().
OK. I forgot to play it.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Updated•9 years ago
|
Attachment #8741608 -
Flags: review?(bbirtles) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles
https://reviewboard.mozilla.org/r/46387/#review43529
Comment 19•9 years ago
|
||
Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles
https://reviewboard.mozilla.org/r/46435/#review43531
Looking good but I'd like to have a quick look at web-platform/tests/web-animations/document/getAnimations.html before we land this.
::: dom/animation/test/chrome/test_animation_observers.html:1703
(Diff revision 2)
> +addAsyncAnimTest("create_animation_without_target",
> + { observe: document, subtree: true }, function*() {
> + var effect = new KeyframeEffectReadOnly(null,
> + { opacity: [ 0, 1 ] },
> + { duration: 10000 });
> + var anim = new Animation(effect, document.timeline);
You need to play the animation before we'd consider dispatching an "added" record for it.
::: dom/animation/test/chrome/test_animation_observers.html:1706
(Diff revision 2)
> + { opacity: [ 0, 1 ] },
> + { duration: 10000 });
> + var anim = new Animation(effect, document.timeline);
> +
> + yield await_frame();
> + assert_records([], "no records after animation is added");
After adding the call to play(), we should probably cancel the animation here and add a check that we don't get a removed record.
::: testing/web-platform/tests/web-animations/animation/finish.html:222
(Diff revision 2)
> + return animation.ready.then(function() {
> + animation.finish();
> + }).then(function() {
This is probably fine, but maybe we should have a separate test for when the animation finishes normally?
The reason is that it's quite possible that an implementation would successfully resolve the finished promise when finish() is called but not during a normal tick.
So perhaps we could add a test that:
* sets the currentTime to animation.effect.getComputedTiming().endTime - 1
* waits 1 frame (or maybe 2 frames? I can't remember the exact sequencing of when the finished state is evaluated, promise callbacks are run, and rAF callbacks are run---there's probably a similar test elsewhere in this file we can follow, though)
* checks that the animation has finished
::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:710
(Diff revision 2)
> test(function(t) {
> + var effect = new KeyframeEffectReadOnly(null,
> + { left: ["10px", "20px"] },
> + { duration: 10000,
> + fill: "forwards" });
> + assert_true(!!effect, "Create effect with null target successfully");
I don't think the constructor will ever return null without throwing so I don't know that this test makes sense.
How about we just add a test like the following:
assert_equals(effect.target, null, "Effect created with null target has correct target");
::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:712
(Diff revision 2)
> + var anim = new Animation(effect, document.timeline);
> + anim.play();
> + assert_equals(document.getAnimations().length, 0,
> + "document.getAnimations() doesn't return animations with " +
> + "null target");
We should make this a separate test in a new file:
web-platform/tests/web-animations/document/getAnimations.html
and put this test there (plus probably adding a few simple tests similar to those in dom/animation/test/css-animations/file_document-get-animations.html).
Attachment #8741610 -
Flags: review?(bbirtles)
Assignee | ||
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review43531
> This is probably fine, but maybe we should have a separate test for when the animation finishes normally?
>
> The reason is that it's quite possible that an implementation would successfully resolve the finished promise when finish() is called but not during a normal tick.
>
> So perhaps we could add a test that:
>
> * sets the currentTime to animation.effect.getComputedTiming().endTime - 1
> * waits 1 frame (or maybe 2 frames? I can't remember the exact sequencing of when the finished state is evaluated, promise callbacks are run, and rAF callbacks are run---there's probably a similar test elsewhere in this file we can follow, though)
> * checks that the animation has finished
OK. I will also add this separate test for null target.
> We should make this a separate test in a new file:
>
> web-platform/tests/web-animations/document/getAnimations.html
>
> and put this test there (plus probably adding a few simple tests similar to those in dom/animation/test/css-animations/file_document-get-animations.html).
OK. I agree. Let me add web-platform/tests/web-animations/document/getAnimations.html.
Assignee | ||
Comment 21•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review43531
> OK. I will also add this separate test for null target.
BTW, it should wait 2 frames in accordance with other similar and my tests. 1 frame is not enough. Thanks.
Assignee | ||
Comment 22•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review43531
> OK. I agree. Let me add web-platform/tests/web-animations/document/getAnimations.html.
I will add the test for null target in document/getAniamtions.html in this patch. For other similar tests, from dom/animations/test/css-animations, I prefer to use another patch (i.e. part 4) to make my purpose clearer. Thanks.
Assignee | ||
Comment 23•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47113/
Attachment #8741608 -
Attachment description: MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r?birtles → MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles
Attachment #8742299 -
Flags: review?(bbirtles)
Attachment #8741610 -
Flags: review?(bbirtles)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 27•9 years ago
|
||
Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles
https://reviewboard.mozilla.org/r/46435/#review44011
r=birtles with comments addressed
::: testing/web-platform/tests/web-animations/animation/finish.html:238
(Diff revisions 2 - 3)
> + resolvedFinished = true;
> + });
> +
> + return animation.ready.then(function() {
> + animation.currentTime = animation.effect.getComputedTiming().endTime - 1;
> + animation.play();
We don't need this line, right?
::: testing/web-platform/tests/web-animations/document/getAnimations.html:5
(Diff revision 3)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>document.getAnimations tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-document-getanimations">
> +<link rel="author" title="Boris Chiou" href="boris.chiou@gmail.com">
We're not adding author lines anymore.
Attachment #8741610 -
Flags: review?(bbirtles) → review+
Comment 28•9 years ago
|
||
Comment on attachment 8742299 [details]
MozReview Request: Bug 1067769 - Part 4: Add some simple tests for document.getAnimation() in wpt. r=birtles
https://reviewboard.mozilla.org/r/47113/#review44023
r=birtles with comments addressed
::: testing/web-platform/tests/web-animations/document/getAnimations.html:31
(Diff revision 1)
> + assert_equals(document.getAnimations().length, 1,
> + 'getAnimation returns a running animation');
> +
> + var anim2 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> + assert_equals(document.getAnimations().length, 2,
> + 'getAnimation returns running animations');
We should add a test here for the order. (In fact, that probably more important than the test on line 26-27 for document.getAnimations().length == 1. I think we could probably drop that.)
e.g.
var anim1 = div.animate(...);
var anim2 = div.animate(...);
assert_array_equals(document.getAnimations(),
[ anim1, anim2 ],
'getAnimations() returns running animations');
::: testing/web-platform/tests/web-animations/document/getAnimations.html:37
(Diff revision 1)
> +
> + anim1.finish();
> + anim2.finish();
> + assert_equals(document.getAnimations().length, 0,
> + 'getAnimation only returns running animations');
> +}, 'Test document.getAnimations for script generated animations')
Nit: script-generated
::: testing/web-platform/tests/web-animations/document/getAnimations.html:39
(Diff revision 1)
> +test(function(t) {
> + var div = createDiv(t);
> + var anim1 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> + var anim2 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> + var anims = document.getAnimations();
> + assert_equals(anims[0], anim1, 'first generated animation');
> + assert_equals(anims[1], anim2, 'second generated animation');
> +}, 'Test the order of document.getAnimations with script generated animations')
Oh, I see you are testing the order here! I think we could probably simplify this as suggested above and make it the one test.
Attachment #8742299 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 29•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review44011
> We don't need this line, right?
After setting currentTime, the playState is still "paused", so I found I have to play it explicity, or we will not receive finished event.
> We're not adding author lines anymore.
Sure. I will remove it.
Assignee | ||
Comment 30•9 years ago
|
||
https://reviewboard.mozilla.org/r/47113/#review44023
> We should add a test here for the order. (In fact, that probably more important than the test on line 26-27 for document.getAnimations().length == 1. I think we could probably drop that.)
>
> e.g.
>
> var anim1 = div.animate(...);
> var anim2 = div.animate(...);
>
> assert_array_equals(document.getAnimations(),
> [ anim1, anim2 ],
> 'getAnimations() returns running animations');
OK. I will drop line 26-27, and simplify the test of order according to your suggestion. Thanks.
Comment 31•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review44011
> After setting currentTime, the playState is still "paused", so I found I have to play it explicity, or we will not receive finished event.
Oh right, we never played it to begin with. We should probably just play it when we create the animation.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 34•9 years ago
|
||
https://reviewboard.mozilla.org/r/46435/#review44011
> Oh right, we never played it to begin with. We should probably just play it when we create the animation.
OK. I will move the play() after creation.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 37•9 years ago
|
||
Hi Brian,
I have a problem about the state of the old target after setting a new target for a specific effect.
e.g.
var anim = div.animate({ 'marginLeft': ['0px', '100px'] }, 4000);
anim.play();
anim.ready.then(function() {
anim.currentTime = 3000;
console.log(getComputedStyle(div).marginLeft); // restyle, and the output is '75px'
anim.effect.target = null; // set target to null
// what is the expected value or marginLeft now?
});
I checked the spec and I didn't see the expected behavior of setting an effect target for this case. (Do I miss anything?)
In most cases, after finishing or canceling an animation, the computed value (e.g. marginLeft) will become the initial value. But after we set the target as null, I think this animation/effect shouldn't do anything to the original target, and we have to keep this animation going, so should we set the marginLeft of the original target back to the initial value, (just as cancel())? Or keep the marginLeft being its last value after restyling, so its value in the above example is still '75px', not initial value, '0px'.
Thanks.
Comment 38•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #37)
> Hi Brian,
>
> I have a problem about the state of the old target after setting a new
> target for a specific effect.
>
> e.g.
>
> var anim = div.animate({ 'marginLeft': ['0px', '100px'] }, 4000);
> anim.play();
>
> anim.ready.then(function() {
> anim.currentTime = 3000;
> console.log(getComputedStyle(div).marginLeft); // restyle, and the output
> is '75px'
> anim.effect.target = null; // set target to null
>
> // what is the expected value or marginLeft now?
It should be its initial value.
> I checked the spec and I didn't see the expected behavior of setting an
> effect target for this case. (Do I miss anything?)
I think the relevant section is: https://w3c.github.io/web-animations/#model-liveness
> In most cases, after finishing or canceling an animation, the computed value
> (e.g. marginLeft) will become the initial value. But after we set the target
> as null, I think this animation/effect shouldn't do anything to the original
> target,
Yes, when we compose the animation value for marginLeft we will find no animation effects targeting that property and will simply set it to its base value (initial value)
> and we have to keep this animation going, so should we set the
> marginLeft of the original target back to the initial value, (just as
> cancel())? Or keep the marginLeft being its last value after restyling, so
> its value in the above example is still '75px', not initial value, '0px'.
It should be '0px'.
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #38)
> Yes, when we compose the animation value for marginLeft we will find no
> animation effects targeting that property and will simply set it to its base
> value (initial value)
Thanks, Brian. Looks like I have to make sure we set the property of the old element to its base value if no animation targeting it.
Comment hidden (obsolete) |
Assignee | ||
Comment 41•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #38)
> Yes, when we compose the animation value for marginLeft we will find no
> animation effects targeting that property and will simply set it to its base
> value (initial value)
Hi Brian,
Sorry, I still have some questions about this. I'm thinking what is the most suitable way to set it to its base value. (Apparently, according to the current implementation, setting mTarget to nullptr directly cannot let it back to its base value.)
Firstly, I'm not familiar with that how to cascade the value from animation interpolation and the value from original css rule, so could you please give me some hints about this part? How do we override the interpolation value on the animation property. We compute the interpolation value in KeyframeEffectReadOnly::ComposeStyle(), and add the new value to the AnimValuesStyleRule which will map them into rule data. After finishing/canceling the animation, how do we remove the computed value from this property? So the property value becomes the original css value.
Thanks.
Comment 42•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #39)
> (In reply to Brian Birtles (:birtles) from comment #38)
> > Yes, when we compose the animation value for marginLeft we will find no
> > animation effects targeting that property and will simply set it to its base
> > value (initial value)
>
> Thanks, Brian. Looks like I have to make sure we set the property of the old
> element to its base value if no animation targeting it.
I think you should only need to call prescontext->EffectCompositor()->RequestRestyle(elem, pseudo, EffectCompositor::RestyleType::Layer, mAnimation->CascadeLevel()). Basically the same thing we do at the end of KeyframeEffect::NotifySpecifiedTimingUpdated. Of course you'll need to keep a reference to elem / pseudo if you've already cleared mTarget / mPseudoType.
You certainly shouldn't need to set the base value directly.
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #42)
> I think you should only need to call
> prescontext->EffectCompositor()->RequestRestyle(elem, pseudo,
> EffectCompositor::RestyleType::Layer, mAnimation->CascadeLevel()). Basically
> the same thing we do at the end of
> KeyframeEffect::NotifySpecifiedTimingUpdated. Of course you'll need to keep
> a reference to elem / pseudo if you've already cleared mTarget / mPseudoType.
>
> You certainly shouldn't need to set the base value directly.
Actually, I tried to call RequestRestyle(..), but it may be not enough.
e.g.
I declare two new members
nsCOMPtr<Element> mRemovedTarget;
CSSPseudoElementType mRemovedPseudoType;
void SetTarget()
{
// if set to null
// backup first
mRemovedTarget = mTarget;
mRemovedPseudoType = mPseudoType;
mTarget = nullptr;
mPseudoType = CSSPseudoElementType::NotPseudo;
// Request restyle
presContext->EffectCompositor()->
RequestRestyle(mRemovedTarget, mRemovedPseudoType,
EffectCompositor::RestyleType::Layer,
mAnimation->CascadeLevel());
}
The marginLeft is still '75px' after calling SetTarget(null) in the example (comment 37) and it keeps the value (75px) forever. Therefore, I think there are sill some bugs for these case. Maybe I have to check why we don't apply the base value back if mTarget becomes null, so I may need some hints for this part. Thanks.
Comment 44•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #43)
> e.g.
> I declare two new members
> nsCOMPtr<Element> mRemovedTarget;
> CSSPseudoElementType mRemovedPseudoType;
Why aren't local variables enough?
> void SetTarget()
> {
> // if set to null
> // backup first
> mRemovedTarget = mTarget;
> mRemovedPseudoType = mPseudoType;
> mTarget = nullptr;
> mPseudoType = CSSPseudoElementType::NotPseudo;
>
> // Request restyle
> presContext->EffectCompositor()->
> RequestRestyle(mRemovedTarget, mRemovedPseudoType,
> EffectCompositor::RestyleType::Layer,
> mAnimation->CascadeLevel());
I think we could call RequestRestyle before update mTarget / mPseudoType?
> The marginLeft is still '75px' after calling SetTarget(null) in the example
> (comment 37) and it keeps the value (75px) forever. Therefore, I think there
> are sill some bugs for these case. Maybe I have to check why we don't apply
> the base value back if mTarget becomes null, so I may need some hints for
> this part. Thanks.
Yes, you'll need to debug that one. I suspect we're doing an early return somewhere when we see mTarget is nullptr.
A few things we probably ought to be doing when we clear the target are:
* Some subset of KeyframeEffectReadOnly::UpdateTargetRegistration where we remove ourselves from the EffectSet.
* (Possibly calling effectSet->MarkCascadeNeedsUpdate() although hopefully calling RemoveEffect does that.)
* Reset the mIsRunningOnCompositor / mWinsInCascade member on all the animation properties.
* You can call ResetIsRunningOnCompositor for the former.
* For the latter, normally EffectCompositor::UpdateCascadeResults would do it, but I think we'll need to do it ourselves here.
* You *may* need to trigger a call to mAnimation->mTimeline->NotifyAnimationUpdated(mAnimation) but I'm not sure. Hopefully not.
Comment hidden (typo) |
Assignee | ||
Comment 46•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #44)
> * Some subset of KeyframeEffectReadOnly::UpdateTargetRegistration where we
> remove ourselves from the EffectSet.
> * (Possibly calling effectSet->MarkCascadeNeedsUpdate() although hopefully
> calling RemoveEffect does that.)
> * Reset the mIsRunningOnCompositor / mWinsInCascade member on all the
> animation properties.
> * You can call ResetIsRunningOnCompositor for the former.
> * For the latter, normally EffectCompositor::UpdateCascadeResults would do
> it, but I think we'll need to do it ourselves here.
OK. I think I only need to do these:
1. RemoveEffect, if it is empty, also call DestroyEffectSet().
2. RequestRestyle for the original element
3. Set mTarget = nullptr.
By these three steps, I can reset the animation property of the original element to its base value, and looks like I don't have to reset mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)
Thanks, these hints are really helpful.
Comment 47•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #46)
> By these three steps, I can reset the animation property of the original
> element to its base value, and looks like I don't have to reset
> mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)
Yes, you need to reset those. We should probably add a test that calling the chrome-only getProperties() method returns false for 'runningOnCompositor' immediately after setting target = null.
Assignee | ||
Comment 48•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #47)
> > Looks like I don't have to reset
> > mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)
>
> Yes, you need to reset those. We should probably add a test that calling the
> chrome-only getProperties() method returns false for 'runningOnCompositor'
> immediately after setting target = null.
I see. I should add a test for it.
Assignee | ||
Comment 49•9 years ago
|
||
I'm writing the mutation observer for setting a new target, and I think there are 4 cases:
1. null -> null : do nothing
2. null -> non-null : add this animation to the new target
3. non-null->null : remove this animation from the old target
4. non-null->non-null: remove this animation from the old target and add this animation to the new target.
Does it make sense? I will implement it by the above rule.
Assignee | ||
Comment 50•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48427/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48427/
Attachment #8744269 -
Flags: review?(bugs)
Attachment #8744270 -
Flags: review?(bbirtles)
Attachment #8744271 -
Flags: review?(bbirtles)
Attachment #8744272 -
Flags: review?(bbirtles)
Attachment #8744273 -
Flags: review?(bbirtles)
Attachment #8744274 -
Flags: review?(bbirtles)
Attachment #8744275 -
Flags: review?(bbirtles)
Attachment #8744276 -
Flags: review?(bbirtles)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 54•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48435/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48435/
Comment hidden (obsolete) |
Assignee | ||
Comment 56•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48439/
Comment hidden (obsolete) |
Comment 62•9 years ago
|
||
Comment on attachment 8744269 [details]
MozReview Request: Bug 1067769 - Part 5: Support setting KeyframeEffect.target webidl interface. r=smaug
https://reviewboard.mozilla.org/r/48427/#review45217
Attachment #8744269 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Attachment #8744270 -
Flags: review?(bbirtles)
Comment 63•9 years ago
|
||
Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles
https://reviewboard.mozilla.org/r/48429/#review45395
::: dom/animation/KeyframeEffect.cpp:687
(Diff revision 1)
> +static void
> +UnpackTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget,
> + RefPtr<Element>& aElement,
> + CSSPseudoElementType& aPseudoType)
Why don't we just make this return a Maybe<NonOwningAnimationTarget> and call it ConvertTarget or something like that?
That seems like it would be simpler and avoid some odd states in this current set up where, for example, if aTarget is null, we initialize aElement but not aPseudoType.
Comment 64•9 years ago
|
||
https://reviewboard.mozilla.org/r/48429/#review45399
::: dom/animation/KeyframeEffect.cpp:687
(Diff revision 1)
> +static void
> +UnpackTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget,
> + RefPtr<Element>& aElement,
> + CSSPseudoElementType& aPseudoType)
Looking at subsequent patches, it might be better still to introduce an OwningAnimationTarget and return that instead.
Comment 65•9 years ago
|
||
Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles
https://reviewboard.mozilla.org/r/48431/#review45397
::: dom/animation/KeyframeEffect.h:439
(Diff revision 1)
> void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
>
> protected:
> ~KeyframeEffect() override;
> +
> +private:
Unless there's a reason to make this private, I think protected would be better here, but see my comments later--I think we could probably get away without adding this method.
::: dom/animation/KeyframeEffect.cpp:1374
(Diff revision 1)
> - // TODO: fix in next patch
> + RefPtr<Element> newTarget;
> + CSSPseudoElementType newPseudoType = CSSPseudoElementType::NotPseudo;
> + UnpackTarget(aTarget, newTarget, newPseudoType);
> +
> + if (mTarget == newTarget && mPseudoType == newPseudoType) {
> + // Assign the same target, skip it.
> + return;
> + }
(This would become simpler if ConvertTarget returns an Maybe<(Non)OwningAnimationTarget> and we make mTarget itself a Maybe<OwningAnimationTarget> and we use the operator== on that for comparison.)
::: dom/animation/KeyframeEffect.cpp:1383
(Diff revision 1)
> + if (mTarget) {
> + // TODO: fix in the next patch
> + MOZ_ASSERT(false, "not implemented yet");
> + } else if (newTarget) {
> + // Replace null target with a valid element
> + AssignTarget(newTarget, newPseudoType);
> + }
Looking at this patch and the next, I wonder if it would be more clear if we split out the various utility methods and kept the flow in this method. What do you think about something like the following:
void
KeyframeEffect::SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget)
{
Maybe<NonOwningAnimationTarget> target = ConvertTarget(target);
if (target == mTarget) {
return;
}
if (mTarget) {
UnregisterTarget();
ResetIsRunningOnCompositor();
ResetWinsInCascade();
RequestRestyle(EffectCompositor::RestyleType::Layer);
}
mTarget = target;
if (mTarget) {
UpdateTargetRegistration();
MaybeUpdateProperties();
RequestRestyle(EffectCompositor::RestyleType::Layer);
}
}
void
KeyframeEffect::UnregisterTarget()
{
EffectSet* effectSet =
EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
if (effectSet) {
effectSet->RemoveEffect(*this);
if (effectSet->IsEmpty()) {
EffectSet::DestroyEffectSet(mTarget, mPseudoType);
}
}
}
void
KeyframeEffect::RequestRestyle(RestyleType aRestyleType)
{
nsPresContext* presContext = GetPresContext();
if (presContext && mAnimation) {
presContext->EffectCompositor()->
RequestRestyle(mTarget->mElement, mTarget->mPseudoType,
aRestyleType, mAnimation->CascadeLevel());
}
}
void
KeyframeEffect::MaybeUpdateProperties()
{
// We need to be careful to *not* call this when we are processing restyles
// since calling GetStyleContextForElement for element when we are in the
// process of building a style context may trigger various forms of infinite
// recursion.
nsIDocument* doc = mTarget->mElement->OwnerDoc();
if (!doc) {
return;
}
nsIAtom* pseudo = mPseudoType < CSSPseudoElementType::Count ?
nsCSSPseudoElements::GetPseudoAtom(mPseudoType) :
nullptr;
RefPtr<nsStyleContext> styleContext =
nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo,
doc->GetShell());
if (!styleContext) {
return;
}
UpdateProperties(styleContext);
}
?
::: dom/animation/KeyframeEffect.cpp:1405
(Diff revision 1)
>
> +void
> +KeyframeEffect::AssignTarget(Element* aNewTarget,
> + CSSPseudoElementType aNewPseudoType)
> +{
> + MOZ_ASSERT(aNewTarget, "Only assign a valid target");
So this is ok because we don't support assigning when the original target is null and we return early when the new target and original are the same, I guess?
::: dom/animation/KeyframeEffect.cpp:1412
(Diff revision 1)
> + if (mAnimation) {
> + mAnimation->UpdateRelevance();
> + }
This isn't needed, right? Relevance doesn't depend on having a target or not, right?
Attachment #8744271 -
Flags: review?(bbirtles)
Comment 66•9 years ago
|
||
Comment on attachment 8744273 [details]
MozReview Request: Bug 1067769 - Part 11: Implement animation mutation observer while setting the target. r=birtles
https://reviewboard.mozilla.org/r/48435/#review45411
Attachment #8744273 -
Flags: review?(bbirtles) → review+
Comment 67•9 years ago
|
||
Just to summarize my main feedback so far, I think we should:
* Introduce an OwningAnimationTarget type that has a RefPtr<Element> member
(If needed, we can add operator== and operator= members that take
a NonOwningAnimationTarget type -- but I think we might not need these)
* Replace the KeyframeEffectReadOnly mTarget/mPseudoType members with
a Maybe<OwningAnimationTarget> mTarget member.
We can probably leave KeyframeEffectReadOnly::GetTarget() as returning a
Maybe<NonOwningAnimationTarget>. So maybe we do need a converting assignment
operator like the following:
NonOwningAnimationTarget&
NonOwningAnimationTarget::operator=(const OwningAnimationTarget& aOther)
(Maybe we should rename NonOwningAnimationTarget.h to AnimationTarget.h and
put both NonOwningAnimationTarget and OwningAnimationTarget in the same
file?)
* Add a ConvertTarget helper method that takes a
Nullable<ElementOrCSSPseudoElement> argument and returns a
Maybe<OwningAnimationTarget> object.
Updated•9 years ago
|
Attachment #8744274 -
Flags: review?(bbirtles)
Comment 68•9 years ago
|
||
Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles
https://reviewboard.mozilla.org/r/48437/#review45413
This looks good but I'd like to take another look if you decide to simplify the tests at all.
::: testing/web-platform/meta/MANIFEST.json:28881
(Diff revision 1)
> + "path": "web-animations/keyframe-effect/setTarget.html",
> + "url": "/web-animations/keyframe-effect/setTarget.html"
> + },
> + {
Did you use ./mach web-platform-test --manifest-update for this? I think it normally adds tests to an "added_tests" section of some sort. I'm not sure if that matters.
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:28
(Diff revision 1)
> + anim.finish();
> + assert_equals(getComputedStyle(div).marginLeft, '10px',
> + 'Value if finished');
Do we need this test? Isn't the 50% test enough?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:33
(Diff revision 1)
> +test(function(t) {
> + var div = createDiv(t);
> + div.style.marginLeft = '10px';
> + var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> + anim.currentTime = 50 * MS_PER_SEC;
> + anim.effect.target = div;
> + assert_equals(getComputedStyle(div).marginLeft, '50px',
> + 'Value at 50% progress')
> + anim.finish();
> + assert_equals(getComputedStyle(div).marginLeft, '10px',
> + 'Value if finished')
> +}, 'Test setting the same target');
Do we need this test?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:56
(Diff revision 1)
> + assert_equals(getComputedStyle(div).marginLeft, '50px',
> + 'Value at 50% progress after setting new target');
We should probably test getComputedStyle(div).marginLeft immediately before changing the target?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:59
(Diff revision 1)
> + anim.finish();
> + assert_equals(getComputedStyle(div).marginLeft, '10px',
> + 'Value if finished')
Again, I'm not sure exactly why this is needed?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:79
(Diff revision 1)
> + var div = createDiv(t);
> + var div2 = createDiv(t);
Nit: Call them 'a' and 'b'? 'divA' and 'divB'?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:86
(Diff revision 1)
> + div.style.marginLeft = '10px';
> + div2.style.marginLeft = '20px';
> + var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> + anim.currentTime = 50 * MS_PER_SEC;
> + anim.effect.target = div2;
We should probably test the result of getComputedStyle before changing the target?
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:98
(Diff revision 1)
> + anim.finish();
> + assert_equals(getComputedStyle(div2).marginLeft, '20px',
> + 'Value of new target if finished')
Again, not sure this is needed?
Updated•9 years ago
|
Attachment #8744275 -
Flags: review?(bbirtles) → review+
Comment 69•9 years ago
|
||
Comment on attachment 8744275 [details]
MozReview Request: Bug 1067769 - Part 14: Test for our animation mutation observer. r=birtles
https://reviewboard.mozilla.org/r/48439/#review45415
This is really good but we need to test that redundant changes don't trigger updates:
* null -> null
* target -> target
Updated•9 years ago
|
Attachment #8744276 -
Flags: review?(bbirtles)
Comment 70•9 years ago
|
||
Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles
https://reviewboard.mozilla.org/r/48441/#review45417
This is good.
I think we also need to add a test that fails if we forget to call ResetWinsOnCompositor (assuming it is possible to create such a test!).
::: dom/animation/test/chrome/test_running_on_compositor.html:429
(Diff revision 1)
> + 'on the compositor');
> +
> + animation.effect.target = div;
> + return waitForFrame();
> + }).then(function() {
> + assert_equals(animation.isRunningOnCompositor, true,
Should this be s/true/omtaEnabled/ ?
::: dom/animation/test/chrome/test_running_on_compositor.html:436
(Diff revision 1)
> + 'after setting a valid target');
> + });
> +}, 'animation is added to the compositor when setting a valid target');
> +
> +promise_test(function(t) {
> + var animation = addDivAndAnimate(t,
Nit: I'd rather we don't use addDivAndAnimate. I think we should remove that function in the future since it obscures what the test is doing.
::: dom/animation/test/chrome/test_running_on_compositor.html:445
(Diff revision 1)
> + return animation.ready.then(function() {
> + assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> + 'Animation reports that it is running on the compositor');
> +
> + animation.effect.target = null;
> + return waitForFrame();
I don't think we need to wait for a frame here, right?
Comment 71•9 years ago
|
||
Comment on attachment 8744272 [details]
MozReview Request: Bug 1067769 - Part 8: Implement SetTarget(), when the original target is non-null
https://reviewboard.mozilla.org/r/48433/#review45419
Clearing this review for now, since I think it might be slightly better to do something like the suggestion in comment 65.
Attachment #8744272 -
Flags: review?(bbirtles)
Assignee | ||
Comment 72•9 years ago
|
||
Thanks for your review. I will try your suggestion in comment 65.
(In reply to Brian Birtles (:birtles) from comment #67)
> Just to summarize my main feedback so far, I think we should:
>
> * Introduce an OwningAnimationTarget type that has a RefPtr<Element> member
> (If needed, we can add operator== and operator= members that take
> a NonOwningAnimationTarget type -- but I think we might not need these)
OK. I will introduce this type.
>
> * Replace the KeyframeEffectReadOnly mTarget/mPseudoType members with
> a Maybe<OwningAnimationTarget> mTarget member.
>
> We can probably leave KeyframeEffectReadOnly::GetTarget() as returning a
> Maybe<NonOwningAnimationTarget>. So maybe we do need a converting
> assignment
> operator like the following:
>
> NonOwningAnimationTarget&
> NonOwningAnimationTarget::operator=(const OwningAnimationTarget& aOther)
>
> (Maybe we should rename NonOwningAnimationTarget.h to AnimationTarget.h and
> put both NonOwningAnimationTarget and OwningAnimationTarget in the same
> file?)
Good. I'd put them together and rename the file, and try to use them in KeyframeEffect.
>
> * Add a ConvertTarget helper method that takes a
> Nullable<ElementOrCSSPseudoElement> argument and returns a
> Maybe<OwningAnimationTarget> object.
OK. I should replace unpackTarget with "ConvertTarget" and revise its API. Thanks.
Assignee | ||
Comment 73•9 years ago
|
||
https://reviewboard.mozilla.org/r/48431/#review45397
> This isn't needed, right? Relevance doesn't depend on having a target or not, right?
I added this because I saw UpdateTargetRegistration() might be asserted if the relevance is not up-to-date, and I'm not sure if there is any possiblibliy of resulting in this assertion while calling SetTarget(). Actually, we don't need this while assigning a new target. Thanks.
Assignee | ||
Comment 74•9 years ago
|
||
https://reviewboard.mozilla.org/r/48437/#review45413
> Did you use ./mach web-platform-test --manifest-update for this? I think it normally adds tests to an "added_tests" section of some sort. I'm not sure if that matters.
I will re-run --manifest-update and make sure it is in the right place.
Assignee | ||
Comment 75•9 years ago
|
||
https://reviewboard.mozilla.org/r/48439/#review45415
OK. I will add an extra test, "set_redundant_animation_target" for these two cases.
Assignee | ||
Comment 76•9 years ago
|
||
https://reviewboard.mozilla.org/r/48441/#review45417
This is an interesting problem. We don't have an API for getting the property value, mWinsInCascade. Should I add one for Chrome-only?
> Should this be s/true/omtaEnabled/ ?
OK
> Nit: I'd rather we don't use addDivAndAnimate. I think we should remove that function in the future since it obscures what the test is doing.
OK. I will use addDiv and div.animate().
> I don't think we need to wait for a frame here, right?
Yes, I should remove it.
Comment 77•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #76)
> https://reviewboard.mozilla.org/r/48441/#review45417
>
> This is an interesting problem. We don't have an API for getting the
> property value, mWinsInCascade. Should I add one for Chrome-only?
No, that value is likely to change when we implement additive animation so I'd rather not add an API for it.
What I mean is we should try to create some other kind of test that would produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if it is possible to create such a test, however. That's because assuming we successfully call UpdateCascadeResults for *both* the old and new target (which I think we will do in UnregisterTarget and UpdateTargetRegistration) I guess this member will be updated then? So maybe we don't even need ResetIsRunningOnCompositor?
Assignee | ||
Comment 78•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #77)
> What I mean is we should try to create some other kind of test that would
> produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> it is possible to create such a test, however. That's because assuming we
> successfully call UpdateCascadeResults for *both* the old and new target
> (which I think we will do in UnregisterTarget and UpdateTargetRegistration)
I checked the log in this test case (i.e. test_running_on_compositor.html), and we will call EffectCompositor::UpdateCascadeResults() only for _new_ target. We removed the EffectSet of old target, so we will not call UpdateCascadeResults() for it.
> I guess this member will be updated then? So maybe we don't even need
> ResetIsRunningOnCompositor?
If I comment out "ResetIsRunningOnCompositor()" in SetTarget(), the following example fails:
promise_test(function(t) {
var div = addDiv(t);
var animation = div.animate({ opacity: [ 0, 1 ] }, 100 * MS_PER_SEC);
return animation.ready.then(function() {
assert_equals(animation.isRunningOnCompositor, omtaEnabled,
'Animation reports that it is running on the compositor');
animation.effect.target = null;
assert_equals(animation.isRunningOnCompositor, false, // Fail here.
'Animation reports that it is NOT running on the ' +
'compositor after setting null target');
});
}, "...")
I think it results from the same reason. We remove the EffectSet of the old target, so the mIsRunningOnCompositor wouldn't be updated. Therefore, we need to call ResetIsRunningOnCompositor() in SetTarget().
Assignee | ||
Comment 79•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48955/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48955/
Attachment #8744270 -
Attachment description: MozReview Request: Bug 1067769 - Part 6: Wrap unpacking of Nullable<ElementOrCSSPseudoElement> → MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r?birtles
Attachment #8744271 -
Attachment description: MozReview Request: Bug 1067769 - Part 7: Implement SetTarget(), when the original target is null → MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r?birtles
Attachment #8744273 -
Attachment description: MozReview Request: Bug 1067769 - Part 9: Implement animation mutation observer while setting the target → MozReview Request: Bug 1067769 - Part 11: Implement animation mutation observer while setting the target. r=birtles
Attachment #8744274 -
Attachment description: MozReview Request: Bug 1067769 - Part 10: Test for setting the target in basic cases → MozReview Request: Bug 1067769 - Part 12: Test for setting the target in basic cases. r?birtles
Attachment #8744275 -
Attachment description: MozReview Request: Bug 1067769 - Part 11: Test for our animation mutation observer → MozReview Request: Bug 1067769 - Part 13: Test for our animation mutation observer. r=birtles
Attachment #8744276 -
Attachment description: MozReview Request: Bug 1067769 - Part 12: Test for setting the target while running on the compositor → MozReview Request: Bug 1067769 - Part 14: Test for setting the target while running on the compositor. r?birtles
Attachment #8745282 -
Flags: review?(bbirtles)
Attachment #8745283 -
Flags: review?(bbirtles)
Attachment #8745284 -
Flags: review?(bbirtles)
Attachment #8744270 -
Flags: review?(bbirtles)
Attachment #8744271 -
Flags: review?(bbirtles)
Attachment #8744274 -
Flags: review?(bbirtles)
Attachment #8744276 -
Flags: review?(bbirtles)
Comment hidden (obsolete) |
Assignee | ||
Comment 81•9 years ago
|
||
We will need to request a restyle and unregister the current target in
SetTarget(), and there are many duplicate code segments for them now, so wrap
them for reusing the code.
Review commit: https://reviewboard.mozilla.org/r/48959/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48959/
Assignee | ||
Comment 87•9 years ago
|
||
Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48429/diff/1-2/
Assignee | ||
Comment 88•9 years ago
|
||
Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48431/diff/1-2/
Assignee | ||
Comment 90•9 years ago
|
||
Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48437/diff/1-2/
Assignee | ||
Comment 92•9 years ago
|
||
Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48441/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Attachment #8744272 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Comment 101•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #78)
> (In reply to Brian Birtles (:birtles) from comment #77)
> > What I mean is we should try to create some other kind of test that would
> > produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> > it is possible to create such a test, however. That's because assuming we
> > successfully call UpdateCascadeResults for *both* the old and new target
> > (which I think we will do in UnregisterTarget and UpdateTargetRegistration)
>
> I checked the log in this test case (i.e. test_running_on_compositor.html),
> and we will call EffectCompositor::UpdateCascadeResults() only for _new_
> target. We removed the EffectSet of old target, so we will not call
> UpdateCascadeResults() for it.
We should be calling UpdateCascadeResults on the old target. I guess we need to add that.
> > I guess this member will be updated then? So maybe we don't even need
> > ResetIsRunningOnCompositor?
>
> If I comment out "ResetIsRunningOnCompositor()" in SetTarget(), the
> following example fails:
Sorry, that was a typo on my part. I meant ResetWinsInCascade.
Comment 102•9 years ago
|
||
Comment on attachment 8745282 [details]
MozReview Request: Bug 1067769 - Part 6: Rename NonOwningAnimationTarget.h to AnimationTarget.h. r=birtles
https://reviewboard.mozilla.org/r/48955/#review45889
Attachment #8745282 -
Flags: review?(bbirtles) → review+
Comment 103•9 years ago
|
||
Comment on attachment 8745283 [details]
MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles
https://reviewboard.mozilla.org/r/48957/#review45899
Nice.
Attachment #8745283 -
Flags: review?(bbirtles) → review+
Updated•9 years ago
|
Attachment #8744270 -
Flags: review?(bbirtles) → review+
Comment 104•9 years ago
|
||
Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles
https://reviewboard.mozilla.org/r/48429/#review45901
r=me with comments addressed
::: dom/animation/KeyframeEffect.cpp:464
(Diff revision 3)
> - mPseudoType < CSSPseudoElementType::Count ?
> - nsCSSPseudoElements::GetPseudoAtom(mPseudoType) : nullptr;
> + mTarget->mPseudoType < CSSPseudoElementType::Count ?
> + nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType) : nullptr;
> styleContext =
> - nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo, shell);
> + nsComputedDOMStyle::GetStyleContextForElement(mTarget->mElement,
> + pseudo, shell);
Does this part belong in the previous patch?
::: dom/animation/KeyframeEffect.cpp:739
(Diff revision 3)
> + RefPtr<Element> temp = target.GetAsCSSPseudoElement().ParentElement();
> + result.emplace(temp, target.GetAsCSSPseudoElement().GetType());
Is this temporary necessary? Perhaps it is because ParentElement returns an already_AddRefed value. Can we rename it to 'elem' or something other than 'temp' though?
::: dom/animation/KeyframeEffect.cpp:767
(Diff revision 3)
> RefPtr<KeyframeEffectType> effect =
> - new KeyframeEffectType(doc, targetElement, pseudoType, timingParams);
> + new KeyframeEffectType(doc,
> + target ? target->mElement.get() : nullptr,
> + target ? target->mPseudoType
> + : CSSPseudoElementType::NotPseudo,
> + timingParams);
We should probably just change the KeyframeEffect and KeyframeEffectReadOnly constructors to take a Maybe<(Non)OwningAnimationElement> parameter.
Otherwise we end up converting:
OwningAnimationElement -> [Element, PseudoType] -> OwningAnimationElement
We can do that in a follow-up patch/bug, however.
Comment 105•9 years ago
|
||
Comment on attachment 8745284 [details]
MozReview Request: Bug 1067769 - Part 9: Wrap RequestRestyle and UnregisterTarget. r=birtles
https://reviewboard.mozilla.org/r/48959/#review45903
::: dom/animation/KeyframeEffect.h:369
(Diff revision 2)
> + // Unregister current target, so it removes this effect from mTarget's
> + // effectSet.
// Remove the current effect target from its EffectSet.
::: dom/animation/KeyframeEffect.h:373
(Diff revision 2)
>
> + // Unregister current target, so it removes this effect from mTarget's
> + // effectSet.
> + void UnregisterTarget();
> +
> + // Request a restyle
I'm not sure this comment is really necessary.
Attachment #8745284 -
Flags: review?(bbirtles) → review+
Comment 106•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #101)
> (In reply to Boris Chiou [:boris] from comment #78)
> > (In reply to Brian Birtles (:birtles) from comment #77)
> > > What I mean is we should try to create some other kind of test that would
> > > produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> > > it is possible to create such a test, however. That's because assuming we
> > > successfully call UpdateCascadeResults for *both* the old and new target
> > > (which I think we will do in UnregisterTarget and UpdateTargetRegistration)
> >
> > I checked the log in this test case (i.e. test_running_on_compositor.html),
> > and we will call EffectCompositor::UpdateCascadeResults() only for _new_
> > target. We removed the EffectSet of old target, so we will not call
> > UpdateCascadeResults() for it.
>
> We should be calling UpdateCascadeResults on the old target. I guess we need
> to add that.
It looks like UnregisterTarget will call EffectSet::RemoveEffect which will call MarkCascadeNeedsUpdate so that should be enough in the case where we stop targetting a particular element but some other animation is still targetting it.
Comment 107•9 years ago
|
||
Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles
https://reviewboard.mozilla.org/r/48431/#review45907
r=me with comments addressed
::: dom/animation/KeyframeEffect.cpp:1419
(Diff revision 3)
> +
> + if (mTarget) {
> + UnregisterTarget();
> + ResetIsRunningOnCompositor();
> + ResetWinsInCascade();
> +
I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.
We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:
1. Set mTarget to Nothing()
(Fail to clear mProgressOnLastCompose)
2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
with any EffectSet.
3. Update mTarget to some new element.
4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
NotifyAnimationTimingUpdated) on the new element because
mProgressOnLastCompose happens to match the current time.
I'm not sure if that can ever happen, however.
At (3) we'll request a restyle so we should be ok I think?
::: dom/animation/KeyframeEffect.cpp:1427
(Diff revision 3)
> +
> + MaybeUpdateProperties();
Nit: Perhaps put the blank line before the RequestRestyle (or just get rid of it altogether). I only added blank lines in comment 65 to highlight the symmetry between the two branches: both tidy up internal state then request restyle.
::: dom/animation/KeyframeEffect.cpp:1445
(Diff revision 3)
> }
>
> +void
> +KeyframeEffect::MaybeUpdateProperties()
> +{
> + // We need to be careful to *not* call this when we are processing restyles
Perhaps we could reword this somewhat--it's not so much about processing restyles as about building a style context.
// We need to be careful to *not* call this when we are updating the style
// context. That's because calling GetStyleContextForElement when we are in
// the process of building a style context may trigger various forms of
// infinite recursion.
We should also add a comment to SetTarget to say something like:
// This method calls MaybeUpdateProperties which is not safe to use when
// we are in the middle of updating style. If we need to use this when
// updating style, we should pass the nsStyleContext into this method and use
// that to update the properties rather than calling
// GetStyleContextForElement.
Perhaps it's best to add that to the header file even?
::: dom/animation/KeyframeEffect.cpp:1450
(Diff revision 3)
> + // We need to be careful to *not* call this when we are processing restyles
> + // since calling GetStyleContextForElement for element when we are in the
> + // process of building a style context may trigger various forms of infinite
> + // recursion.
> +
> + nsIDocument* doc = mTarget->mElement->OwnerDoc();
This should check for !mTarget right?
Attachment #8744271 -
Flags: review?(bbirtles) → review+
Comment 108•9 years ago
|
||
Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles
https://reviewboard.mozilla.org/r/48437/#review45909
Looks good but I wonder if we should be updating MANIFEST.json manually like this. We should probably use --manifest-update.
::: testing/web-platform/meta/MANIFEST.json:35307
(Diff revision 3)
> + "web-animations/keyframe-effect/setTarget.html": [
> + {
> + "path": "web-animations/keyframe-effect/setTarget.html",
> + "url": "/web-animations/keyframe-effect/setTarget.html"
> + }
> + ],
Did you use ./mach web-platform-tests --manifest-update ?
See comment 68.
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:18
(Diff revision 3)
> +
> +var gKeyFrames = { 'marginLeft': ['0px', '100px'] };
> +
> +test(function(t) {
> + var div = createDiv(t);
> + div.style.marginLeft = '10px';
Nit: I think we can drop this.
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:52
(Diff revision 3)
> + div.style.marginLeft = '10px';
> + var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> + anim.currentTime = 50 * MS_PER_SEC;
> + assert_equals(getComputedStyle(div).marginLeft, '50px',
> + 'Value at 50% progress')
Nit: 'Value at 50% progress before clearing the target'
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:56
(Diff revision 3)
> + assert_equals(getComputedStyle(div).marginLeft, '50px',
> + 'Value at 50% progress')
> +
> + anim.effect.target = null;
> + assert_equals(getComputedStyle(div).marginLeft, '10px',
> + 'Value after reset')
Nit: 'Value after clearing the target'
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:68
(Diff revision 3)
> + b.style.marginLeft = '20px';
> + var anim = a.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> + anim.currentTime = 50 * MS_PER_SEC;
> + assert_equals(getComputedStyle(a).marginLeft, '50px',
> + 'Value of the 1st target before setting')
Nit: These test messages could be a little more clear.
e.g.
'Value of 1st element (currently targeted) before changing the effect target'
'Value of 2nd element (currently not targeted) before changing the effect target'
::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:77
(Diff revision 3)
> + anim.currentTime = 75 * MS_PER_SEC;
> + assert_equals(getComputedStyle(a).marginLeft, '10px',
> + 'Value of the 1st target')
> + assert_equals(getComputedStyle(b).marginLeft, '75px',
> + 'Value of the 2nd target')
These messages in particular could be a lot clearer. (Also, I wonder if we really need this part, but maybe we do.)
Attachment #8744274 -
Flags: review?(bbirtles)
Updated•9 years ago
|
Attachment #8744276 -
Flags: review?(bbirtles) → review+
Comment 109•9 years ago
|
||
Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles
https://reviewboard.mozilla.org/r/48441/#review45911
Assignee | ||
Comment 110•9 years ago
|
||
https://reviewboard.mozilla.org/r/48429/#review45901
> Does this part belong in the previous patch?
Yes. Looks like I squashed it to the wrong patch. Sorry, I will fix it.
> Is this temporary necessary? Perhaps it is because ParentElement returns an already_AddRefed value. Can we rename it to 'elem' or something other than 'temp' though?
Yes, you're right. It's necessary. I will rename it to elem.
> We should probably just change the KeyframeEffect and KeyframeEffectReadOnly constructors to take a Maybe<(Non)OwningAnimationElement> parameter.
>
> Otherwise we end up converting:
>
> OwningAnimationElement -> [Element, PseudoType] -> OwningAnimationElement
>
> We can do that in a follow-up patch/bug, however.
OK. I will do it in a follow-up patch. Thanks.
Assignee | ||
Comment 111•9 years ago
|
||
https://reviewboard.mozilla.org/r/48431/#review45907
> I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.
>
> We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:
>
> 1. Set mTarget to Nothing()
> (Fail to clear mProgressOnLastCompose)
> 2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
> with any EffectSet.
> 3. Update mTarget to some new element.
> 4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
> NotifyAnimationTimingUpdated) on the new element because
> mProgressOnLastCompose happens to match the current time.
>
> I'm not sure if that can ever happen, however.
>
> At (3) we'll request a restyle so we should be ok I think?
For (3), yes. Actually, we have to request a restyle in (3), or we cannot pass the 4th test in setTarget.html.
I tried to reproduce your case, but at the next Animation:Tick(), the GetComputedTiming().mProgress is changed a litte (part of it might be due to my printfs), so we still request a restyle because the current progress is different from last unreset mProgressOnLastCompose. However, I think requesting a restyle in (3) is necessary to make sure we can show the update of effect.target immediately.
Therefore, I think not reseting mProgressOnLastCompose is OK here.
Comment 112•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #111)
> https://reviewboard.mozilla.org/r/48431/#review45907
>
> > I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.
> >
> > We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:
> >
> > 1. Set mTarget to Nothing()
> > (Fail to clear mProgressOnLastCompose)
> > 2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
> > with any EffectSet.
> > 3. Update mTarget to some new element.
> > 4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
> > NotifyAnimationTimingUpdated) on the new element because
> > mProgressOnLastCompose happens to match the current time.
> >
> > I'm not sure if that can ever happen, however.
> >
> > At (3) we'll request a restyle so we should be ok I think?
>
> For (3), yes. Actually, we have to request a restyle in (3), or we cannot
> pass the 4th test in setTarget.html.
> I tried to reproduce your case, but at the next Animation:Tick(), the
> GetComputedTiming().mProgress is changed a litte (part of it might be due to
> my printfs), so we still request a restyle because the current progress is
> different from last unreset mProgressOnLastCompose. However, I think
> requesting a restyle in (3) is necessary to make sure we can show the update
> of effect.target immediately.
>
> Therefore, I think not reseting mProgressOnLastCompose is OK here.
I am guessing the reason is that mTarget check in NotifyAnimationTimingUpdated() is inside GetComputedTiming().mProgress != mProgressOnLastCompose in your part 1 patch.
I think we can also skip calling CanThrottle() if mTarget is null. If we do the null check outside GetComputedTiming().mProgress != mProgressOnLastCompose check, there might be some cases mProgressOnLastCompose is not cleared.
For your reference, I've written a reftest for a similar case in bug 1235002.
Assignee | ||
Comment 113•9 years ago
|
||
https://reviewboard.mozilla.org/r/48437/#review45909
> Did you use ./mach web-platform-tests --manifest-update ?
>
> See comment 68.
Yes. I used ./mach web-platform-tests --manifest-update [file path]
My steps are:
1. Comment out this line [1] to prevent it from updating reftests.
2. ./mach web-platform-tests --manifest-update testing/web-platform/tests/web-animations/
3. Revert non-related parts.
e.g.
"local_changes": {
"deleted": [
- "web-animations/keyframe-effect/getComputedTiming-currentIteration.html",
- "web-animations/keyframe-effect/getComputedTiming-progress.html"
+ "web-animations/keyframe-effect/getComputedTiming-progress.html",
+ "web-animations/keyframe-effect/getComputedTiming-currentIteration.html"
],
[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/testing/web-platform/tests/tools/manifest/manifest.py#166
Comment 114•9 years ago
|
||
Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles
https://reviewboard.mozilla.org/r/48437/#review45989
r=me with comments addressed
Attachment #8744274 -
Flags: review+
Comment 115•9 years ago
|
||
https://reviewboard.mozilla.org/r/48437/#review45909
> Yes. I used ./mach web-platform-tests --manifest-update [file path]
>
> My steps are:
> 1. Comment out this line [1] to prevent it from updating reftests.
> 2. ./mach web-platform-tests --manifest-update testing/web-platform/tests/web-animations/
> 3. Revert non-related parts.
> e.g.
> "local_changes": {
> "deleted": [
> - "web-animations/keyframe-effect/getComputedTiming-currentIteration.html",
> - "web-animations/keyframe-effect/getComputedTiming-progress.html"
> + "web-animations/keyframe-effect/getComputedTiming-progress.html",
> + "web-animations/keyframe-effect/getComputedTiming-currentIteration.html"
> ],
>
> [1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/testing/web-platform/tests/tools/manifest/manifest.py#166
Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!
I tend to just do:
./mach web-platform-tests --manifest-update yer
To avoid running the tests.
Assignee | ||
Comment 116•9 years ago
|
||
https://reviewboard.mozilla.org/r/48437/#review45909
> Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!
>
> I tend to just do:
>
> ./mach web-platform-tests --manifest-update yer
>
> To avoid running the tests.
I will upload an extra patch for updating MANIFEST.json before applying this patch, so it will be clearer.
Assignee | ||
Comment 117•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #115)
> I tend to just do:
>
> ./mach web-platform-tests --manifest-update yer
>
> To avoid running the tests.
Thanks. This command is helpful. :)
Comment 118•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #116)
> https://reviewboard.mozilla.org/r/48437/#review45909
>
> > Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!
> >
> > I tend to just do:
> >
> > ./mach web-platform-tests --manifest-update yer
> >
> > To avoid running the tests.
>
> I will upload an extra patch for updating MANIFEST.json before applying this
> patch, so it will be clearer.
No need, what you have is fine. It just wasn't obvious from the diff in MozReview that it was the "local_changes" part that we were touching.
Assignee | ||
Comment 119•9 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #112)
> I am guessing the reason is that mTarget check in
> NotifyAnimationTimingUpdated() is inside GetComputedTiming().mProgress !=
> mProgressOnLastCompose in your part 1 patch.
> I think we can also skip calling CanThrottle() if mTarget is null. If we do
> the null check outside GetComputedTiming().mProgress !=
> mProgressOnLastCompose check, there might be some cases
> mProgressOnLastCompose is not cleared.
>
> For your reference, I've written a reftest for a similar case in bug 1235002.
Thanks, Hiro. I saw you just filed Bug 1267937 which may handle mProgressOnLastCompose properly.
Assignee | ||
Comment 120•9 years ago
|
||
https://reviewboard.mozilla.org/r/48437/#review45909
> These messages in particular could be a lot clearer. (Also, I wonder if we really need this part, but maybe we do.)
Just want to make sure we change the property value corretly on the new target. However, I can drop the 1st target part. Thanks.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 131•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #106)
> (In reply to Brian Birtles (:birtles) from comment #101)
> > We should be calling UpdateCascadeResults on the old target. I guess we need
> > to add that.
>
> It looks like UnregisterTarget will call EffectSet::RemoveEffect which will
> call MarkCascadeNeedsUpdate so that should be enough in the case where we
> stop targetting a particular element but some other animation is still
> targetting it.
OK, so calling EffectSet::RemoveEffect() in UnregisterTarget is enough now. (And I don't have to add UpdateCascadeResults()).
I still have a patch, part 12, which replaces Element/CSSPseudoElementType with Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly)::KeyframeEffect(ReadOnly)(). I use |OwningAnimationTarget| here, so we don't have to write the conversion from Maybe<NonOwningAnimationTarget> to Maybe<OwningAnimationTarget>.
Assignee | ||
Comment 132•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #131)
> I still have a patch, part 12, which replaces Element/CSSPseudoElementType
> with Maybe<OwningAnimationTarget> in
> KeyframeEffect(ReadOnly)::KeyframeEffect(ReadOnly)(). I use
> |OwningAnimationTarget| here, so we don't have to write the conversion from
> Maybe<NonOwningAnimationTarget> to Maybe<OwningAnimationTarget>.
I'm thinking should I use rvalue reference and Move() for Maybe<OwningAnimationTarget> to reduce the memory copy? However, OwningAnimationTarget is not a big object, so I still use lvalue reference here.
Updated•9 years ago
|
Component: DOM → DOM: Animation
Comment 133•9 years ago
|
||
Comment on attachment 8745939 [details]
MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles
https://reviewboard.mozilla.org/r/49173/#review46171
Looks good. Thanks.
One comment, is there any reason not to pass Maybe<OwningAnimationTarget> as a const ref?
r=me with that change made unless there is a good reason to use a non-const ref.
Attachment #8745939 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 134•9 years ago
|
||
https://reviewboard.mozilla.org/r/49173/#review46171
I should add _const_. Thanks. I will update them.
Assignee | ||
Comment 135•9 years ago
|
||
Comment on attachment 8745939 [details]
MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49173/diff/1-2/
Attachment #8745939 -
Attachment description: MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r?birtles → MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (typo) |
Assignee | ||
Comment 143•9 years ago
|
||
Try server said: multiple definition of 'mozilla::ImplCycleCollectionUnlink' and 'mozilla::ImplCycleCollectionTraverse" in many places. I think I have to add _inline_ in these functions (in part 7). I will update them and rebase these patches.
Assignee | ||
Comment 150•9 years ago
|
||
Comment on attachment 8745283 [details]
MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48957/diff/3-4/
Assignee | ||
Comment 170•9 years ago
|
||
Sorry for these spams. I have to rebase these patch again tomorrow because there are some conflicts in mozilla-inbound.
Comment 186•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cfa5af78077
https://hg.mozilla.org/integration/mozilla-inbound/rev/558d977d7cc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8e27fc1e18
https://hg.mozilla.org/integration/mozilla-inbound/rev/8628768ddf1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78c3f967032
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ce9cec5247
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f88a50a4ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/cff7ac3b0307
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e126b275f8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70f145ab9b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6383fb78ede9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3cd399cf46
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbe8d41149
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e8827792d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad471499e704
Comment 187•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8cfa5af78077
https://hg.mozilla.org/mozilla-central/rev/558d977d7cc8
https://hg.mozilla.org/mozilla-central/rev/9f8e27fc1e18
https://hg.mozilla.org/mozilla-central/rev/8628768ddf1f
https://hg.mozilla.org/mozilla-central/rev/c78c3f967032
https://hg.mozilla.org/mozilla-central/rev/50ce9cec5247
https://hg.mozilla.org/mozilla-central/rev/2f88a50a4ece
https://hg.mozilla.org/mozilla-central/rev/cff7ac3b0307
https://hg.mozilla.org/mozilla-central/rev/7e126b275f8a
https://hg.mozilla.org/mozilla-central/rev/f70f145ab9b8
https://hg.mozilla.org/mozilla-central/rev/6383fb78ede9
https://hg.mozilla.org/mozilla-central/rev/1c3cd399cf46
https://hg.mozilla.org/mozilla-central/rev/89fbe8d41149
https://hg.mozilla.org/mozilla-central/rev/d6e8827792d4
https://hg.mozilla.org/mozilla-central/rev/ad471499e704
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 188•7 years ago
|
||
Updated documentation:
https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/target
Submitted PR to update BCD: https://github.com/mdn/browser-compat-data/pull/2441
And updated Firefox 49 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Comment 189•7 years ago
|
||
This interface is still preffed off by default. I hope to ship in 63.
You need to log in
before you can comment on or make changes to this bug.
Description
•