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)
Core
CSS Parsing and Computation
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).
Updated•17 years ago
|
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 1•15 years ago
|
||
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-
Comment 2•15 years ago
|
||
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
Comment 3•14 years ago
|
||
How does this bug relate to columns support - CSS3 (MultiCol) as per https://developer.mozilla.org/en/Mozilla_CSS_support_chart?
Assignee | ||
Comment 4•14 years ago
|
||
It doesn't.
Updated•14 years ago
|
Assignee | ||
Comment 5•14 years ago
|
||
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.)
Assignee | ||
Comment 6•14 years ago
|
||
Much like transitions.
Attachment #523699 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #523700 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #523701 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #523702 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #523703 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 11•14 years ago
|
||
Much like @font-face rules.
Attachment #523704 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #523705 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #523706 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•14 years ago
|
||
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.
Assignee | ||
Comment 15•14 years ago
|
||
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)
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #523743 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #523699 -
Attachment is obsolete: true
Attachment #523744 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #523705 -
Attachment is obsolete: true
Attachment #523745 -
Flags: review?(bzbarsky)
Attachment #523705 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #523744 -
Attachment description: patch 2: add new properties → patch 1: add new properties
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #523808 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #523809 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 21•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee: nobody → dbaron
Priority: -- → P4
Assignee | ||
Comment 22•14 years ago
|
||
Attachment #524497 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 23•14 years ago
|
||
This adds animation events, which are almost the same as transition events except for a few names.
Attachment #524534 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 24•14 years ago
|
||
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)
Assignee | ||
Comment 25•14 years ago
|
||
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 26•14 years ago
|
||
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+
Assignee | ||
Comment 27•14 years ago
|
||
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)
Assignee | ||
Comment 28•14 years ago
|
||
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)
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #524897 -
Flags: review?(bzbarsky)
Comment 30•14 years ago
|
||
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?
Assignee | ||
Comment 31•14 years ago
|
||
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.
Assignee | ||
Comment 32•14 years ago
|
||
Attachment #523700 -
Attachment is obsolete: true
Attachment #525035 -
Flags: review?(bzbarsky)
Attachment #523700 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 33•14 years ago
|
||
Attachment #523702 -
Attachment is obsolete: true
Attachment #525036 -
Flags: review?(bzbarsky)
Attachment #523702 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 34•14 years ago
|
||
Attachment #523703 -
Attachment is obsolete: true
Attachment #525037 -
Flags: review?(bzbarsky)
Attachment #523703 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 35•14 years ago
|
||
Attachment #524895 -
Attachment is obsolete: true
Attachment #525038 -
Flags: review?(bzbarsky)
Attachment #524895 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 36•14 years ago
|
||
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 37•14 years ago
|
||
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+
Assignee | ||
Comment 38•14 years ago
|
||
(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 39•14 years ago
|
||
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-
Comment 40•14 years ago
|
||
Er, ignore comment 39. I misread the EnsureUniqueInner change.
Comment 41•14 years ago
|
||
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 42•14 years ago
|
||
Comment on attachment 523743 [details] [diff] [review]
patch 0: implement step-start, step-end, and steps()
r=me
Attachment #523743 -
Flags: review?(bzbarsky) → review+
Updated•14 years ago
|
Attachment #525036 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 43•14 years ago
|
||
Attachment #525288 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 44•14 years ago
|
||
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 45•14 years ago
|
||
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 46•14 years ago
|
||
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 47•14 years ago
|
||
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 48•14 years ago
|
||
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 49•14 years ago
|
||
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 50•14 years ago
|
||
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 51•14 years ago
|
||
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 52•14 years ago
|
||
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 53•14 years ago
|
||
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+
Assignee | ||
Comment 54•14 years ago
|
||
I filed bug 649238 as a followup (to a FIXME in patch 11) on removing from the refresh driver more aggressively.
Comment 55•14 years ago
|
||
Comment on attachment 524897 [details] [diff] [review]
patch 14: fire animation events
r=bzbarsky
Attachment #524897 -
Flags: review?(bzbarsky) → review+
Comment 56•14 years ago
|
||
Comment on attachment 525288 [details] [diff] [review]
patch 15: #ifdef the feature
r=me
Attachment #525288 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 57•14 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6bf7649f64a6
https://hg.mozilla.org/mozilla-central/rev/0a0314bdf5c6
https://hg.mozilla.org/mozilla-central/rev/b579b02a57af
https://hg.mozilla.org/mozilla-central/rev/9e703bf91ff5
https://hg.mozilla.org/mozilla-central/rev/2597d6ff2793
https://hg.mozilla.org/mozilla-central/rev/f8dba37f4761
https://hg.mozilla.org/mozilla-central/rev/23d79d8f5eda
https://hg.mozilla.org/mozilla-central/rev/548241dd0c12
https://hg.mozilla.org/mozilla-central/rev/f4d2a9cb8e06
https://hg.mozilla.org/mozilla-central/rev/618c5d784dac
https://hg.mozilla.org/mozilla-central/rev/6ab8e5df08ec
https://hg.mozilla.org/mozilla-central/rev/1c17ed72040c
https://hg.mozilla.org/mozilla-central/rev/3a3c77941d26
https://hg.mozilla.org/mozilla-central/rev/6645b30313c5
https://hg.mozilla.org/mozilla-central/rev/37cc67bd29b0
https://hg.mozilla.org/mozilla-central/rev/5f6f0204b682
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Assignee | ||
Comment 58•14 years ago
|
||
bustage fix for mac compiler:
http://hg.mozilla.org/mozilla-central/rev/a1ab5a4ed1f3
Assignee | ||
Comment 59•14 years ago
|
||
(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.
Assignee | ||
Comment 60•14 years ago
|
||
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
Comment 61•14 years ago
|
||
Documentation has been written; a review would be appreciated:
https://developer.mozilla.org/en/CSS/CSS_animations
https://developer.mozilla.org/en/DOM/event/AnimationEvent
https://developer.mozilla.org/en/CSS/animation
https://developer.mozilla.org/en/CSS/animation-name
https://developer.mozilla.org/en/CSS/animation-duration
https://developer.mozilla.org/en/CSS/animation-timing-function
https://developer.mozilla.org/en/CSS/animation-iteration-count
https://developer.mozilla.org/en/CSS/animation-direction
https://developer.mozilla.org/en/CSS/animation-play-state
https://developer.mozilla.org/en/CSS/animation-delay
https://developer.mozilla.org/en/CSS/@keyframes
Links have also been added to Firefox 5 for developers and to the main CSS Reference index page.
Keywords: dev-doc-needed → dev-doc-complete
Updated•12 years ago
|
Depends on: CVE-2012-1953
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•