Closed
Bug 1241784
Opened 9 years ago
Closed 9 years ago
Implement CSSPseudoElement.animate()
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: boris, Assigned: boris)
References
()
Details
Attachments
(4 files, 13 obsolete files)
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
According to our spec, (CSS)PseudoElement implements Animatable; Bug 1174575 only implemented basic attributes for (CSS)PseudoElement, so we have to implement animate() and getAnimations() [1] in (CSS)PseudoElement here. [1] https://w3c.github.io/web-animations/#animatable
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → boris.chiou
Comment hidden (obsolete) |
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8720199 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8721652 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Attachment #8720200 -
Flags: review?(bbirtles)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8721653 [details] [diff] [review] Part 3: Test Review of attachment 8721653 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/web-animations/animatable/animate.html @@ +153,5 @@ > + div.className = ''; > + var sheetToBeRemoved = doc.getElementById('pseudoTest'); > + var sheetParent = sheetToBeRemoved.parentNode; > + sheetParent.removeChild(sheetToBeRemoved); > +} I am not sure if adding/removing a css rule for pseudo element dynamically is a good way in web-platform-tests. (Maybe we can hack a solution to achieve this in the future.) I don't have a good idea to get a pseudo element by other ways, so use these two functions to achieve the purpose.
Attachment #8721653 -
Flags: review?(bbirtles)
Comment 6•9 years ago
|
||
Comment on attachment 8721652 [details] [diff] [review] Part 1: Add a helper function for Element.animate() (v2) Review of attachment 8721652 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Element.cpp @@ +3322,5 @@ > +{ > + if (aTarget.IsNull()) { > + aError.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } There are a number of error code paths here that we don't need. As best possible, we shouldn't add code paths we can't test and especially when we can simply avoid creating error cases altogether. As far as I understand, there are only two call sites for this: Element::Animate and CSSPseudoElement::Animate, both of which we control, both of which will never need a null target, and both of which should never pass an uninitialized target. So, we should: 1. Make the argument type of aTarget just ElementOrCSSPseudoElement&. 2. Assert that aTarget is either an element a CSS pseudo element by putting a MOZ_ASSERT_UNREACHABLE in your else block below. 3. Wrap aTarget in Nullable when we pass it to the constructor. ::: dom/base/Element.h @@ +833,5 @@ > + JS::Handle<JSObject*> aFrames, > + const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions, > + ErrorResult& aError); > + static already_AddRefed<Animation> > + Animate(const Nullable<ElementOrCSSPseudoElement>& aTarget, This needs a comment pointing out that it's a helper method that factors out the common functionality needed by Element::Animate and CSSPseudoElement::Animate.
Attachment #8721652 -
Flags: review?(bbirtles)
Updated•9 years ago
|
Attachment #8720200 -
Flags: review?(bbirtles) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8721653 [details] [diff] [review] Part 3: Test Review of attachment 8721653 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/web-animations/animatable/animate.html @@ +143,5 @@ > + '.before::before { content: ""; animation: anim 10s; }'; > + doc.body.appendChild(style); > + div.className = 'before'; > + return doc.getAnimations()[0].effect.target; > +} Why not use the CSSOM like this: https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/style/test/test_animations_event_order.html#161 @@ +145,5 @@ > + div.className = 'before'; > + return doc.getAnimations()[0].effect.target; > +} > + > +function removeStyleOnPseudoElement(div, doc) { Instead of making each test call this cleanup function, register it automatically in createStyleOnPseudoElement: https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/testing/web-platform/tests/web-animations/testcommon.js#44 @@ +179,5 @@ > + assert_class_string(anim.effect, 'KeyframeEffect', > + 'Returned Animation has a KeyframeEffect'); > + removeStyleOnPseudoElement(div); > +}, 'CSSPseudoElement.animate() creates an Animation object with a ' + > + 'KeyframeEffect'); I don't think we need to repeat all these tests. It's a maintenance burden to try and keep two copies in sync. Either we should refactor the first set of tests to do everything on both an Element and a CSSPseudoElement, or just work out what we really need to test with regards to pseudo elements (assuming that most implementations will re-use mostly the same code paths for either case.) In this case, I think probably the first two tests are enough.
Attachment #8721653 -
Flags: review?(bbirtles)
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8721652 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #7) > Why not use the CSSOM like this: > > https://dxr.mozilla.org/mozilla-central/rev/ > af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/style/test/ > test_animations_event_order.html#161 Thanks. I should follow the same rules as the test cases in Bug 1234403. > I don't think we need to repeat all these tests. It's a maintenance burden > to try and keep two copies in sync. > > Either we should refactor the first set of tests to do everything on both an > Element and a CSSPseudoElement, or just work out what we really need to test > with regards to pseudo elements (assuming that most implementations will > re-use mostly the same code paths for either case.) In this case, I think > probably the first two tests are enough. I agree. Just test the basic functions for CSSPseudoElement::Animate() because we indeed use the same code paths for both Element::Animate() and CSSPseudoElement::Animate(). Thanks.
Assignee | ||
Updated•9 years ago
|
Attachment #8721902 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8720200 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8721653 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8721926 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8722381 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8721924 -
Flags: review?(bbirtles)
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8723422 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8723423 -
Flags: review?(bbirtles)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Summary: Implement Animatable interface for (CSS)PseudoElement → Implement CSSPseudoElement.animate()
Comment 16•9 years ago
|
||
Comment on attachment 8721924 [details] [diff] [review] Part 1: Add a helper function for Element.animate() (v4) Review of attachment 8721924 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed. ::: dom/base/Element.cpp @@ +3323,5 @@ > +{ > + // We don't support operator= and copy constructor for > + // ElementOrCSSPseudoElement, so we have to set it as Element or > + // CSSPseudoElement manually. > + Nullable<ElementOrCSSPseudoElement> targetWrapper; In that case I take back forget my comments about passing a bare ElementOrCSSPseudoElement. This is too messy. I still think we shouldn't add code paths we can't test, however. We should just make the start of this method: MOZ_ASSERT(!aTarget.IsNull() && (aTarget.Value().IsElement() || aTarget.Value().IsCSSPseudoElement()), "aTarget should be initialized"); RefPtr<Element> referenceElement; if (aTarget.Value().IsElement()) { referenceElement = &aTarget.Value().GetAsElement(); } else { referenceElement = aTarget.Value().GetAsCSSPseudoElement().ParentElement(); } nsCOMPtr<nsIGlobalObject> ownerGlobal = referenceElement->GetOwnerGlobal(); ... ::: dom/base/Element.h @@ +831,5 @@ > + Animate(JSContext* aContext, > + JS::Handle<JSObject*> aFrames, > + const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions, > + ErrorResult& aError); > + // A helper method that factors out the common functionality needed by Nit: Add a blank line before this
Attachment #8721924 -
Flags: review?(bbirtles) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8723423 [details] [diff] [review] Part 3: Test (v5) Review of attachment 8723423 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/web-animations/animatable/animate.html @@ +128,5 @@ > +// Tests on CSSPseudoElement > + > +test(function(t) { > + createStyle(t, {'@keyframes anim': '', > + '.before::before': 'animation: anim 10s;'}); We normally put spaces on either side of braces in Javascript.[1] I know this is web-platform-tests so Gecko coding styles don't necessarily apply, but I think it makes it easier to read. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects @@ +131,5 @@ > + createStyle(t, {'@keyframes anim': '', > + '.before::before': 'animation: anim 10s;'}); > + var div = createDiv(t); > + div.classList.add('before'); > + // Check if addAnimationOnPseudoElement adds an animation on a What is addAnimationOnPseudoElement? @@ +138,5 @@ > + 'Add an animation on a pseudo element successfully'); > + assert_class_string(document.getAnimations()[0].effect.target, > + 'CSSPseudoElement', > + 'The returned target is a CSSPseudoElement'); > +}, 'Create a CSSPseudoElement successfully') This doesn't seem to belong as a separate test in animatable/animate.html, right? In the long-term, hopefully there will be a way to create a CSSPseudoElement directly. So perhaps we should add a helper method to testcommon.js that calls createDiv and createStyle, fetches the pseudo element, asserts that it exists and has the right type, cancels the generated animation and then returns the object? e.g. function createPseudo(type) { // createDiv // createStyle (uses type) // assert_equals(document.getAnimations().length, 1) // assert_class_string(...) // (In future might be able to use div.getAnimations({subtree: true})) // anim = document.getAnimations()[0] // anim.cancel(); // return anim.effect.target; } Then when there is a more direct way of creating CSSPseudoElement objects we can just update/drop that method? What do you think? ::: testing/web-platform/tests/web-animations/testcommon.js @@ +46,5 @@ > }); > return div; > } > > +// Creates a style sheet and appends it to the document head "Creates a style element with the specified rules, appends it to the document head and removes the created element during test cleanup." You should probably also give an example of the format of |rules| too. @@ +69,4 @@ > // Removes element > function removeElement(element) { > element.parentNode.removeChild(element); > } Nit: createDiv calls this removeElement() function. It was something the UniPro guys added which I opposed at the time. Now that every user agent that is likely to implement this supports remove() we should probably drop 'removeElement' and update createDiv. But we can do that later if you want to keep this patch simple.
Attachment #8723423 -
Flags: review?(bbirtles)
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #16) > In that case I take back forget my comments about passing a bare > ElementOrCSSPseudoElement. This is too messy. I still think we shouldn't add > code paths we can't test, however. > > We should just make the start of this method: > > MOZ_ASSERT(!aTarget.IsNull() && > (aTarget.Value().IsElement() || > aTarget.Value().IsCSSPseudoElement()), > "aTarget should be initialized"); > > RefPtr<Element> referenceElement; > if (aTarget.Value().IsElement()) { > referenceElement = &aTarget.Value().GetAsElement(); > } else { > referenceElement = > aTarget.Value().GetAsCSSPseudoElement().ParentElement(); > } > > nsCOMPtr<nsIGlobalObject> ownerGlobal = referenceElement->GetOwnerGlobal(); > ... Thanks for your example. I will use Nullable<ElementOrCSSPseudoElement> as the function argument directly and add the assertion.
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #17) > What is addAnimationOnPseudoElement? Oops, I forgot to remove this comment. > In the long-term, hopefully there will be a way to create a CSSPseudoElement > directly. So perhaps we should add a helper method to testcommon.js that > calls createDiv and createStyle, fetches the pseudo element, asserts that it > exists and has the right type, cancels the generated animation and then > returns the object? > > e.g. > > function createPseudo(type) { > //... > } > > Then when there is a more direct way of creating CSSPseudoElement objects we > can just update/drop that method? > > What do you think? I agree. We could move this part into testcommon.js > Nit: createDiv calls this removeElement() function. It was something the > UniPro guys added which I opposed at the time. > > Now that every user agent that is likely to implement this supports remove() > we should probably drop 'removeElement' and update createDiv. But we can do > that later if you want to keep this patch simple. OK. I will update a new individual patch (part 4) to remove removeElement() and use Element.remove() directly.
Assignee | ||
Comment 20•9 years ago
|
||
Therefore, CSSPseudoElement.animate() can also use it. Updated: address Brian's comments.
Attachment #8726632 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8721924 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8721925 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8723423 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8726634 -
Flags: review?(bbirtles)
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8726635 [details] [diff] [review] Part 4: Remove removeElement from testcommon.js Review of attachment 8726635 [details] [diff] [review]: ----------------------------------------------------------------- Looks like only createDiv() uses removeElement().
Attachment #8726635 -
Flags: review?(bbirtles)
Comment 25•9 years ago
|
||
Comment on attachment 8726634 [details] [diff] [review] Part 3: Test (v6) Review of attachment 8726634 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that possible tweak to createPseudo if you agree it makes sense. ::: testing/web-platform/tests/web-animations/testcommon.js @@ +81,5 @@ > + createStyle(test, { '@keyframes anim': '', > + ['.pseudo::' + type]: 'animation: anim 10s;' }); > + var div = createDiv(test); > + div.classList.add('pseudo'); > + assert_equals(document.getAnimations().length, 1); Should this just get the last animation in the list and verify that it has the right target? Then you could call createPseudo in a test that has already generated an animation?
Attachment #8726634 -
Flags: review?(bbirtles) → review+
Updated•9 years ago
|
Attachment #8726635 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #25) > > + assert_equals(document.getAnimations().length, 1); > > Should this just get the last animation in the list and verify that it has > the right target? Then you could call createPseudo in a test that has > already generated an animation? OK. I agree. We can remove this assert (no need to check we have '1' animation) and revise the second one to: var anims = document.getAnimations(); assert_class_string(anims[anims.length-1].effect.target, 'CSSPseudoElement'); So we just check the last animation and make sure it is a CSSPseudoElement target.
Comment 27•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #26) > (In reply to Brian Birtles (:birtles) from comment #25) > > > + assert_equals(document.getAnimations().length, 1); > > > > Should this just get the last animation in the list and verify that it has > > the right target? Then you could call createPseudo in a test that has > > already generated an animation? > > OK. I agree. We can remove this assert (no need to check we have '1' > animation) and revise the second one to: > > var anims = document.getAnimations(); > assert_class_string(anims[anims.length-1].effect.target, 'CSSPseudoElement'); > > So we just check the last animation and make sure it is a CSSPseudoElement > target. Sounds good. We could also assert that: * anims.length >= 1 * effect.target.parentElement equals |div| * effect.target.type equals |type| (although this would be redundant since if we passed the above test there couldn't possibly be another animation targeting |div|) The only reason that I'm suggesting these assertions is that if we start using this in a complex test, we might find that the newly added animation is *not* the last one in getAnimations(). If that's the case, it would be good to know that (rather than have the test fail mysteriously, or pass for the wrong reason).
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #27) > Sounds good. We could also assert that: > * anims.length >= 1 > * effect.target.parentElement equals |div| > * effect.target.type equals |type| (although this would be redundant since > if we passed the above test there couldn't possibly be another animation > targeting |div|) Cool. Thanks. Let's use these three assertions. > > The only reason that I'm suggesting these assertions is that if we start > using this in a complex test, we might find that the newly added animation > is *not* the last one in getAnimations(). If that's the case, it would be > good to know that (rather than have the test fail mysteriously, or pass for > the wrong reason). Got it. It makes sense.
Assignee | ||
Comment 29•9 years ago
|
||
Updated: address birtles' comments.
Attachment #8727241 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8726634 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8726635 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c31ffcf7874b
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb7c63a1154 https://hg.mozilla.org/integration/mozilla-inbound/rev/e289c18ace28 https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a3f5ba34b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c532283a04f
Keywords: checkin-needed
Comment 33•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9bb7c63a1154 https://hg.mozilla.org/mozilla-central/rev/e289c18ace28 https://hg.mozilla.org/mozilla-central/rev/e1a3f5ba34b3 https://hg.mozilla.org/mozilla-central/rev/2c532283a04f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•