The Webkit team has proposed a CSS extension for transitions between property values: http://webkit.org/specs/CSSVisualEffects/CSSTransitions.html It would be nice to implement this in Mozilla as well (using -webkit- names for compatibility, until standardized).

Two months ago I played with smooth animations in Firefox and created this nsIAnimatedTransition interface. It might be helpful for this bug, so I'm attaching it here. It's used like this: 1. Create the animation object: this._smoothScrollAnimation = Components.classes["@mozilla.org/animated-transition;1"].createInstance(Components.interfaces.nsIAnimatedTransition); 2. Initialize the animation: this.smoothScrollAnimation.initWithVelocities(Date.now(), 500, startX, this._targetScrollPosition, curVelocity, 0, 0.42, 0.42); 3. Set up a timer and periodically query valueAt(Date.now()): var newX = self.smoothScrollAnimation.valueAt(Date.now()); 4. Stop the timer when the animation is finished: if (self.smoothScrollAnimation.animationFinished(Date.now())) self._stopSmoothScroll(); (The example code is from an experimental implementation of smooth tab bar scrolling; I can attach that patch as well.) The animation curve is set up using a cubic bezier function with implicit start and end control points (0,0) and (1,1) as suggested in the Webkit transition proposal. The x-coordinates of the other two control points are clipped to [0,1] so that the resulting curve is a proper function. The y-coordinates aren't clipped in order to enable smooth changes of direction (e.g. when scrolling). This bug probably doesn't need the "initWithVelocities" and "velocityAt" parts; however, they're very useful for smooth scrolling. And I might add that most of the math was inspired by Webkit's AnimationController.cpp.

I'm currently working on this. We've got the CSS parsing mostly implemented, I'll try to post an initial patch fairly soon.

This is probably nowhere near ready to be applied, but I would really appreciate at least an informal review on the code so far. As this is my first real attempt at hacking on mozilla code, there may be things here that are obviously incorrect to somebody with more experience. Any suggestions for how to implement the transitions in the rendering code are also welcome.

So, without looking at the patch in any more detail than the list of files changed, I notice that you didn't change layout/style/test/property_database.js . I would suggest: * making appropriate additions to that file * running the mochitests in layout/style/ , which exercise a number of aspects of the style system code for all CSS properties, and getting them to pass though to get to that point, you'd also need to make changes to nsRuleNode, etc. I would suggest looking at other recent patches that have introduced new CSS properties. Many of the "diff" links in http://hg.mozilla.org/mozilla-central/index.cgi/log/tip/layout/style/nsCSSPropList.h should be helpful. I would also suggest that you *not* add a new nsCSSTransition struct, but instead put the new properties in an existing struct. Adding a new struct will make the rule node changes harder. (You need to add member variables to the structs in both nsCSSStruct and the structs in nsStyleStruct. There is a 1:many correspondence between those structs; for any nsStyleStruct struct, there is exactly one appropriate nsCSSStruct struct. I'd also note that each nsStyleStruct struct contains only inherited or only non-inherited properties. Perhaps these should go in nsStyleDisplay or nsStyleVisibility depending on whether they are inherited or not, and then in nsCSSDisplay to correspond. And while adding a new nsCSS* struct isn't too hard, adding a new nsStyle* is a bit more work.) I would also avoid putting the vendor prefix in the code anywhere more than necessary (e.g., don't put it in NS_STYLE_* constant names), since the idea of vendor prefixes is that they eventually go away. I haven't looked at the patch too closely, though. Let me know if there's something in particular you want me to look at more closely at this point.

Thanks for the feedback, it's much appreciated. I'm working to address some of the comments. I hope you don't mind me asking a few more questions. The -webkit-transition property is quite unique compared to other CSS properties because of a few things: a) it is defined as a shorthand (-webkit-transition) and a set of 'longhand' properties (-duration, -delay, -property, -timing-function), but each of the 'longhand' properties don't make much sense on their own. (e.g. a 1-second transition doesn't make any sense until you know which property it applies to). By contrast, most existing CSS properties that have a shorthand version can also stand on their own (e.g. border-top makes sense without any other information). This is similar to how the multiple-backgrounds is defined in CSS3, but different than most other CSS properties. b) the -webkit-transition property is not really a property of an html element, it's really a property of other CSS properties. E.g. it changes the behavior of existing CSS properties, but doesn't really change the static representation of an html element. Because of these considerations, I don't know if it makes sense to store the parsed values in the same way that other CSS values are stored. Do you have any thoughts on the matter? Also, when I was reading http://developer.mozilla.org/en/docs/Adding_a_new_style_property#CSSStyleRule, it mentioned modifying Declaration mapping functions, but I wasn't able to find such a thing. Is that document out of date or am I missing something? Also, in nsComputedDOMStyle.cpp, there are several getter functions for style properties (for example, GetOpacity()), but I wasn't able to find anywhere that they were actually called. Would you be able to give me a little bit of insight about what these functions are and where they are used and whether we need to provide functions for the transition properties? Thanks.

(In reply to comment #5) > Because of these considerations, I don't know if it makes sense to store the > parsed values in the same way that other CSS values are stored. Do you have any > thoughts on the matter? It would take me a while to figure this out. That's essentially the hard part of implementing this. > Also, when I was reading > http://developer.mozilla.org/en/docs/Adding_a_new_style_property#CSSStyleRule, > it mentioned modifying Declaration mapping functions, but I wasn't able to find > such a thing. Is that document out of date or am I missing something? It's out of date. Look at the URL I gave in comment 4 instead. > Also, in nsComputedDOMStyle.cpp, there are several getter functions for style > properties (for example, GetOpacity()), but I wasn't able to find anywhere that > they were actually called. Would you be able to give me a little bit of insight > about what these functions are and where they are used and whether we need to > provide functions for the transition properties? They're called from macros. See the other patches in the URL in comment 4.

Comment on attachment 322271 [details] [diff] [review] experimental nsIAnimatedTransition implementation (moved to bug 447856) I've filed bug 447856 for the nsIAnimatedTransition helper.

OK, here's an updated patch implementing the backend functionality for CSS transitions. I tried to address most of your previous comments, and this version is still certainly incomplete and probably does things 'wrong' in some places, but I could really use some good review and feedback on this patch. This patch does not do any actual rendering or animation yet. It should apply cleanly to revision 0d6458ad345d By the way, I did run the mochitests and made very good progress on reducing the test failures for the new properties, but there are failures yet. It wasn't obvious to me how to fix the remaining failures, so I am attaching the patch here in the hopes that your feedback may point the way toward fixes for these test failures as well.

Could you attach the list of test failures (e.g., run mochitests with --log-file=filename) so that I don't have to apply and build the patch to find what failures you're hitting?

certainly, here's the gzipped test log output from the following: $ python ../ff-build/_tests/testing/mochitest/runtests.py --test-path=layout/style/ --log-file=transitions-test-results.txt

Also, thinking ahead to doing the actual animations, we need to detect and 'intercept' when a particular element's style is changed This could be in response to setting the property via the DOM (e.g. element.style.width='100%') or for example because an element has a :hover style and the user has hovered over it. Does anybody have any ideas about where is the best place to detect both of these things? Which class or classes should I be looking at? Ideally we would also have access to the old and new style values at that point as well.

Oh, by the way, I've noticed at least one obvious problem with the patch. Several of the functions are accidentally defined inside an #ifdef MOZ_SVG block. I am planning to wait until I got some feedback on the overall approach / design before updating the patch, but please let me know if you'd like me to make the fixes first.

It looks like a bunch of the test failures are related to serialization bugs. In other words, what happens when you set element.style.propertyname or rule.style.propertyname and then read it again, or the code that's doing serialization of window.getComputedStyle(element, "").propertyname. The tests do things like parse a specific value, reserialize it using one of the two methods (since there's different code when serializing the stored rules or the computed style), and then try reparsing it and then serializing a second time to make sure the same result comes out. I think the use of a single nsTransition object rather than a set of separate linked lists in nsStyleDisplay is actually going to lead to some subtle bugs. The spec is unclear about what happens when the lists specified by the different transition properties are different lengths. However, no matter what, you have to get a bunch of interesting cases right, such as: p { transition-property: color, left, width; transition-duration: 2s; } p#foo { transition-property: left; } In such a case, you should definitely end up with only one transition in the list, and only one item in the computed value. Likewise for inheritance. I'm also not sure why mProperty, mPropertyNone, and mPropertyAll are separate members. It looks like the spec only allows none and all to be specified on their own, and not as part of a list. In any case, the spec needs to be clarified as far as handling of lists of different lengths. I'd also note that this is by far the easier part of the work to implement transitions; actually making the properties do something is a lot more work, and I'm not even sure how to do it -- it requires a good bit of thought. Perhaps more in a bit...

Yes, I agree that there are a lot of 'interesting' cases to get right here. However, I did talk with dave hyatt a bit when starting to do this work and he told me that although the spec isn't explicit about it, the proper behavior for the 'jagged array' problem is to handle it in the same way as you would handle the multiple-background specification from CSS3. So my implementation attempts to do that, though there obviously may be bugs. Regarding the use of the nsTransition class vs separate linked lists, my opinion was that it would make the *use* of transitions (by the rendering/ animation classes) much easier, even though it may make the parsing/setup of the transition objects a bit more tricky. I still think that, but I'm open to being convinced otherwise. Also, I definitely agree that the attached code is definitely the easier part of this problem. Since posting this patch, I've been working on the actual animation functionality and have made some progress, but will probably need quite a bit of guidance to get it right. I've been waiting until I get some feedback on the backend parsing part before bugging you too much about the animation part. If you'd prefer me to start posting thoughts or ideas about the animation implementation, please let me know.

I'd definitely suggest posting thoughts before implementing, since we might tell you to implement an entirely different way. I'd been hoping to talk Zack into reviewing your patch in detail...

And, for what it's worth, I posted some work in progress on multiple backgrounds in bug 322475.

It's on my list, really it is. Gotta finish putting out the fires from the transparency patch first. :-/

OK, so while I wait for feedback on the parsing backend patch, I thought I'd throw out a few ideas for doing the animation. My initial experiment with animation was partially successful (I had width, height, and opacity animating concurrently), but it was quite a hack and in any case I seem to have hit a wall that I think renders my approach unworkable. Let me summarize my current thoughts about performing CSS animations: - in order to start a transition, we need to detect that a style property changed. That change can be due to a variety of things: manipulating an element's style via DOM (e.g. element.style.width=...), changing an actual style declaration (e.g. document.styleSheets[0].cssRules[0].style.width=...), applying a different class to an element (e.g. this.setAttribute('class', "foo")), state changes due to :hover selectors, and probably others. - Once we have detected that a property has changed, we need to be able to look up whether that property has a transition applied to it, so we need access to the full style information of the element, not just the element that is being modified - In order to start the animation, we need access to both the old and new values of the affected property The best place that I have found that seems to satisfy all of these requirements is in nsFrameManager::ReResolveStyleContext(), after it has resolved the style context and has references to both newContext and oldContext. You can look at the newContext's transition property to see which properties have active transitions, check if the values for those properties are different between the old and new style contexts, and start an animation if they are. But that gets us to the question of how to do the animation. In essence, we need to start a timer that fires at the desired frame rate, and on each animation frame we need to update the style property to its new intermediate value. The challenge is that we need to make sure that updating the style property to its intermediate value must not cause us to detect a style change (see previous paragraph) and start a new animation. I haven't come up with a satisfying solution to this problem yet.

(In reply to comment #13) > I think the use of a single nsTransition object rather than a set of separate > linked lists in nsStyleDisplay is actually going to lead to some subtle bugs. > The spec is unclear about what happens when the lists specified by the > different transition properties are different lengths. However, no matter > what, you have to get a bunch of interesting cases right, such as: Actually, I think I take this back; having a single nsTransition is OK, but in order to do this you'll need to store the length of the list for each property so that you can rebuild the nsTransition list each time one of the properties changes.

Let me make sure I understand what you're saying. If you have something like: -webkit-transition-property: length, width, opacity; -webkit-transition-duration: 1s; you'd end up with a list of three transitions: 1s length, 1s width, 1s opacity; Then if you later changed the property to something like: -webkit-transition-property: padding-left; You'd want to end up with a list of only a single transition: '1s padding-left' instead of adding extra -property values to fill up the three existing transitions and ending up with 1s padding-left, 1s padding-left, 1s padding-left. Is that what you're referring to? If so, I'm pretty sure that my patch handles this already. Because I do store the individual property arrays separately in nsCSSDisplay, and I combine them into nsTransition objects in nsRuleNode::ComputeDisplayData() and then store that list in nsStyleDisplay. So when an individual property changed, it would use the separate arrays to re-build the nsTransition list. Or did I misunderstand your comment?

It's not a question of later changes; it's a question of rules overriding another, or of inheritance. When one rule overrides another, ComputeDisplayData can be given an aStartStruct that has computed data for an initial subsequence (in priority order) of the rules that match, and then specified data only for the rules that override those. Likewise, for handling eCSSUnit_Inherit (the 'inherit' value), you don't want to pick up longer lists from different properties. So you need to keep track of the length associated with the list for each property, even if the whole thing is then stored as one list, filled up using repetition.

Ah, you're right, I don't handle that correctly at the moment. I'll work on it. Thanks.

Flagging as wanted-1.9.1-? so that we can get some visibility on this feature. If we're working on it we need to understand if this is going to land in 1.9.1 or if we are going to take it in the next release vehicle. That fact needs to be triaged, hence the flag.

If this became the predominate way of doing transitions it would help accessibility -- it would be possible to turn it off at a global level. http://accessgarage.wordpress.com/2008/09/23/motion-sickness-and-transition-effects/ See also bug 456620 for turning off transitions in the Firefox UI.

Here is a new version of the patch that handles the cascading better by keeping track of which subproperties in the nsTransition class are explicitly specified and which are not. In addition, it has some other changes that fix a variety of test failures. There are still a few test failures, but they're all of the form "Expected '0' but got '0s'"

I think we should also use -moz-prefix, the same as for CSS transforms.

I used the -webkit prefix since it was suggested in comment #1, but if there's consensus on using -moz, I can make the change

> consensus on using -moz, I can make the change I think we should use -moz or -webkit for all CSS transitions/transformations/animations.... otherwise it will confuse people. Btw: does it 'https://bugzilla.mozilla.org/attachment.cgi?id=341065' works for you? I have tried to apply patch and test, but no one transition does not work for me....

no, the patch does not actually perform animation yet. This patch only implements the backend support (CSS Parsing, proper style cascading and inheritance, etc). The patch does not yet actually use the style information to perform the animations

(In reply to comment #28) > > consensus on using -moz, I can make the change > > I think we should use -moz or -webkit for all CSS > transitions/transformations/animations.... otherwise it will confuse people. It should use the -moz- prefix, similar to all other CSS properties implemented in Gecko when the relevant specification is still in draft form (and -webkit- is the reserved prefix for WebKit's implementations).

I discovered a bug in the cascading logic, which is fixed by this updated patch.

Sorry for uploading a new patch so quickly, but I updated to tip and uncovered a problem with handling unspecified (initial) values of shorthand properties incorrectly (e.g. -webkit-transition: 2s width;). These were not serialized correctly because I was accidentally treating the unspecified subproperties as -moz-initial values and the changes for bug #160403 meant that it no longer serialized the shorthand since it thought it was mixed initial and non-initial values) In addition, I discovered a serialization problem when the shorthand property has more than one value, e.g. (-webkit-transition: 1s width linear, 2s height ease;). The serialization forgot to add commas between these two values.

For what it's worth, I added a few comments to https://wiki.mozilla.org/CSS_Transitions (which doesn't seem to have been mentioned here previously).

So I know you're waiting on some stuff from me or others, but it's not entirely clear what. I know you want review on the details of the backend patch, but are there specific questions you have about the design for the remainder? (I don't see all that much in https://wiki.mozilla.org/CSS_Transitions .) Or do you want somebody else to sketch out the design for how the rest should work?

Sorry for the lack of responses here lately. I'm sorry to say that I've been asked to work on different projects, so I'm no longer actively working on this. Basically, the missing piece of the puzzle from my perspective was a way trigger transitions from actual style changes (e.g. a script changin the style via DOM, a :hover event, etc), but not trigger a new transition due to a style change caused by the transition itself. During the meeting late last year, there was discussion about adding a new Rule node to facilitate this, but I was never familiar enough with the rulenode stuff to know exactly how this should work.

I've merged the above patch to tip and I'm working on it; my work is at http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/tip/css-transitions-parsing

For CSS property animation I think we're going to want to end up sharing code between this and bug 474049; I should try to discuss this with dholbert next week when I'm back in the office.

This is a prerequisite for the later patches (not ready yet) but seems slightly more likely to bitrot, and there's no reason it can't go in now. I want to put RuleProcessorData in its own header file so that I can (indirectly) include nsIStyleRuleProcessor.h in nsPresContext.h. (RuleProcessorData needs nsPresContext to be defined; nsIStyleRuleProcessor does not.)

A checkpoint of patch 3 is at: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/a62936e3936e/css-transitions-pass-frame-through and a checkpoint of patch 4 is at: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/a62936e3936e/css-transitions-management (Patch 3 is separate because it's the bit I'd need to remember to revert if I associate transitions with content instead of with frames; more on that in a bit, once I post to www-style about the relevant issues and then summarize all my www-style posts here.) There's still a lot of work to be done, but I do have length-based properties transitioning (that is, they animate smoothly on the screen) with those 4 patches (and possibly depending on something else in my patch queue...).

Comments on patch 1 up to the beginning of nsRuleNode.cpp: >+++ b/layout/style/nsCSSDeclaration.cpp >+ subprops[4] == eCSSProperty_UNKNOWN, I'd really like the "4" here and throughout the rest of this block to have a name. Say kTransitionSubpropertyCount or something? That would make it a lot clearer what we're iterating over in the for loop you have. You could also re-roll the loops checking for null; the compiler can presumably unroll them as needed, right? That'll at least make sure we don't accidentally end up with weird mismatches between parts of this code if the number of subproperties changes. >+++ b/layout/style/nsCSSParser.cpp >@@ -4306,16 +4319,17 @@ CSSParserImpl::TranslateDimension(nsCSSV .... > VARIANT_NONE | \ >+ VARIANT_ALL | \ > VARIANT_NORMAL | \ > VARIANT_SYSFONT Shouldn't VARIANT_CUBIC_BEZIER be added here too? >@@ -4531,16 +4551,23 @@ CSSParserImpl::ParseVariant(nsCSSValue& >+ if (((aVariantMask & VARIANT_CUBIC_BEZIER) != 0) && >+ (eCSSToken_Function == tk->mType)) { >+ if (tk->mIdent.LowerCaseEqualsLiteral("cubic-bezier")) { The indentantion here is all weird, and I'd prefer && to the nested if. >+CSSParserImpl::ParseTransitionTime(nsCSSProperty aPropID) >+ // first see if 'inherit' is specified. If it is, it can be the only thing >+ // specified, so don't attempt to parse any additional properties >+ if (ParseVariant(timeval, VARIANT_INHERIT, nsnull)) { That checks for -moz-initial too, right? Should the comment reflect that? We should really consolidate the pattern we use for this stuff, I think; we do it slightly differently in the various places that parse lists like this.... >+ if (ExpectEndProperty()) { This should be CheckEndProperty(). >+CSSParserImpl::ParseTransitionProperty() >+ // first see if 'inherit' is specified. If it is, it can be the only thing >+ // specified, so don't attempt to parse any additional properties >+ if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE | VARIANT_ALL, >+ nsnull)) { Please make the comment match the code? >+ // Exclude 'none' and 'all' and 'inherit' according to the same >+ // rules as for 'counter-reset'. >+ nsDependentString str(value.GetStringBufferValue()); >+ if (str.EqualsLiteral("none") || str.EqualsLiteral("all") || >+ str.EqualsLiteral("inherit") || str.EqualsLiteral("initial")) { Please make the comment match the code (i.e. mention "initial"). Should this ins fact be checking for "initial"? Or "-moz-initial"? Or both? >+ if (ExpectEndProperty()) { CheckEndProperty() >+CSSParserImpl::ParseTransitionTimingFunction() >+ // first see if 'inherit' is specified. If it is, it can be the only thing >+ // specified, so don't attempt to parse any additional properties Again, this checks for -moz-initial as well. >+ if (ExpectEndProperty()) { CheckEndProperty() >+CSSParserImpl::ParseTransitionTimingFunctionValues(nsCSSValue& aValue) >+ REPORT_UNEXPECTED_EOF(PEColorEOF); That can't be the right string! >+ if (!val) { >+ mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY); >+ } Need to return PR_FALSE if !val, right? And fix the indent? >+ float x1,x2,y1,y2; Spaces after those commas, please. >+ if (tk->mType == eCSSToken_Function){ >+ if (ExpectSymbol('(',PR_FALSE)) { >+ ParseTransitionTimingFunctionValueComponent(x1,','); >+ ParseTransitionTimingFunctionValueComponent(y1,','); >+ ParseTransitionTimingFunctionValueComponent(x2,','); >+ ParseTransitionTimingFunctionValueComponent(y2,')'); >+ } >+ } I doubt you should be ignoring the return value of ParseTransitionTimingFunctionValueComponent, right? If this is supposed to be modeled on how we parse rgb() colors, then you should be copying that code more closely.... Please put spaces after the commas there (before the stop chars). >+CSSParserImpl::ParseTransitionTimingFunctionValueComponent(float& aComponent, >+ REPORT_UNEXPECTED_EOF(PEColorComponentEOF); Wrong string. >+ if (ExpectSymbol(aStop,PR_TRUE)) { >+ return PR_TRUE; Space after that comma, please. And fix the indent? Please generally go through this patch and fix the indentation issues... >+CSSParserImpl::AppendValueToList(nsCSSValueList**& aListTail, If you're adding this, why not use it in all those functions above? >+CSSParserImpl::ParseTransition() >+ eCSSProperty_transition_timing_function, >+ // Must check 'transition-delay' after 'transition-property', since >+ // that's our assumption about what the spec means for the shorthand >+ // syntax (the first time given is the duration, and the second >+ // given is the delay). >+ eCSSProperty_transition_delay, s/'transition-propety'/'transition-duration'/ in that comment? >+ // first see if 'inherit' is specified. If it is, it can be the only thing >+ // specified, so don't attempt to parse any additional properties Again, that includes -moz-initial.... >+ for (PRSize i = 0; i < numProps; ++i) { PRSize doesn't seem like the right type for this. >+ // flags to determine whether a particular subproperty was specified for this >+ // transition >+ PRBool atEOP = PR_FALSE; >+ for (;;) { // loop over comma-separated transitions >+ PRBool parsedProperty[numProps] = >+ { PR_FALSE, PR_FALSE, PR_FALSE, PR_FALSE }; That "flags" comment seems to be misplaced. It should be right before parsedProperty is declared, right? And atEOP could use documenting. >+ if (ExpectEndProperty()) { CheckEndProperty() >+ for (PRSize i = 0; !foundProperty && i < numProps; ++i) { Again, why PRSize? >+ // we're not at a ',' or at the end of a the property but we couldn't s/a the/the/ >+ // we hit the end of the property or the end of one transition >+ // definition, add its components to the list s/,/;/, please. And capitalize and punctuate the sentence? Same for other sentence-like comments everywhere you add them. >+ // if the all of the subproperties were not explicitly specified, fill s/the all/all/ >+ switch (kTransitionProperties[i]) { >+ case eCSSProperty_transition_duration: >+ tempValue.SetFloatValue(0.0, eCSSUnit_Seconds); >+ break; ... >+ case eCSSProperty_transition_delay: >+ tempValue.SetFloatValue(0.0, eCSSUnit_Seconds); >+ break; Why not combine those cases? >+ if (str.EqualsLiteral("inherit") || str.EqualsLiteral("initial")) { Again, should this be checking for -moz-initial? >+ // Save all parsed transition sub-properties in mTempData >+ for (PRSize i = 0; i < numProps; ++i) { Why PRSize? >+++ b/layout/style/nsCSSPropList.h Need to reformat the changes here like the other lines now look, right? The identifiers need to start with "_moz", no? >+++ b/layout/style/nsCSSProps.h >+ * Joanthon Jongsma <jonathon.jongsma@collabora.co.uk>, Collabora Ltd. s/Joanthon/Jonathon/

Comment on attachment 384537 [details] [diff] [review] patch 1, v 6: CSS transitions parsing / storage >+++ b/layout/style/nsRuleNode.cpp >@@ -3369,27 +3370,271 @@ static nsStyleTransformMatrix ReadTransf >+struct TransitionPropInfo { >+ nsCSSValueList* nsRuleDataDisplay::* rdList; >+ PRUint32 nsStyleDisplay::* sdCount; Please document these members. >+// Lined up with mutable array below inside ComputeDisplayData. Lined up in the sense that the properties have to come in the same order? If so, please say that instead of just "lined up". > nsRuleNode::ComputeDisplayData(void* aStartStruct, I guess we need transition stuff often enough that bloating nsStyleDisplay with it is ok, right? >+ // Lined up with const array above. Again, plese document what this really means. >+#define FOR_ALL_TRANSITION_PROPS(var_) for (PRSize var_ = 0; var_ < 4; ++var_) Hey, here's that macro I wanted in nsCSSDeclaration! And again, why PRSize? >+ const nsTransition *inheritedTransition = >+ &parentDisplay->mTransitions[i % parentNumTransitions]; I'm not seeing anything in the draft spec about this. Did I just miss it? Same thing for the handling of different-length lists... But that behavior made sense, whereas I'm not convinced that this one does, expecially when we're talking about transition-property. For one thing, it can lead us to effectively have multiple transitions on the same property in a single display struct, no? >+ } else if (i >= delay.num) { >+ transition->SetDelay(display->mTransitions[i % delay.num].GetDelay()); >+ } As far as I can tell, if we get here, then delay.num is display->mTransitionDelayCount. And this is the case when delay is not explicitly specified, right? So then we're replicating the explicitly-specified values, not setting the original values? That doesn't match the big comment about using default values, does it? Which do we really want? >+ transition->SetDuration( >+ display->mTransitions[i % duration.num].GetDelay()); Should be GetDuration(). Please add tests that would have caught this. >+ default: >+ NS_NOTREACHED("Invalid transition property unit"); >+ case eCSSUnit_All: >+ case eCSSUnit_Initial: Why not put default last? Or if we want to assert and fall through to default value, why not do the same for duration/delay/timing-function? >+ transition->SetProperty( >+ display->mTransitions[i % property.num].GetProperty()); So for transition-property that needs padding out it seems like we're treating unknown properties as unknown but not setting mUnknownProperty. This will crash when getting computed style, no? Or is it not an issue because we don't go past mTransitionPropertyCount there? It still seems weird to me to not just copy the mUnknownProperty here and not have to worry about it being null. >+++ b/layout/style/nsStyleStruct.h >+ PRBool operator!=(const nsTimingFunction& aOther) const >+ { >+ return ((mX1 != mX1) || >+ (mY1 != mY1) || >+ (mX2 != mX2) || >+ (mY2 != mY2)); That's overparenthesized. Please remove the extra parens (or heck, all of them in this case. > PRUint8 mClipFlags; // [reset] see nsStyleConsts.h >+ nsAutoTArray<nsTransition, 1> mTransitions; // [reset] >+ // The number of elements in mTransitions that are not from repeating >+ // a list due to another property being longer. >+ PRUint32 mTransitionTimingFunctionCount, >+ mTransitionDurationCount, >+ mTransitionDelayCount, >+ mTransitionPropertyCount; > PRPackedBool mTransformPresent; // [reset] Whether there is a -moz-transform. Don't you want to put that after the mTransformPresent? Otherwise your packing will be all off.

Comment on attachment 384539 [details] [diff] [review] patch 2: give RuleProcessorData and subclasses their own header r+sr=bzbarsky on patch 2.

patch 2 landed as http://hg.mozilla.org/mozilla-central/rev/db2699e729c3

I need this for the main transitions patch in this bug, and dholbert also needs it for bug 504652, so I'd rather get in in sooner rather than later.

(In reply to comment #41) > clearer what we're iterating over in the for loop you have. You could also > re-roll the loops checking for null; the compiler can presumably unroll them as > needed, right? For what it's worth, that means turning: if (!val[0] || !val[1] || !val[2] || !val[3]) { break; } into: PRBool done = PR_FALSE; for (int i = 0; i < NUM_TRANSITION_SUBPROPS; ++i) { if (!val[i]) { done = PR_TRUE; break; } } if (done) { break; } which I don't think is worth it. I'll just PR_STATIC_ASSERT right above the checks instead.

(In reply to comment #41) > The identifiers need to start with "_moz", no? No. There's no need to rename enums when we drop prefixes. We use the prefix for the enums for some of our prefixed properties but not others; I tend to think we shouldn't in general, but I haven't been enforcing it either way in reviews (though I probably ought to).

(In reply to comment #42) > >+ const nsTransition *inheritedTransition = > >+ &parentDisplay->mTransitions[i % parentNumTransitions]; > > I'm not seeing anything in the draft spec about this. Did I just miss it? > > Same thing for the handling of different-length lists... But that behavior > made sense, whereas I'm not convinced that this one does, expecially when we're > talking about transition-property. For one thing, it can lead us to > effectively have multiple transitions on the same property in a single display > struct, no? The handling of different length lists is per http://lists.w3.org/Archives/Public/www-style/2009May/0154.html , except I wouldn't mind changing it per http://lists.w3.org/Archives/Public/www-style/2009May/0155.html , although that wouldn't affect this code. The only WG resolutions on transitions that I know of are in http://lists.w3.org/Archives/Public/www-style/2009Jul/0050.html but we didn't discuss that issue. However, the idea of the code above was that it would match the handling of different length lists. I just got it wrong. > >+ } else if (i >= delay.num) { > >+ transition->SetDelay(display->mTransitions[i % delay.num].GetDelay()); > >+ } > > As far as I can tell, if we get here, then delay.num is > display->mTransitionDelayCount. And this is the case when delay is not > explicitly specified, right? So then we're replicating the > explicitly-specified values, not setting the original values? That doesn't > match the big comment about using default values, does it? Which do we really > want? Oh, the first paragraph of that big comment is now entirely wrong. The goal was to repeat the values, just like multiple backgrounds. (I rewrote the code, but missed the first paragraph of that comment.)

Comment on attachment 393020 [details] [diff] [review] patch 3: add more parameters to ResolveStyleForRules Document the aRuleNode argument, and looks good.

(In reply to comment #49) > Document the aRuleNode argument, and looks good. It was already documented, but I've improved the comment a bit: + // Get a style context (with the given parent and pseudo-tag) for a + // sequence of style rules consisting of the concatenation of: + // (1) the rule sequence represented by aRuleNode (which is the empty + // sequence if aRuleNode is null or the root of the rule tree), and + // (2) the rules in the |aRules| array. (In reply to comment #42) > >+ transition->SetDuration( > >+ display->mTransitions[i % duration.num].GetDelay()); > > Should be GetDuration(). Please add tests that would have caught this. So I wrote a nice random combinations test of the computed style (which I'll keep), and then realized that that doesn't test it because computed style doesn't reflect the part past the end of the property's count. I'll add a test that would catch this, except it can't be in this patch, but instead will be in the main "implement transitions" patch. (Although I'm not really sure that that test will be reliable to enable for mochitests...) > >+ transition->SetProperty( > >+ display->mTransitions[i % property.num].GetProperty()); > > So for transition-property that needs padding out it seems like we're treating > unknown properties as unknown but not setting mUnknownProperty. This will > crash when getting computed style, no? Or is it not an issue because we don't > go past mTransitionPropertyCount there? It still seems weird to me to not just > copy the mUnknownProperty here and not have to worry about it being null. It's true that it's actually not a real issue (and untestable except via assertions). However, the code as it was would have triggered the assertion in SetProperty in that case, so eventually we'd have noticed the assertion. Actually, I'm not sure any tests in the patch as you saw it would have hit the assertion, but my new test_transitions_computed_value_combinations.html hits it a lot when I reintroduce the bug. At least it's good for something.

I believe this addresses the comments in comment 41 and comment 42. My responses to the comments that needed responses are in comment 46, comment 47, comment 48, and comment 50 (the last 3/4 of it)

Landed patch 3: http://hg.mozilla.org/mozilla-central/rev/929ccf186103

David, how much pain would it be to produce an interdiff between v6 and v7?

Comment on attachment 394147 [details] [diff] [review] interdiff between v6 and v7 >+++ layout/style/nsCSSDeclaration.cpp 2009-08-12 15:52:03.000000000 -0700 >+#undef NUM_TRANSITION_SUBPROPS 4 Don't need the 4 there. >+++ layout/style/nsCSSParser.cpp 2009-08-12 15:52:03.000000000 -0700 > CSSParserImpl::ParseTransitionTimingFunctionValues(nsCSSValue& aValue) >+ float x1,x2,y1,y2; Still need spaces after commas here. >+++ layout/style/nsRuleNode.cpp 2009-08-12 15:52:03.000000000 -0700 >+ } else if (delay.inherited) { >+ // FIXME (for all transition properties): write a test that >+ // detects when this was wrong for i >= delay.num if parent had >+ // count for this property not equal to length >+ NS_ABORT_IF_FALSE(i < parentDisplay->mTransitionDelayCount, >+ "delay.num computed incorrectly"); >+ transition->SetDelay(parentDisplay->mTransitions[i].GetDelay()); Need to set canStoreInRuleTree to false here, right? Similar for the other properties... >+++ layout/style/nsStyleStruct.h 2009-08-12 15:52:03.000000000 -0700 > PRBool operator!=(const nsTimingFunction& aOther) const >+ return mX1 != mX1 || mY1 != mY1 || mX2 != mX2 || mY2 != mY2; Er, and need "aOther." before all the RHS or something, right? I should have caught this the first time.... With that above fixed, looks good.

Comment on attachment 393367 [details] [diff] [review] patch 1, v7: CSS transitions parsing / storage r+ with the comments addressed.

Landed the parsing patch: http://hg.mozilla.org/mozilla-central/rev/aa90f2f56c6b Note that this just means that you can specify the transition properties and they'll show up as far as getComputedStyle(). They don't do anything yet.

We want to treat style changes that are the result of animations differently in terms of starting transitions. So this separates restyles caused by animations from those that aren't, so that we can know which type we're processing.

This adds support for multiple post-resolve callbacks. I have mixed feelings about doing this, since I somewhat dislike post-resolve callbacks. However, I think using post-resolve callbacks for the animation data from transitions is both less work for the programmer (since transitions won't need code to convert from computed values back to specified) and less work at runtime.

Same general thoughts as the previous patch.

This is actually needed for the main patch, believe-it-or-not.

Comment on attachment 396506 [details] [diff] [review] patch 4: make style changes know whether they're for animation or not Can processing animation restyles trigger frame reconstructs? If so, the restyle processing doesn't really look right to me...

What's wrong with it? Is the problem that the order in the queue matters and they need to be processed in the order posted?

No, it's that processing a frame reconstruct can post new frame reconstructs (always into the non-animation hashtable, with your patch) due to our lazy containing block reframes. The loop in the processing method currently takes care of this, but if the reconstructs happen while processing the animation restyles they won't get immediately handled. Which means that a Flush_Frames might leave pending reframes hanging about. Just reversing the order of processing the two tables should fix this, I think.

(In reply to comment #65) > Just reversing the order of processing the two tables should fix this, I think. Except I think I prefer them in the order given because processing non-animation restyles immediately posts an animation restyle for anything that's currently or just starting to animate. (I should probably add a comment about that.) So I can probably make some of the restyle-posting depend on the current is-processing-animation rather than just PR_FALSE. I should also think a little more about whether it's ok to have two restyles posted for the same node.

Comment on attachment 396507 [details] [diff] [review] patch 5: separate nsCSSPropertySet from nsCSSDataBlock so it can be used elsewhere >+++ b/layout/style/nsCSSDataBlock.h >+ nsCSSPropertySet mPropertiesImportant; Not sure whether mImportantProperties would be a better name... Either way, r=bzbarsky

(In reply to comment #67) > Not sure whether mImportantProperties would be a better name... Either way, I think if I wanted to rename this, I'd also want to rename mPropertiesSet. I'm inclined to leave it for now (right now I'm leaving existing names); maybe I'll try to find better names later.

Landed patches 4 through 8: http://hg.mozilla.org/mozilla-central/rev/b32c955f09d3 http://hg.mozilla.org/mozilla-central/rev/2c12630e5db6 http://hg.mozilla.org/mozilla-central/rev/9ef12a27ab14 http://hg.mozilla.org/mozilla-central/rev/c87e6a6a41bb http://hg.mozilla.org/mozilla-central/rev/b3bf933fdce4

This still has some FIXME comments remaining, but the ones that are left are things that I think can be fixed after landing (although a few of them are somewhat thorny). The bulk of this patch is in two new classes: nsTransitionManager starts and animates CSS transitions nsRefreshDriver manages the timing of refreshing the animations, and is intended to either evolve into or be replaced by the refresh timing part of compositor; it might also be useful for managing timing of other things With a workaround for bug 516009, this passed try server unit tests on Windows, but a few of the timing tests ended up slightly out-of-range on Mac, so test_transitions.html still needs a little more tweaking. (Linux hasn't cycled yet, but it passes for me locally.)

+class nsARefreshObserver { nsIRefreshObserver please ... I don't think we need to change our conventions here + * The flush type affects: + * + the order in which the observers are notified (lowest flush + * type to highest, in order registered) + * + which observers get notified when there is a flush during times + * when we're not painting This isn't clear, especially the second part. We don't run observers just because someone did a Flush_Style, right? + nsTArray<nsARefreshObserver*> observers[3] = { + mObservers[0], mObservers[1], mObservers[2] + }; + PR_STATIC_ASSERT(NS_ARRAY_LENGTH(observers) == NS_ARRAY_LENGTH(mObservers)); You could just write nsTArray<nsARefreshObserver*> observers[NS_ARRAY_LENGTH(mObservers)] = { Instead of cloning the arrays and having finished animations remove their observer, it's probably a bit simpler and more efficient to have WillRefresh return true when the animation is finished and false otherwise, and just not have a RemoveObserver method. Then you can use SwapElements to move the observers to a temporary observer array and after you call WillRefresh you re-add the observer to the real observer array if necessary. In nsTransitionManager::StyleContextChanged, should we have an early exit if aNewStyleContext->GetStyleDisplay()->mTransitionPropertyCount is zero? Seems like this will get called a lot during style reresolution.

Typically (views and frames aside), nsIFoo means a subclass of nsISupports, no? nsA* is used for things like nsAString which are not subclasses of nsISupports...

(In reply to comment #73) > Typically (views and frames aside), nsIFoo means a subclass of nsISupports, no? Those are two pretty big asides. But OK.

gcc on Leopard is giving me a lot of warnings about signed/unsigned mismatch on line 832 of nsPresContext.h where we compare a PRBool to a PRPackedBool.

(In reply to comment #72) > + * The flush type affects: > + * + the order in which the observers are notified (lowest flush > + * type to highest, in order registered) > + * + which observers get notified when there is a flush during times > + * when we're not painting > > This isn't clear, especially the second part. We don't run observers just > because someone did a Flush_Style, right? I should make this a bit clearer, but the idea (not yet implemented) is that some types of observers don't need to be notified all the time. For example, if a page isn't visible, we don't need to notify observers that affect only the display; however, when we make it visible we should notify them once before we paint. Likewise, if a tab isn't displayed, we might be suppressing reflows on anything inside the tab unless script causes a flush that requires up-to-date layout data; in such cases we don't need to notify the observers from the timer, but when we're not notifying them from the timer we do need to notify them on-flush (once per refresh cycle). > + nsTArray<nsARefreshObserver*> observers[3] = { > + mObservers[0], mObservers[1], mObservers[2] > + }; > + PR_STATIC_ASSERT(NS_ARRAY_LENGTH(observers) == NS_ARRAY_LENGTH(mObservers)); > > You could just write > > nsTArray<nsARefreshObserver*> observers[NS_ARRAY_LENGTH(mObservers)] = { Hmmm. I could, although it would just default-construct if there's a mismatch, so I'd sort of prefer to leave it. > Instead of cloning the arrays and having finished animations remove their > observer, it's probably a bit simpler and more efficient to have WillRefresh > return true when the animation is finished and false otherwise, and just not > have a RemoveObserver method. Then you can use SwapElements to move the > observers to a temporary observer array and after you call WillRefresh you > re-add the observer to the real observer array if necessary. If I wanted to do this I'd need to have strong references to the observers, and thus make them inherit from nsISupports. I was hoping to avoid that. Cloning arrays before notification is also a reasonably standard technique. > In nsTransitionManager::StyleContextChanged, should we have an early exit if > aNewStyleContext->GetStyleDisplay()->mTransitionPropertyCount is zero? Seems > like this will get called a lot during style reresolution. It's never zero (and defaults to 1), so I don't think it would be particularly useful. The default values are transition-property: all, transition-duration: 0, and transition-delay: 0. That's why I check duration/delay before checking transition-property, though transition-property: none also gets handled quite quickly within the loop.

Should we special-case disp->mTransitionPropertyCount == 1 && disp->mTransitions[0].GetDuration() == 0.0f, then?

Is it worth it? The current code looks like this: PRBool startedAny = PR_FALSE; nsCSSPropertySet whichStarted; ElementTransitions *et = nsnull; for (PRUint32 i = disp->mTransitionPropertyCount; i-- != 0; ) { const nsTransition& t = disp->mTransitions[i]; // Check delay and duration first, since they default to zero, and // when they're both zero, we can ignore the transition. if (t.GetDelay() != 0.0f || t.GetDuration() != 0.0f) { // ... stuff never hit in that case } } if (!startedAny) { return nsnull; // we hit this early return }

Plus if (aNewStyleContext->PresContext()->IsProcessingAnimationStyleChange()) { return nsnull; } nsIAtom *pseudo = aNewStyleContext->GetPseudoType(); if (pseudo && (pseudo != nsCSSPseudoElements::before && pseudo != nsCSSPseudoElements::after)) { return nsnull; } if (aNewStyleContext->GetParent() && aNewStyleContext->GetParent()->HasPseudoElementData()) { // Ignore transitions on things that inherit properties from // pseudo-elements. // FIXME: Add tests for this. return nsnull;

I'm not sure it's worth it, just checking.

(In reply to comment #72) > + * The flush type affects: > + * + the order in which the observers are notified (lowest flush > + * type to highest, in order registered) > + * + which observers get notified when there is a flush during times > + * when we're not painting > > This isn't clear, especially the second part. We don't run observers just > because someone did a Flush_Style, right? OK, I've changed this comment to: * + (in the future) which observers are suppressed when the display * doesn't require current position data or isn't currently * painting, and, correspondingly, which get notified when there * is a flush during such suppression > In nsTransitionManager::StyleContextChanged, should we have an early exit if > aNewStyleContext->GetStyleDisplay()->mTransitionPropertyCount is zero? Seems > like this will get called a lot during style reresolution. I've added: // Return sooner (before the startedAny check below) for the most // common case: no transitions specified. const nsStyleDisplay *disp = aNewStyleContext->GetStyleDisplay(); if (disp->mTransitionPropertyCount == 1 && disp->mTransitions[0].GetDelay() == 0.0f && disp->mTransitions[0].GetDuration() == 0.0f) { return nsnull; } so the current patch is now: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c0318f22892f/css-transitions-management

Comment on attachment 400135 [details] [diff] [review] patch 9: handle starting and animation of CSS transitions (the main patch) Thanks

Checked in attachment 400730 [details] [diff] [review]: http://hg.mozilla.org/mozilla-central/rev/4bb565dd4283

Comment on attachment 402054 [details] [diff] [review] one mPendingRestyles -> aRestyles was missed in patch 4 r=dbaron

Pushed attachment 402054 [details] [diff] [review]: http://hg.mozilla.org/mozilla-central/rev/2882ece4795d

Comment on attachment 400135 [details] [diff] [review] patch 9: handle starting and animation of CSS transitions (the main patch) I assume I'm supposed to be reviewing http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c0318f22892f/css-transitions-management >+++ b/layout/base/nsPresContext.h >+ static nsPresContext* FromRefreshDriver(nsRefreshDriver* aRefreshDriver) { >+ return reinterpret_cast<nsPresContext*>( >+ reinterpret_cast<char*>(aRefreshDriver) - >+ offsetof(nsPresContext, mRefreshDriver)); >+ } Given that this will trigger those silly non-POD warnings and that we just don't have that many nsRefreshDrivers around, why not just give nsRefreshDriver and nsPresContext* member? >+++ b/layout/base/nsFrameManager.cpp It looks like we don't start transitions on the "additional" style contexts in ReResolveStyleContext (e.g. various tree pseudo-stuff). Is that the behavior we want? If not, probably a followup bug to transition those too is fine. >+++ b/layout/base/nsRefreshDriver.cpp All callers of nsRefreshDriver::StartTimer check that mTimer is null first. Why not just push that check into the function itself, with an early return? >+ // Clone observers to deal with removal during notification. Is there a reason to not use nsTObserverArray here? That does involve using a separate iterator object to iterate the array, but handles all the removal stuff for you. The cost is effectively a slightly more limited API plus 2 words per empty array (since nsTObserverArray array uses nsAutoTArray<T,0>). We don't have enough of these around for that to matter, I think, so the only question is which one leads to simpler code (it's a tossup; copying nsTArrays is easy) and has fewer edge-case performance issues. I guess cloning is ok as long as the arrays aren't likely to be big.... >+++ b/layout/base/nsRefreshDriver.h >+ * used by callers who want to start an animation now and what to know "want to know" >+++ b/layout/style/nsStyleSet.cpp >@@ -597,16 +607,20 @@ nsStyleSet::FileRules(nsIStyleRuleProces >+ if (mRuleProcessors[eTransitionSheet]) We always have such a rule processor, don't we? So no point in doing that test.. >@@ -634,16 +648,18 @@ nsStyleSet::WalkRuleProcessors(nsIStyleR >+ if (mRuleProcessors[eTransitionSheet]) >+ (*aFunc)(mRuleProcessors[eTransitionSheet], aData); Same here. >+++ b/layout/style/nsTransitionManager.cpp >+class ElementTransitionsStyleRule : public nsIStyleRule >+class CoverTransitionStartStyleRule : public nsIStyleRule It would be good to document what these classes do. In particular, CoverTransitionStartStyleRule is not really all that clear (to me, so far) on what its function is). >+ friend struct CoveredValue; Why is that needed? CoveredValue has no methods, so you'll never be in code that has the "permissions" of CoveredValue while trying to poke CoverTransitionStartStyleRule. You null-check the return value of ElementData() in MapRuleInfoInto but not in the post-resolve callback. Why is that ok, esp. given your FIXME comment? ElementData() can start returning null only the rule is removed from the rule processor, right? Can that happen during style resolution? >+ nsIAtom *mElementProperty; // the atom we use in mElement's prop table I assume this is safe to not be a strong ptr because it's a static atom? If so, please document that this must be a static atom? >+ElementTransitionsStyleRule::MapRuleInfoInto(nsRuleData* aRuleData) Won't this unnecessarily add ElementTransitionsPostResolveCallback multiple times if we're transitioning multiple properties in the same struct? Might be better to make a copy of aRuleData->mSIDs on the stack and remove bits from it when we add the post-resolve callback for a particular SID. Or something. In practice, there's only ever one struct involved in all the cases where we handle post-resolve callbacks, no? That is, outside the native theme stuff... >+CoverTransitionStartStyleRule::MapRuleInfoInto(nsRuleData* aRuleData) Similar here. >+nsTransitionManager::StyleContextChanged(nsIContent *aElement, >+ // FIXME: When we have multiple continuations, we actually repeat this >+ // for each one, and if we have transitions we create separate cover >+ // rules for each one. However, since we're attaching the transition >+ // data to the element, during the animation we create the same style >+ // rule, so it's not too horrible. I'm not sure I follow.... We'd create a bunch of separate cover rules, agreed. Won't we then end up giving the different continuations different style contexts (pointing to these different cover rules)? That doesn't sound so great... Am I missing something? I guess this explosion of style contexts is just temporary until we process the RestyleForAnimation restyle, but still.... I guess this is ok for now, but we should consider a followup to prevent this. As a separate note, do we really redo selector matching for every continuation when doing ReResolveStyleContext? It looks like we do, that sucks, and we should fix it. Please file a followup bug? >+ et = GetElementTransitions(aElement, >+ aNewStyleContext->GetPseudoType(), >+ PR_FALSE); There's no need to do that if property == eCSSPropertyExtra_no_properties or eCSSProperty_UNKNOWN, right? Does it matter that we still do it in those cases? >+nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty, >+ // FIXME: This call on the old style context gets incorrect style data >+ // since we don't quite enforce style rule immutability: we didn't >+ // need to worry about callers calling GetStyleData rather than >+ // PeekStyleData after a style rule becomes "old" before transitions >+ // existed. Why do we need GetStyleData here? Because we might need to transition even things no one has asked about (like color in a visibility:hidden subtree)? This part really worries me, though I think it's ok... I don't _think_ we can ever run into crashes this way. I hope. >+ PRUint32 currentIndex = PRUint32(-1); nsTArray_base::NoIndex (and elsewhere in this method). >+ // When we interrupt an running transition, we want to reduce the "a running" I don't see this function setting pt.mUnit anywhere. That seems wrong, except for the fact that the only thing mUnit is used for is to set the CoveredValue mUnit, and this last is unused. Still, this would at least trigger valgrind warnings, and if mUnit is really unused we should just remove it. >+nsTransitionManager::WillRefresh(mozilla::TimeStamp aTime) >+ if (et->mElement->GetCurrentDoc() != mPresContext->Document()) { Hmm. So if I remove a node from the DOM and then reinsert it immediately or after a small delay, then transitions will continue on it(?), but if a WillRefresh call happens while it's outside the DOM the transition will be canceled? That doesn't seem like the right behavior to me. Perhaps it would be better if nsGeneticElement::UnbindFromTree removed all three of the relevant properties from the element if NODE_HAS_PROPERTIES is true on it? Or the ElementTransitions could observe ParentChainChanged on the element and destroy itself or something? >+++ b/layout/style/nsTransitionManager.h >+ * StyleContextChanged Please actually document this.

> nsGeneticElement::UnbindFromTree nsGenericElement, of course.

(In reply to comment #87) > (From update of attachment 400135 [details] [diff] [review]) > I assume I'm supposed to be reviewing > http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c0318f22892f/css-transitions-management Yes. (Or anything newer. It keeps getting tweaked very slightly due to merging.) > >+++ b/layout/base/nsPresContext.h > >+ static nsPresContext* FromRefreshDriver(nsRefreshDriver* aRefreshDriver) { > >+ return reinterpret_cast<nsPresContext*>( > >+ reinterpret_cast<char*>(aRefreshDriver) - > >+ offsetof(nsPresContext, mRefreshDriver)); > >+ } > > Given that this will trigger those silly non-POD warnings and that we just > don't have that many nsRefreshDrivers around, why not just give nsRefreshDriver > and nsPresContext* member? We should really just pass -Wno-invalid-offsetof and stop worrying about those warnings. I guess I'd rather file a followup bug to do that... > >+++ b/layout/base/nsFrameManager.cpp > It looks like we don't start transitions on the "additional" style contexts in > ReResolveStyleContext (e.g. various tree pseudo-stuff). Is that the behavior > we want? If not, probably a followup bug to transition those too is fine. I really wouldn't know where to store the transitions for such things, so I don't see any alternative to stop running the transitions for them. > >+++ b/layout/base/nsRefreshDriver.cpp > > All callers of nsRefreshDriver::StartTimer check that mTimer is null first. > Why not just push that check into the function itself, with an early return? Done, with a rename to EnsureTimerStarted. > >+ // Clone observers to deal with removal during notification. > > Is there a reason to not use nsTObserverArray here? That does involve using a > separate iterator object to iterate the array, but handles all the removal > stuff for you. The cost is effectively a slightly more limited API plus 2 > words per empty array (since nsTObserverArray array uses nsAutoTArray<T,0>). > We don't have enough of these around for that to matter, I think, so the only > question is which one leads to simpler code (it's a tossup; copying nsTArrays > is easy) and has fewer edge-case performance issues. > > I guess cloning is ok as long as the arrays aren't likely to be big.... Yeah, I actually realized the other day (and then forgot before I wrote it down) that what I was doing was probably a bad idea given the weak pointers, since if something gets removed during iteration it's probably important that it actually not get notified. But why in the world does it use the name "Iterator" for enumerators? I guess that's another followup bug. I also used a typedef (ObserverArray) to avoid typing nsTObserverArray<nsARefreshObserver*> too many times. > >+++ b/layout/base/nsRefreshDriver.h > >+ * used by callers who want to start an animation now and what to know > > "want to know" Fixed. > >+++ b/layout/style/nsStyleSet.cpp > >@@ -597,16 +607,20 @@ nsStyleSet::FileRules(nsIStyleRuleProces > >+ if (mRuleProcessors[eTransitionSheet]) > > We always have such a rule processor, don't we? So no point in doing that > test.. > > >@@ -634,16 +648,18 @@ nsStyleSet::WalkRuleProcessors(nsIStyleR > >+ if (mRuleProcessors[eTransitionSheet]) > >+ (*aFunc)(mRuleProcessors[eTransitionSheet], aData); > > Same here. Fixed. > >+++ b/layout/style/nsTransitionManager.cpp > >+class ElementTransitionsStyleRule : public nsIStyleRule > >+class CoverTransitionStartStyleRule : public nsIStyleRule > > It would be good to document what these classes do. In particular, > CoverTransitionStartStyleRule is not really all that clear (to me, so far) on > what its function is). Documentation added. Let me know if it's sufficient. > >+ friend struct CoveredValue; > > Why is that needed? CoveredValue has no methods, so you'll never be in code > that has the "permissions" of CoveredValue while trying to poke > CoverTransitionStartStyleRule. Habit for dealing with nested structs. Also, it may have had values at one point. Removed. > You null-check the return value of ElementData() in MapRuleInfoInto but not in > the post-resolve callback. Why is that ok, esp. given your FIXME comment? > ElementData() can start returning null only the rule is removed from the rule > processor, right? Can that happen during style resolution? If it returns null in MapRuleInfoInto, we never call the post-resolve callback. ElementData() would start returning null if the element is destroyed, if the pres context is destroyed, if the element is removed from the document, or if all transitions on the element end. I don't remember which case I was hitting, but I should probably investigate at some point. > >+ nsIAtom *mElementProperty; // the atom we use in mElement's prop table > > I assume this is safe to not be a strong ptr because it's a static atom? If > so, please document that this must be a static atom? Done > >+ElementTransitionsStyleRule::MapRuleInfoInto(nsRuleData* aRuleData) > > Won't this unnecessarily add ElementTransitionsPostResolveCallback multiple > times if we're transitioning multiple properties in the same struct? Might be > better to make a copy of aRuleData->mSIDs on the stack and remove bits from it > when we add the post-resolve callback for a particular SID. Or something. In > practice, there's only ever one struct involved in all the cases where we > handle post-resolve callbacks, no? That is, outside the native theme stuff... No. It contains a return NS_OK to break out of the loop if it finds a matching property. And MapRuleInfoInto will only be called once per rule per WalkRuleTree. > >+CoverTransitionStartStyleRule::MapRuleInfoInto(nsRuleData* aRuleData) > > Similar here. Same here. > >+nsTransitionManager::StyleContextChanged(nsIContent *aElement, > > >+ // FIXME: When we have multiple continuations, we actually repeat this > >+ // for each one, and if we have transitions we create separate cover > >+ // rules for each one. However, since we're attaching the transition > >+ // data to the element, during the animation we create the same style > >+ // rule, so it's not too horrible. > > I'm not sure I follow.... We'd create a bunch of separate cover rules, agreed. > Won't we then end up giving the different continuations different style > contexts (pointing to these different cover rules)? That doesn't sound so > great... Am I missing something? I guess this explosion of style contexts is > just temporary until we process the RestyleForAnimation restyle, but still.... > I guess this is ok for now, but we should consider a followup to prevent this. > > As a separate note, do we really redo selector matching for every continuation > when doing ReResolveStyleContext? It looks like we do, that sucks, and we > should fix it. Please file a followup bug? Yes, it's temporary until we restyle for animation immediately afterwards, and it would be fixed by better handling of continuations in ReResolveStyleContext (which I was actually somewhat surprised we didn't have). So I think the thing to do is file that followup bug and otherwise leave it. > > >+ et = GetElementTransitions(aElement, > >+ aNewStyleContext->GetPseudoType(), > >+ PR_FALSE); > > There's no need to do that if property == eCSSPropertyExtra_no_properties or > eCSSProperty_UNKNOWN, right? Does it matter that we still do it in those > cases? It doesn't matter, since it's PR_FALSE for aCreateIfNeeded, so it basically initializes et to the element transitions object if there is one, and then ConsiderStartingTransition later updates it if it creates one. I could actually consolidate both calls (in StyleContextChanged and ConsiderStartingTransition) into one (earlier in ConsiderStartingTransition, and passing shouldAnimate for aCreateIfNeeded), though that would have the disadvantage of doing considerably more calls with PR_FALSE if we have repeated !shouldAnimate, which is actually a common case since the default transition-property is 'all' and the common case is relatively few properties changing at once. So I'm leaving it as is. > >+nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty, > >+ // FIXME: This call on the old style context gets incorrect style data > >+ // since we don't quite enforce style rule immutability: we didn't > >+ // need to worry about callers calling GetStyleData rather than > >+ // PeekStyleData after a style rule becomes "old" before transitions > >+ // existed. > > Why do we need GetStyleData here? Because we might need to transition even > things no one has asked about (like color in a visibility:hidden subtree)? Yes. And there are other important cases for this, e.g., two style changes with a flush in between, where the first causes frame reconstruction. > This part really worries me, though I think it's ok... I don't _think_ we can > ever run into crashes this way. I hope. Yeah. It's the thing I dislike the most about this patch. I think it would have been relatively fixable before your recent property-changing optimizations to poke directly into compressed data blocks, but now I'm really not sure. > >+ PRUint32 currentIndex = PRUint32(-1); > > nsTArray_base::NoIndex (and elsewhere in this method). Done, although I used the proper type instead of nsTArray_base. > >+ // When we interrupt an running transition, we want to reduce the > > "a running" Fixed. > I don't see this function setting pt.mUnit anywhere. That seems wrong, except > for the fact that the only thing mUnit is used for is to set the CoveredValue > mUnit, and this last is unused. Still, this would at least trigger valgrind > warnings, and if mUnit is really unused we should just remove it. Yes, mUnit is vestigial code from prior to nsStyleAnimation; I'll remove it. Good catch! > >+nsTransitionManager::WillRefresh(mozilla::TimeStamp aTime) > >+ if (et->mElement->GetCurrentDoc() != mPresContext->Document()) { > > Hmm. So if I remove a node from the DOM and then reinsert it immediately or > after a small delay, then transitions will continue on it(?), but if a > WillRefresh call happens while it's outside the DOM the transition will be > canceled? That doesn't seem like the right behavior to me. > > Perhaps it would be better if nsGenericElement::UnbindFromTree removed all > three of the relevant properties from the element if NODE_HAS_PROPERTIES is > true on it? Or the ElementTransitions could observe ParentChainChanged on the > element and destroy itself or something? Neither of these seems particularly pretty. But adding to nsGenericElement::UnbindFromTree is certainly simpler, so I'll do that. > > >+++ b/layout/style/nsTransitionManager.h > >+ * StyleContextChanged > > Please actually document this. Done. I also made three other changes to the patch since the version you were looking at (plus the usual merging): * increased the tolerance of the test (still not sure it's enough) * Added AssertNoCSSRules and AssertNoImportantRules calls in nsStyleSet * Made a change to check the return value of the second ComputeDistance call in ConsiderStartingTransition, since |endVal| need not be a compatible unit type with the other two. This was caught by another test that I'm still working on (css-transitions-style-animation-tests in my queue).

Also, I just attached a patch in bug 520396 that changes the style rule stuff to no longer use post-resolve callbacks, and also will make it a bit easier to add new properties to nsStyleAnimation.

Comment on attachment 404404 [details] [diff] [review] patch 9: handle starting and animation of CSS transitions (the main patch) Please do condition the three DeleteProperty calls in nsGenericElement on HasFlags(NODE_HAS_PROPERTIES)? I'd think most don't. Given this new nsGenericElement code, can we end up in nsTransitionManager::WillRefresh with a node which is not in the right document? I would think no; can you at least add an NS_ERROR to that clause, if not turn it into an assert outright? r=bzbarsky with the above > But why in the world does it use the name "Iterator" for enumerators? Because I suck at naming, apparently. ;) We should change the poking optimization to clone the data first, if that would help.

Landed the main patch: http://hg.mozilla.org/mozilla-central/rev/ff00a422b6f1 Still need to file a bunch of followup bugs based on the comments above and on the FIXMEs in the patch. Also, please note that only a small set of properties are currently animatable (see the list in layout/style/test/test_transitions_per_property.html). Adding support for more properties to nsStyleAnimation will happen in other bugs.

Also landed http://hg.mozilla.org/mozilla-central/rev/d8c914dbd3f8 which backs out patch 6 and patch 7, which are no longer needed after bug 520396.

It looks like test_transitions.html might is failing intermittently: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254996299.1254998758.2575.gz The failures are all things like: 79936 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | delay test for delay -2s: computed value 90.15px should be between 90.219791px and 93.602247px at time between 1.034s and 1.121s. I think timing sensitive tests are sadly prone to failure on our unitest VMs.

Similar failures here: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255000779.1255002364.12383.gz

I increased the tolerance of the test to try to fix that: http://hg.mozilla.org/mozilla-central/rev/2fe394df6031

Test failed again: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255047880.1255049630.31382.gz Needs another tolerance-boost?

I think higher tolerance is the wrong solution; I think I'll: * reduce the tolerance * add a control test and then if the control test isn't in the expected range, do a single todo and skip the rest of the tests for that cycle. If more than X (maybe ?) of the control test checks fail, then do an actual fail. Not right now, though; I think the failures are pretty infrequent at this point so it can wait until tomorrow.

Pushed a patch with a new approach to doing the testing: http://hg.mozilla.org/mozilla-central/rev/347f91d4b48d It uses two reference transitions (started first and last), and has very high tolerance for them, but stricter-than-before tolerance for everything else matching those two.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255163760.1255165686.25451.gz OS X 10.5.2 mozilla-central test opt mochitests on 2009/10/10 01:36:00 80725 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | timing function test for timing function cubic-bezier(1, 0, 0, 1): computed value 41px should be between 41.002945px and 41.062945px at time between 3.993064s and 3.993064s.

Landed http://hg.mozilla.org/mozilla-central/rev/0f867561d021 , which should fix that.

And had to fix another error to fix the orange resulting from that: http://hg.mozilla.org/mozilla-central/rev/8862815409ab

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255232021.1255234561.3322.gz test_transitions.html | early reference: computed value 241.5px should be between 251.750000px and 319.375000px at time between 2.014s and 2.555s.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255378023.1255381037.12728.gz 80894 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | early reference: computed value 1000px should be between 1061.375000px and 1128.875000px at time between 8.491s and 9.031s. 80895 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | late reference: computed value 1000px should be between 1061.375000px and 1128.875000px at time between 8.491s and 9.031s.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255384853.1255387380.21213.gz 80794 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | timing function test for timing function cubic-bezier(1, 0, 0, 1): computed value 54.7333px should be between 54.746771px and 54.886771px at time between 4.0010639999999995s and 4.0010639999999995s.

Fixed some FIXME comments in: http://hg.mozilla.org/mozilla-central/rev/92468ffca422

Landed http://hg.mozilla.org/mozilla-central/rev/5014f994e84e to increase test tolerance, which should fix the errors in comment 103, comment 104, and comment 105.

I filed bug 522563 on making ReResolveStyleContext (and thus StyleContextChanged) handle continuations more efficiently. We actually already pass -Wno-invalid-offsetof, so I don't need to file that followup bug; it was added in bug 450194. I think I'm not going to bother filing a followup to rename nsTObserverArray's Iterator to Enumerator; the terminology seems to be used inconsistently between different languages (and people). I filed bug 522595 on the style data (non-)immutability issues. I filed bug 522597 on ElementData() sometimes being null in MapRuleInfoInto. I filed bug 522599 on the FIXME comments in the patches requesting additional tests. I added these bug numbers to the FIXME comments in the source code in: http://hg.mozilla.org/mozilla-central/rev/f1c05672332b Making additional properties animatable will occur in other bugs, which will generally have "nsStyleAnimation" in their summaries, so I'm now marking this as FIXED.

"dev-doc-needed" keyword, anyone? https://wiki.mozilla.org/CSS_Transitions is probably not enough / not on MDC anyway

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255769201.1255771723.17886.gz Linux mozilla-central test mochitests on 2009/10/17 01:46:41 80523 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_transitions.html | timing function test for timing function cubic-bezier(1, 0, 0, 1): computed value 45.2667px should be between 45.294170px and 45.474170px at time between 3.9990639999999997s and 3.9990639999999997s. should we start a bug about failures in test_transitions.html rather than continuing posting here? i suspect they will continue for some time.

oh well sounds like someone did already: bug 522862

See this also: https://bugzilla.mozilla.org/show_bug.cgi?id=526784

We have docs now: https://developer.mozilla.org/en/CSS/CSS_transitions