Closed Bug 435442 Opened 17 years ago Closed 14 years ago

Implement Webkit's CSS Animation proposal

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: zwol, Assigned: dbaron)

References

()

Details

(Keywords: css3, dev-doc-complete, Whiteboard: [css-animations])

Attachments

(16 files, 12 obsolete files)

(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
The Webkit team has proposed a CSS extension for explicit specification of animations: http://webkit.org/specs/CSSVisualEffects/CSSAnimation.html It would be nice to implement this in Mozilla as well (using -webkit- names for compatibility, until standardized).
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
Status: NEW → ASSIGNED
Flags: wanted1.9.2?
The draft spec has moved to w3.org, updating the bug's URL accordingly.
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
This is a mass change. Every comment has "assigned-to-new" in it. I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Status: ASSIGNED → NEW
How does this bug relate to columns support - CSS3 (MultiCol) as per https://developer.mozilla.org/en/Mozilla_CSS_support_chart?
It doesn't.
Keywords: css3
Whiteboard: [css-animations]
Target Milestone: mozilla2.0 → ---
Depends on: 636029
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/8a3a30ec0d54/css-animations-properties is a patch to parse and compute the values of the animation-* properties. (There's still a lot more work here: parsing @keyframes rules, and actually doing the animations.)
Depends on: 577974
Attached patch patch 1: add new properties (obsolete) (deleted) — Splinter Review
Much like transitions.
Attachment #523699 - Flags: review?(bzbarsky)
Attached patch patch 2: share GetCSSParsingEnvironment (obsolete) (deleted) — Splinter Review
Attachment #523700 - Flags: review?(bzbarsky)
Attached patch patch 4: fix style rule inherit macros (obsolete) (deleted) — Splinter Review
Attachment #523702 - Flags: review?(bzbarsky)
Attached patch patch 5: implement and parse @keyframes rules (obsolete) (deleted) — Splinter Review
Attachment #523703 - Flags: review?(bzbarsky)
Much like @font-face rules.
Attachment #523704 - Flags: review?(bzbarsky)
Patch 9 (doing the animation) is still to come; it works in simple cases, but needs a lot more tests, and probably a bunch of bug fixes to match.
Comment on attachment 523699 [details] [diff] [review] patch 1: add new properties Actually, hold off on patch one. I meant to stick in another patch before patch one to deal with the steps() function, and apply the steps() and step-end/step-start to transitions as well. So hold
Attachment #523699 - Flags: review?(bzbarsky)
Attachment #523743 - Flags: review?(bzbarsky)
Attached patch patch 1: add new properties (deleted) — Splinter Review
Attachment #523699 - Attachment is obsolete: true
Attachment #523744 - Flags: review?(bzbarsky)
Attachment #523705 - Attachment is obsolete: true
Attachment #523745 - Flags: review?(bzbarsky)
Attachment #523705 - Flags: review?(bzbarsky)
Attachment #523744 - Attachment description: patch 2: add new properties → patch 1: add new properties
Attachment #523808 - Flags: review?(bzbarsky)
Er, since refresh drivers are shared, I need a way to restore normalcy.
Attachment #523808 - Attachment is obsolete: true
Attachment #523829 - Flags: review?(bzbarsky)
Attachment #523808 - Flags: review?(bzbarsky)
Assignee: nobody → dbaron
Priority: -- → P4
Status: NEW → ASSIGNED
Attachment #524497 - Flags: review?(bzbarsky)
Attached patch patch 12: animation event structures (obsolete) (deleted) — Splinter Review
This adds animation events, which are almost the same as transition events except for a few names.
Attachment #524534 - Flags: review?(Olli.Pettay)
With a slight tweak to end animations more promptly, since that makes more sense for events (although I explicitly don't test it in the animation test code).
Attachment #524497 - Attachment is obsolete: true
Attachment #524856 - Flags: review?(bzbarsky)
Attachment #524497 - Flags: review?(bzbarsky)
I missed a few pieces related to the events.
Attachment #524534 - Attachment is obsolete: true
Attachment #524857 - Flags: review?(Olli.Pettay)
Attachment #524534 - Flags: review?(Olli.Pettay)
Comment on attachment 524857 [details] [diff] [review] patch 12: animation event structures >+nsresult >+NS_NewDOMAnimationEvent(nsIDOMEvent **aInstancePtrResult, >+ nsPresContext *aPresContext, >+ nsAnimationEvent *aEvent) >+{ >+ nsDOMAnimationEvent *it = new nsDOMAnimationEvent(aPresContext, aEvent); >+ if (!it) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } new is infallible, so no need for OOM check. >+ * The Original Code is nsDOMAnimationEvent. >+ * >+ * The Initial Developer of the Original Code is the Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. 2009? >+class nsDOMAnimationEvent : public nsDOMEvent, >+ public nsIDOMAnimationEvent Nit, extra space before the second 'public' >+{ >+public: >+ nsDOMAnimationEvent(nsPresContext *aPresContext, >+ nsAnimationEvent *aEvent); similar thing here
Attachment #524857 - Flags: review?(Olli.Pettay) → review+
With some minor test updates (be a little stricter about the end boundary condition in one test -- I'd loosened it in the last revision instead of changing it as I probably should).
Attachment #524856 - Attachment is obsolete: true
Attachment #524895 - Flags: review?(bzbarsky)
Attachment #524856 - Flags: review?(bzbarsky)
Layout-dependent computed style was broken on pseudo-elements, as exposed by tests in the next patch. This could perhaps use its own simple test (a very simple one could work), but I didn't bother since it is tested in the next patch.
Attachment #524896 - Flags: review?(bzbarsky)
Attached patch patch 14: fire animation events (deleted) — Splinter Review
Attachment #524897 - Flags: review?(bzbarsky)
Comment on attachment 524897 [details] [diff] [review] patch 14: fire animation events >diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h >--- a/layout/style/nsAnimationManager.h >+++ b/layout/style/nsAnimationManager.h >@@ -59,16 +62,38 @@ class nsAnimationManager : public mozill > public: > nsAnimationManager(nsPresContext *aPresContext) > : mozilla::css::CommonAnimationManager(aPresContext), > mKeyframesListIsDirty(true) > { > mKeyframesRules.Init(16); // FIXME: make infallible! > } > >+ struct AnimationEventInfo { >+ nsCOMPtr<mozilla::dom::Element> mElement; nsRefPtr, right?
Some of these patches required a bit of merging with bug 577976 and perhaps other de-COM patches. I have merged versions in my patch queue; will upload in a bit after testing.
Attachment #523700 - Attachment is obsolete: true
Attachment #525035 - Flags: review?(bzbarsky)
Attachment #523700 - Flags: review?(bzbarsky)
Attachment #523702 - Attachment is obsolete: true
Attachment #525036 - Flags: review?(bzbarsky)
Attachment #523702 - Flags: review?(bzbarsky)
Attached patch patch 5: implement and parse @keyframes rules (obsolete) (deleted) — Splinter Review
Attachment #523703 - Attachment is obsolete: true
Attachment #525037 - Flags: review?(bzbarsky)
Attachment #523703 - Flags: review?(bzbarsky)
Attachment #524895 - Attachment is obsolete: true
Attachment #525038 - Flags: review?(bzbarsky)
Attachment #524895 - Flags: review?(bzbarsky)
CommonAnimationManager needs a virtual destructor to avoid leaking during pretty much any unit tests.
Attachment #523745 - Attachment is obsolete: true
Attachment #525042 - Flags: review?(bzbarsky)
Attachment #523745 - Flags: review?(bzbarsky)
Comment on attachment 525035 [details] [diff] [review] patch 2: share GetCSSParsingEnvironment >+ nsresult GetCSSParsingEnvironmentForRule(nsICSSRule* aRule, This can be a static method, right? r=me with that. Also, can you please file a followup bug on me to change this method to not need to addref all those options?
Attachment #525035 - Flags: review?(bzbarsky) → review+
(In reply to comment #37) > >+ nsresult GetCSSParsingEnvironmentForRule(nsICSSRule* aRule, > > This can be a static method, right? right. > Also, can you please file a followup bug on me to change this method to not > need to addref all those options? Filed bug 649163.
Comment on attachment 523701 [details] [diff] [review] patch 3: handle modifications coming from grandchildren of sheet better >- sheet->SetModified(PR_FALSE); >+ NS_ABORT_IF_FALSE(!sheet->IsModified(), >+ "should not get marked modified during parsing"); I believe this change is wrong (and the one in css::Loader). In particular, parsing an @import rule will call css::Loader::LoadChildSheet which calls css::Loader::InsertChildSheet which calls nsCSSStyleSheet::AppendStyleSheet which calls DidDirty(). I'm somewhat surprised this abort wasn't hit in testing..... We probably need to guard the DidDirty() call or the mDirty set in DidDirty() on mInner->mComplete. Same for the quivalent CSSLoader code. >+++ b/layout/style/nsCSSStyleSheet.cpp > nsCSSStyleSheet::EnsureUniqueInner() > { >+ mDirty = PR_TRUE; This is wrong too. It will cause use to treat as non-dirty sheets that may well have been modified. Say a sheet is modified and then GetItemAt is called on its .rules. This change will cause mDirty to be set to false. Then the css::Loader will think it can coalesce other sheets with this one based on URI (see the IsModified() check in Loader::CreateSheet), which is wrong.
Attachment #523701 - Flags: review?(bzbarsky) → review-
Er, ignore comment 39. I misread the EnsureUniqueInner change.
Comment on attachment 523701 [details] [diff] [review] patch 3: handle modifications coming from grandchildren of sheet better r=me
Attachment #523701 - Flags: review- → review+
Comment on attachment 523743 [details] [diff] [review] patch 0: implement step-start, step-end, and steps() r=me
Attachment #523743 - Flags: review?(bzbarsky) → review+
Attachment #525036 - Flags: review?(bzbarsky) → review+
Attached patch patch 15: #ifdef the feature (deleted) — Splinter Review
Attachment #525288 - Flags: review?(bzbarsky)
I noticed the StyleRule.h changes were a remnant of an earlier approach, so removed them.
Attachment #525037 - Attachment is obsolete: true
Attachment #525293 - Flags: review?(bzbarsky)
Attachment #525037 - Flags: review?(bzbarsky)
Comment on attachment 523744 [details] [diff] [review] patch 1: add new properties >+++ b/layout/style/nsCSSParser.cpp >@@ -4474,18 +4488,22 @@ CSSParserImpl::ParseVariant(nsCSSValue& We should probably assert up front that the variant mask doesn't have both VARIANT_IDENTIFIER and VARIANT_IDENTIFIER_NO_INHERIT set. >+ if (((aVariantMask & >+ (VARIANT_IDENTIFIER | VARIANT_IDENTIFIER_NO_INHERIT)) != 0) && >+ (eCSSToken_Ident == tk->mType) && >+ ((aVariantMask & VARIANT_IDENTIFIER) != 0 || >+ !(tk->mIdent.LowerCaseEqualsLiteral("inherit") || >+ tk->mIdent.LowerCaseEqualsLiteral("initial")))) { And then I think I would prefer that last clause after the tk->mType check to read: !((aVariantMask & VARIANT_IDENTIFIER_NO_INHERIT) && (tk->mIdent.LowerCaseEqualsLiteral("inherit") || tk->mIdent.LowerCaseEqualsLiteral("initial"))) but it's not a terribly strong preference. >+CSSParserImpl::ParseAnimationOrTransitionShorthand( >+ for (;;) { // loop over comma-separated transitions "or animations" >+ // whether a particular subproperty was specified for this transition "or animation" >+ for (;;) { // loop over values within a transition "or animation" >+ // check to see if we're at the end of one full transition definition "or animation definition" >+ // else, try to parse the next transition sub-property "or animation sub-property" >+ // We hit the end of the property or the end of one transition "or animation" >@@ -3873,16 +3894,331 @@ nsRuleNode::ComputeDisplayData(void* aSt >+ case eCSSUnit_Enumerated: >+ animation->SetIterationCount(NS_IEEEPositiveInfinity()); Could use an assert on the value of the enumeration there. r=me with those nits.
Attachment #523744 - Flags: review?(bzbarsky) → review+
Comment on attachment 525293 [details] [diff] [review] patch 5: implement and parse @keyframes rules r=me with the comment nit I mentioned.
Attachment #525293 - Flags: review?(bzbarsky) → review+
Comment on attachment 523704 [details] [diff] [review] patch 6: cascade @keyframes rules >+++ b/layout/style/nsCSSRuleProcessor.cpp >+// Append all the currently-active font face rules to aArray. Return s/font face/key frame/ r=me with that.
Attachment #523704 - Flags: review?(bzbarsky) → review+
Comment on attachment 525042 [details] [diff] [review] patch 7: move parts of transitions code into common base class to share with animation >+++ b/layout/style/AnimationCommon.cpp >+ // Content nodes might outlive the transition manager. s/transition/animation/ >+AnimValuesStyleRule::MapRuleInfoInto(nsRuleData* aRuleData) >+ // Don't apply transitions to things inside of pseudo-elements. "or animations" r=me
Attachment #525042 - Flags: review?(bzbarsky) → review+
Comment on attachment 523706 [details] [diff] [review] patch 8: make AddEmptyValue explicitly Infallible, and assume it is Fix the comment about "non-null" too? r=me
Attachment #523706 - Flags: review?(bzbarsky) → review+
Comment on attachment 523829 [details] [diff] [review] patch 9: allow tests to take over control of refresh driver time and refreshes r=me
Attachment #523829 - Flags: review?(bzbarsky) → review+
Comment on attachment 523809 [details] [diff] [review] patch 10: copy some bits of test_transitions.html into a shared file r=me
Attachment #523809 - Flags: review?(bzbarsky) → review+
Comment on attachment 524896 [details] [diff] [review] patch 13: fix layout-dependent computed style for pseudo-elements r=me
Attachment #524896 - Flags: review?(bzbarsky) → review+
Comment on attachment 525038 [details] [diff] [review] patch 11: implement and test animations of css animations >+++ b/layout/style/nsAnimationManager.cpp >+ * Data about one animation (i.e., one of the values of >+ * 'animation-name') animation running on an element. s/animation running/running/ >+nsAnimationManager::BuildSegment(nsTArray<AnimationSegment, >+ nsTArrayInfallibleAllocator>& aSegments, InfallibleTArray, and same in the header. r=me
Attachment #525038 - Flags: review?(bzbarsky) → review+
I filed bug 649238 as a followup (to a FIXME in patch 11) on removing from the refresh driver more aggressively.
Comment on attachment 524897 [details] [diff] [review] patch 14: fire animation events r=bzbarsky
Attachment #524897 - Flags: review?(bzbarsky) → review+
Comment on attachment 525288 [details] [diff] [review] patch 15: #ifdef the feature r=me
Attachment #525288 - Flags: review?(bzbarsky) → review+
(In reply to comment #58) > bustage fix for mac compiler: > http://hg.mozilla.org/mozilla-central/rev/a1ab5a4ed1f3 And I double-checked the mochitest log for the 32-bit Mac optimized run of test_value_computation.html (and some of the other related mochitests) to make sure the animation-iteration-count output looked as I expected it to.
And, for the record, the error that the bustage fix in comment 58 was fixing was: /var/folders/7I/7I253dv+HLesSBUBGCX08E+++TM/-Tmp-//ccFCi3BG.s:6479:non-relocatable subtraction expression, "LC14" minus "L00000000025$pb" /var/folders/7I/7I253dv+HLesSBUBGCX08E+++TM/-Tmp-//ccFCi3BG.s:6479:symbol: "L00000000025$pb" can't be undefined in a subtraction expression /var/folders/7I/7I253dv+HLesSBUBGCX08E+++TM/-Tmp-//ccFCi3BG.s:unknown:Undefined local symbol L00000000025$pb make[7]: *** [nsComputedDOMStyle.o] Error 1
Depends on: 650469
Depends on: 652976
Depends on: CVE-2012-1953
Blocks: 829369
Depends on: 1332550
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: