Closed Bug 435442 Opened 16 years ago Closed 13 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)

7.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
36.13 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
79.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.22 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
26.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
25.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.94 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
103.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
36.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
82.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
47.56 KB, 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?
Keywords: css3
Whiteboard: [css-animations]
Target Milestone: mozilla2.0 → ---
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.)
Attached patch patch 1: add new properties (obsolete) — Splinter Review
Much like transitions.
Attachment #523699 - 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 #523699 - Attachment is obsolete: true
Attachment #523744 - Flags: review?(bzbarsky)
Attachment #523744 - Attachment description: patch 2: add new properties → patch 1: add new properties
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
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)
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)
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+
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
See Also: → 978712
Depends on: 1332550
You need to log in before you can comment on or make changes to this bug.