We need to rewrite the interaction of CSS transitions and animations with each other, and the way CSS transitions start, to match recent spec changes whose goal is to define an interoperable (and understandable) behavior. Current implementations do not agree, and by in large don't make sense. (Though the Chromium changes to implement the new spec text may have landed by now.) Links with more details: http://lists.w3.org/Archives/Public/www-style/2013Mar/0297.html - my initial spec proposal https://groups.google.com/d/msg/mozilla.dev.tech.layout/Mu0z1kqTUR0/S5iQj-qGeJQJ - a description of what the above means for Gecko http://lists.w3.org/Archives/Public/www-style/2013Jun/0682.html - CSS WG resolution to accept my proposal with modified version of (B) http://lists.w3.org/Archives/Public/www-style/2013Nov/0192.html - description of completed spec edits This bug will have two big but inter-related pieces (quoting from the scond link above): (1) Removing the concepts of "style with animation" and "style without animation" that we have, which I think has proven not to meet the goals I originally intended them to meet (providing sane behavior for requirements (3) and (5) in the first link). This would be replaced with what I propose in item (A) in the first link, which is essentially the miniflush concept that we have implemented for suppressing the main thread style updates for animations that are running on the compositor, but which we don't currently use beyond that code. I think this removal will end up being a pretty substantial code simplification, and the performance cost of the miniflush will be roughly the same as the performance cost of the current dual-style setup. (2) Changing how animations and transitions work in the cascade. Note that currently compositor-driven animations have different cascading behavior than animations not running on the compositor, which is a relatively serious bug 847287. I admit I don't have a completely concrete idea about how to do this, but I think it's not going to be all that hard; at a concrete level it might come down to having nsIStyleRules in the cascade at two different places (both at the top of the cascade and at the official location) in order to let the rule safely record which properties the animation or transition is the winning rule for, and then recording that information so that we can use it for the compositor-driven animations. I don't think this is too hard, though; I just haven't implemented it yet.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #0) > (1) Removing the concepts of "style with animation" and "style > without animation" that we have, which I think has proven not to > meet the goals I originally intended them to meet (providing sane > behavior for requirements (3) and (5) in the first link). Actually, on second thought, I might not quite remove all of them. I might preserve the ability for the RestyleTracker to track two different types of restyles (and the associated content bits), and then use the animation bits for doing the short-term coalescing needed to do the mini-flush (updating of only the animation style and no other style), since the mini-flush's coalescing will now be used more and its performance might matter more. Then again, maybe the existing coalescing it does is good enough (though I'd at least want to fix that it coalesces all transforms and coalesces all animations, but doesn't coalesce animations and transforms together -- this would also be a net reduction i code, I think). It's a little bit of overkill since we don't have any restyle roots (and, given that, it would have been nice to use the RootBit() as a has-data-in-table bit), but it might be the best answer in terms of code size and complexity.

On March 10 in https://groups.google.com/d/msg/mozilla.dev.tech.layout/0Ehe1BO5ixw/78F_kCoG3TYJ I wrote a description of the work in this bug: ====== I recently started on a large patch queue for https://bugzilla.mozilla.org/show_bug.cgi?id=960465 (rewrite CSS transition starting and transitions/animations interaction) which will contain some related changes: 1. refactor the current miniflush code (code to update the animation styles to the current refresh driver time while leaving style data otherwise out-of-date, so that we can do correct style comparisons when deciding whether to start CSS transitions when we've suppressed style updates on the main thread while running animations on the compositor) to do coalescing using a RestyleTracker rather than their current ad-hoc tree-walking and coalescing code (which will also, I believe, fix some correctness bugs). This also means introducing the miniflush's ability to replace certain special style rules without rerunning selector matching into the RestyleTracker, which is useful in part (4). 2. redesign the way we start CSS transitions to use the model described in the current spec, which involves replacing the code whose goal is to avoid starting transitions as a result of animations. The current code for doing this maintains without-animation and with-animation styles as separate concepts (but never stores both at the same time), so that style computation involves a pass of computing without-animation styles, and then a pass replacing them with with-animation styles; this allowed starting of CSS transitions to be computed based on styles that did not include animations. The new code will remove this mechanism, and instead use the miniflush mechanism, which updates currently-running animation style to the present refresh driver time without updating other styles, so that style comparisons used for starting CSS transitions are done on style including animation data, but always comparing animation data associated with the same time. 3. Change the way CSS transitions and animations interact with each other in the CSS cascade. 4. Do a restyling performance optimization that we'd planned to do a while ago, but then forgot about. This is covered by https://bugzilla.mozilla.org/show_bug.cgi?id=977991 and could land separately, but should be trivial to do on top of part (1). This will prevent us from rerunning *any* selector matching for style attribute changes. (We currently rerun selector matching on the element whose style attribute changed, but not on its descendants.) At this point I've got part (1) and parts of part (2) written, although not particularly well-tested. Unfortunately I don't think I'll be able to land (1) without (2), unless I temporarily change the nsINode flags from 32-bits to 64-bits, since (1) introduces a new type of RestyleTracker (each of which requires two node bits) and (2) removes an existing type, returning us to 2 types total. It *might* be possible to land (2) and (3) separately, but they might be too interrelated in terms of getting Web-compatible behavior. So I'm currently expecting that parts 1-3 will end up having to land at the same time, though I might find a way to avoid that. I'm hoping this will all (parts 1-4) land sometime in the Firefox 31 cycle, and hopefully early in the cycle. ====== I've decided to move part (1) above into bug 996796 so it can land sooner.

https://hg.mozilla.org/try/pushloghtml?changeset=77877bc63682 https://treeherder.mozilla.org/#/jobs?repo=try&revision=77877bc63682 https://tbpl.mozilla.org/?tree=Try&rev=77877bc63682

This bug now covers only item (1) from comment 0 / item (2) from comment 2. I'll file a separate bug for item (2) from comment 0 / item (3) from comment 2.

... which is bug 1125455.

https://hg.mozilla.org/try/pushloghtml?changeset=731dc05a2313 https://treeherder.mozilla.org/#/jobs?repo=try&revision=731dc05a2313 https://tbpl.mozilla.org/?tree=Try&rev=731dc05a2313

https://hg.mozilla.org/try/pushloghtml?changeset=f6cb672e4e68 https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6cb672e4e68 https://tbpl.mozilla.org/?tree=Try&rev=f6cb672e4e68

This is used in patch 2.

This is used in patch 3.

This is needed to make the tests for bug 686656 in test_animations.html pass. Note that once the rest of bug 960465 happens this will start producing slightly different results in edge cases, since we will only be skipping animation styles for the element itself and not for ancestors. However, both old and new behaviors are incorrect, since per spec we should be updating the base values dynamically. FIXME: File a bug on this.

This depends on bug 1087536 patch 3, which posts animation restyles using the eRestyle_CSSTransitions and eRestyle_CSSAnimations hints. This is used by patch 6.

This is needed to prevent these reftests from failing: layout/reftests/svg/smil/smil-transitions-interaction-1a.svg layout/reftests/svg/smil/smil-transitions-interaction-1b.svg layout/reftests/svg/smil/smil-transitions-interaction-2a.svg layout/reftests/svg/smil/smil-transitions-interaction-2b.svg layout/reftests/svg/smil/smil-transitions-interaction-4a.svg layout/reftests/svg/smil/smil-transitions-interaction-4b.svg The mIsCSS path fixes the a tests, and the !mIsCSS path fixes the b tests. This is because this patch series changes the way in which transitions interact with other types of animations to depend on those animations being flushed in the animation-only style flush. (The relevant call is added in patch 6, though we don't really depend on it until patch 17.)

This will be needed when (in later patches) we stop separating animation phases.

This switches to the new rules for how transitions start on descendants when also starting on ancestors.

Note that this increases memory use for completed transitions since we don't throw away the data when the transitions complete. That said, this matches what we do for CSS Animations, and it's needed (once we switch to the new rules for starting transitions) to maintain the invariant that unrelated style changes don't trigger transitions. The storage issues could be optimized in the future if it turns out to be a problem, but I think that's unlikely, given that we'll never store more than one for any element+property combination.

This is needed for patch 17, which removes restyling phases, so that when the transition manager posts a restyle to undo the covering done by the cover rule, that restyle doesn't get consumed by an inner frame.

Note that this means that when we start transitions, we post restyles that are processed during the current restyling operation, rather than in a later phase. This depends on patch 11, which makes the transition manager skip style changes that it posts while starting transitions, to ensure that this doesn't lead to an infinite loop. This also depends on patch 16, which only consumes restyle data for the primary frame, to ensure that the animation restyles posted are processed properly. It also depends on patch 14, which makes us retain data on finished transitions, to avoid triggering extra transitions on descendants when both an ancestor and a descendant transition an inherited property, and the descendant does so faster. This fixes a known failure in layout/style/test/test_animations.html and test_animations_omta.html (as visible in the patch). I haven't investigated why. FIXME: Do so, and at least comment in bug 978833. FIXME: Look for more code (e.g., function arguments, things to compute them) that can be removed.

I confirmed that we're actually using this codepath by manually testing <input type=color>: it works with the patch, but if I comment out the call to nsHTMLCSSStyleSheet::PseudoElementRulesMatching from nsStyleSet::RuleNodeWithReplacement, then the color swatch breaks, which proves that we're depending on the code. I think I included this in the queue because it is needed for patch 22, although I've forgotten the full reasoning.

The try run makes it look like this made the previously-intermittent Gij2 orange in bug 1106777 (the failures recorded for most of the bug, but not the beginning) reliable; I need to investigate that.

Comment on attachment 8561614 [details] [diff] [review] patch 1 - Add parameter to skip animations work to ResolveStyleWithReplacement Review of attachment 8561614 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.h @@ +133,5 @@ > // Resolve style by making replacements in the list of style rules as > // described by aReplacements, but otherwise maintaining the status > // quo. > + enum { > + // flags for paramete below Comment needs to be written.

Comment on attachment 8561615 [details] [diff] [review] patch 2 - Add method to return a modified version of a style context, with all or part of the animation data removed Review of attachment 8561615 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.h @@ +145,5 @@ > uint32_t aFlags = 0); > > + // Resolve style by returning a style context with the animation data > + // removed. It is allowable to remove all animation data with > + // eRestyle_ChangeAnimationPhase, or by using any othe hints that "other hints" (Although I think you touch this line in a later patch so you might want to fix it there instead.) Also, this comment is somewhat confusing. The first sentence suggests all animation data is removed automatically whilst the second sentence suggests the user is responsible for removing the animation data. Perhaps it should be "... with the specified animation data removed"?

Comment on attachment 8561616 [details] [diff] [review] patch 3 - Use style without animation for base values for CSS animations (refixes bug 686656 in the new architecture) ># HG changeset patch ># User L. David Baron <dbaron@dbaron.org> ># Date 1423514898 -39600 ># Tue Feb 10 07:48:18 2015 +1100 ># Node ID 4d37a744ad00a5f3dd7ebce32a34afd985431ae8 ># Parent 1d835dfe1f05c95b0ecbf937db6e42d02e951eaa >Bug 960465 patch 3 - Use style without animation for base values for CSS animations (refixes bug 686656 in the new architecture). > >This is needed to make the tests for bug 686656 in test_animations.html >pass. > >Note that once the rest of bug 960465 happens this will start producing >slightly different results in edge cases, since we will only be skipping >animation styles for the element itself and not for ancestors. However, >both old and new behaviors are incorrect, since per spec we should be >updating the base values dynamically. > >FIXME: File a bug on this. Did you file a bug on this in the end?

Comment on attachment 8561617 [details] [diff] [review] patch 4 - Track whether there are any pending non-animation restyles >@@ -1760,16 +1763,21 @@ RestyleManager::PostRestyleEventCommon(E > // Nothing to do here > return; > } > > RestyleTracker& tracker = > aForAnimation ? mPendingAnimationRestyles : mPendingRestyles; > tracker.AddPendingRestyle(aElement, aRestyleHint, aMinChangeHint); > >+ if (aRestyleHint & ~(eRestyle_CSSTransitions | eRestyle_CSSAnimations | >+ eRestyle_ChangeAnimationPhase)) { >+ mHavePendingNonAnimationRestyles = true; >+ } At first I thought this list should include eRestyle_SVGAttrAnimations and eRestyle_StyleAttribute here but I notice we always set eRestyle_ChangeAnimationPhase in combination with those. Later in this patch series, however, we drop eRestyle_ChangeAnimationPhase. I think I haven't understood how this is used but perhaps it's just because I haven't gone through all the patches yet. At this point in the patch series, Element::SetSMILOverrideStyleRule, for example, will call RestyleForAnimation -> PostAnimationRestyleEvent -> PostRestyleEventCommon with hint eRestyle_StyleAttribute | eRestyle_ChangeAnimationPhase, which will mean mHavePendingNonAnimationRestyles gets set to true. However, by the end of this patch series, Element::SetSMILOverrideStyleRule will call RestyleForAnimation -> PostRestyleEvent with hint eRestyle_StyleAttribute, which will mean mHavePendingNonAnimationRestyles does not get set to true. Is that the intended outcome? r=me assuming that is the intended outcome but I'd still like to understand this better

Comment on attachment 8561618 [details] [diff] [review] patch 5 - Make SMIL animations participate in the animation-only style flush >@@ -811,16 +812,46 @@ nsSMILAnimationController::GetTargetIden ... >+ // REVIEW: I'm assuming that mIsCSS means that the rules are the ones >+ // returned from Element::GetSMILOverrideStyleRule, and otherwise >+ // they're the rules returned from >+ // nsSVGElement::GetAnimatedContentStyleRule. >+ nsRestyleHint rshint = key.mIsCSS ? eRestyle_StyleAttribute >+ : eRestyle_SVGAttrAnimations; Yes, that seems to be right. In nsSMILCompositor::CreateSMILAttr() we switch on that value. If mIsCSS is true, we create a new nsSMILCSSProperty object which uses GetSMILOverrideStyleRule http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp?rev=20729b28eb1e#90 http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp?rev=20729b28eb1e#161 If mIsCSS is false, we create a new nsSMILMappedAttribute object (in nsSVGElement::GetAnimatedAttr) which applies animation values by calling nsINode::SetProperty() with category SMIL_MAPPED_ATTR_ANIMVAL. These get added to the animated content style rule in nsSVGElement::UpdateAnimatedContentStyleRule: http://mxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGElement.cpp#1326 One thing I'm not sure about, however, is why nsSMILMappedAttribute::FlushChangesToTargetAttr() calls RestyleForAnimation *without* passing eRestyle_SVGAttrAnimations. http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILMappedAttribute.cpp?rev=32f48d6d3389#140 Do you know if this ought to be passing that flag here? It seems like some of the places it is called we should have been passing eRestyle_ChangeAnimationPhase? > void > RestyleManager::UpdateOnlyAnimationStyles() > ... >+ bool doSMIL = false; >+ nsIDocument* document = mPresContext->Document(); >+ nsSMILAnimationController *animationController = nullptr; nsSMILAnimationController* animationController = nullptr; The rest looks fine, but I'm curious to hear what you think about nsSMILMappedAttribute::FlushChangesToTargetAttr.

(In reply to Brian Birtles (:birtles, away 5~6 Feb) from comment #34) > I think I haven't understood how this is used but perhaps it's just because > I haven't gone through all the patches yet. At this point in the patch > series, Element::SetSMILOverrideStyleRule, for example, will call > RestyleForAnimation -> PostAnimationRestyleEvent -> PostRestyleEventCommon > with hint eRestyle_StyleAttribute | eRestyle_ChangeAnimationPhase, which > will mean mHavePendingNonAnimationRestyles gets set to true. > > However, by the end of this patch series, Element::SetSMILOverrideStyleRule > will call RestyleForAnimation -> PostRestyleEvent with hint > eRestyle_StyleAttribute, which will mean mHavePendingNonAnimationRestyles > does not get set to true. Sorry, that's wrong. In both cases we'll set mHavePendingNonAnimationRestyles to true.

Comment on attachment 8561620 [details] [diff] [review] patch 7 - Use SetInAnimationOnlyStyleUpdate for ProcessPendingRestyles runs that are only updating animation data >+ // If we don't have non-animation style updates, then we have queued >+ // up animation style updates from the refresh driver tick. This >+ // doesn't necessarily include *all* animation style updates, since >+ // we might be suppressing main-thread updates for some animations, >+ // so we don't want to call UpdateOnlyAnimationStyles, which updates >+ // all animations. >+ // >+ // So we want to act here as though we're in an animation-only style >+ // update and suppress changes to transitions, since we're really >+ // doing *part* of UpdateOnlyAnimationStyles. I'm not sure if the connection between animations suppressed because they are running on the compositor and suppressing changes to transitions is obvious. This comment could possibly be more clear but I'm not quite sure how.

Comment on attachment 8561618 [details] [diff] [review] patch 5 - Make SMIL animations participate in the animation-only style flush > Do you know if this ought to be passing that flag here? It seems like some > of the places it is called we should have been passing > eRestyle_ChangeAnimationPhase? As discussed with dbaron, failing to pass this flag might mean the code is suboptimal but it shouldn't be incorrect since eRestyle_ChangeAnimationPhase is a subset of eRestyle_Self. r=birtles but we still need to fix the placement of the '*' in: nsSMILAnimationController *animationController = nullptr;

Comment on attachment 8561628 [details] [diff] [review] patch 14 - Retain finished transitions until the next style change or until removed from transition-property > // Stop any transitions for properties that are no longer in >- // 'transition-property'. >- // Also stop any transitions for properties that just changed (and are >- // still in the set of properties to transition), but we didn't just >- // start the transition because delay and duration are both zero. >+ // 'transition-property', including finished transitions. >+ // Also stop any transitions (and removed any finished transitions) >+ // for properties that just changed (and are still in the set of >+ // properties to transition), but we didn't just start the transition >+ // because delay and duration are both zero. I don't understand this comment. Does stop here refer to cancelling? If so, do we actually stop finished transitions? It seems like we don't call Cancel() when IsFinishedTransition() is true. > if ((checkProperties && > !allTransitionProperties.HasProperty(prop.mProperty)) || > // properties whose computed values changed but delay and > // duration are both zero > !ExtractComputedValueForTransition(prop.mProperty, afterChangeStyle, > currentValue) || > currentValue != segment.mToValue) { > // stop the transition >- player->Cancel(); >+ if (!player->GetSource()->IsFinishedTransition()) { >+ player->Cancel(); >+ collection->UpdateAnimationGeneration(mPresContext); >+ } > players.RemoveElementAt(i); As discussed, the comment here is misleading. We don't actually check for a zero delay and duration here. However, we *do* check this when triggering new transitions and the spec also says we should ignore changes to delay and duration once a transition has started. As a result, we shouldn't need to check for a zero duration and delay for *existing* transitions. A further concern is that we might fail to cancel an existing transition if the duration and delay were updated to zero that we might fail to cancel the existing transition. This is specified in step 3.2 of the algorithm for starting transitions: > Otherwise, if the combined duration is less than or equal to 0s, then implementations must cancel the running transition. > http://dev.w3.org/csswg/css-transitions/#starting A test case shows we are doing the right thing in this case both before and after these patches are applied. http://jsfiddle.net/ajkabj7e/ However, I also tested the "combined duration" definition: > the sum of max(matching transition duration, 0s) and the matching transition delay In this case it seems like we're not cancelling the transition correctly. http://jsfiddle.net/ajkabj7e/2/ This is true both before and after these patches so I'll file a separate bug for this. For now, we should update the comment about zero duration and delay and the other comment described above. r=birtles with that

(In reply to Brian Birtles (:birtles, away 5~6 Feb) from comment #39) > This is true both before and after these patches so I'll file a separate bug > for this. Filed bug 1133375.

Comment on attachment 8561626 [details] [diff] [review] patch 12 - Instead of using the full style covered by the cover rule as the parent style for descendents when we've tried starting transitions, use the after-change style Review of attachment 8561626 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed. ::: layout/style/nsTransitionManager.cpp @@ +390,5 @@ > + if (collection) { > + // Since we have transition styles, we have to undo this replacement. > + // The check of mAnimationGeneration above will ensure that we don't > + // go through the rest of this function again when we do. > + collection->PostRestyleForAnimation(mPresContext); "The comparison of mCheckGeneration against the animation generation above..." would be more clear here since we don't actually refer to mAnimationGeneration anywhere in this file. ::: layout/style/test/test_transitions.html @@ +681,5 @@ > } > > + // Override the parent's transition with the child's as long > + // as the child transition is still running. > + if (property != "letter-spacing" && duration + delay > time) { Should we also be testing the behavior after the child transition has finished (and when a new style change occurs)? Or is that tested elsewhere?

Comment on attachment 8561631 [details] [diff] [review] patch 17 - Remove separate animation and non-animation phases of restyling Review of attachment 8561631 [details] [diff] [review]: ----------------------------------------------------------------- From commit message: >This fixes a known failure in layout/style/test/test_animations.html and >test_animations_omta.html (as visible in the patch). I haven't >investigated why. FIXME: Do so, and at least comment in bug 978833. Did you work out what was going on here? >FIXME: Look for more code (e.g., function arguments, things to compute >them) that can be removed. Did you still want to do this? There's a fair bit of simplification in this patch already. r=birtles but I'd like to know why nsStyleSet::ReparentStyleContext doesn't need to handle SkipAnimationRules(). ::: layout/style/nsStyleSet.cpp @@ +2161,5 @@ > nsCSSPseudoElements::Type pseudoType = aStyleContext->GetPseudoType(); > nsRuleNode* ruleNode = aStyleContext->RuleNode(); > > + NS_ASSERTION(!PresContext()->RestyleManager()->SkipAnimationRules(), > + "we no longer handle SkipAnimationRules()"); I don't understand why this is ok.

Comment on attachment 8561634 [details] [diff] [review] patch 20 - Allow pseudo-elements for style attribute replacement in RuleNodeWithReplacement Review of attachment 8561634 [details] [diff] [review]: ----------------------------------------------------------------- r=birtles with comments addressed ::: layout/base/RestyleManager.cpp @@ +2061,5 @@ > } > > +static dom::Element* > +PseudoElementForStyleContext(nsIFrame* aFrame, > + nsCSSPseudoElements::Type aPseudoType) As discussed, this method returns the content node for the pseudo-element, not its corresponding real element. That would probably be useful to document somewhere. @@ +3081,5 @@ > // ReparentStyleContext that rebuilds the path in the rule tree > // rather than reusing the rule node, as we need to do during a > // rule tree reconstruct. > + Element* pseudoElement = PseudoElementForStyleContext(aSelf, pseudoType); > + MOZ_ASSERT(!element || element != pseudoElement); This assertion could have a description. As I understand it, we're asserting here that we got back the content node for the pseudo element, and not its real element. @@ +3332,5 @@ > // rather than reusing the rule node, as we need to do during a > // rule tree reconstruct. > + Element* pseudoElement = > + PseudoElementForStyleContext(aSelf, extraPseudoType); > + MOZ_ASSERT(!element || element != pseudoElement); This too could do with a description. ::: layout/style/nsStyleSet.h @@ +138,5 @@ > eSkipStartingAnimations = (1<<0), > }; > already_AddRefed<nsStyleContext> > ResolveStyleWithReplacement(mozilla::dom::Element* aElement, > + mozilla::dom::Element* aPseudoElement, Here and below (RuleNodeWithReplacement) we should probably document these parameters; or at least call out which can be null.

Comment on attachment 8561635 [details] [diff] [review] patch 21 - Replace one use of nsRestyleHint_ChangeAnimationPhase with nsRestyleHint_AllHintsWithAnimations so that we can remove the rest Review of attachment 8561635 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleSet.h @@ +151,1 @@ > // are allowed by ResolveStyleWithReplacement. "any other hints"

It seems like the reliable Gij failure went away: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfec41c8f71f

(In reply to Brian Birtles (:birtles) from comment #33) > Comment on attachment 8561616 [details] [diff] [review] > patch 3 - Use style without animation for base values for CSS animations > (refixes bug 686656 in the new architecture) ... > >Note that once the rest of bug 960465 happens this will start producing > >slightly different results in edge cases, since we will only be skipping > >animation styles for the element itself and not for ancestors. However, > >both old and new behaviors are incorrect, since per spec we should be > >updating the base values dynamically. > > > >FIXME: File a bug on this. > > Did you file a bug on this in the end? Bug 1064915. (In reply to Brian Birtles (:birtles) from comment #34) > Comment on attachment 8561617 [details] [diff] [review] > patch 4 - Track whether there are any pending non-animation restyles > > >@@ -1760,16 +1763,21 @@ RestyleManager::PostRestyleEventCommon(E > > // Nothing to do here > > return; > > } > > > > RestyleTracker& tracker = > > aForAnimation ? mPendingAnimationRestyles : mPendingRestyles; > > tracker.AddPendingRestyle(aElement, aRestyleHint, aMinChangeHint); > > > >+ if (aRestyleHint & ~(eRestyle_CSSTransitions | eRestyle_CSSAnimations | > >+ eRestyle_ChangeAnimationPhase)) { > >+ mHavePendingNonAnimationRestyles = true; > >+ } > > At first I thought this list should include eRestyle_SVGAttrAnimations and > eRestyle_StyleAttribute here but I notice we always set > eRestyle_ChangeAnimationPhase in combination with those. Later in this patch > series, however, we drop eRestyle_ChangeAnimationPhase. > > I think I haven't understood how this is used but perhaps it's just because > I haven't gone through all the patches yet. At this point in the patch > series, Element::SetSMILOverrideStyleRule, for example, will call > RestyleForAnimation -> PostAnimationRestyleEvent -> PostRestyleEventCommon > with hint eRestyle_StyleAttribute | eRestyle_ChangeAnimationPhase, which > will mean mHavePendingNonAnimationRestyles gets set to true. > > However, by the end of this patch series, Element::SetSMILOverrideStyleRule > will call RestyleForAnimation -> PostRestyleEvent with hint > eRestyle_StyleAttribute, which will mean mHavePendingNonAnimationRestyles > does not get set to true. > > Is that the intended outcome? > > r=me assuming that is the intended outcome but I'd still like to understand > this better So I'd like to have fully separate cascade levels for things that come from animations and things that don't, but we're not quite there yet. eRestyle_SVGAttrAnimations is only animations, so I should in fact add it to the list there. The problem is that eRestyle_StyleAttribute contains both animation and non-animation styles (and most of the time it will be non-animation, except for the one SMIL case). So I have to set mHavePendingNonAnimationRestyles to true in that case. Erroneously being true just means being slower; erroneously setting false would lead to incorrect behavior. So I'll include SVGAttrAnimations (which I believe I added well after originally writing this patch), and add a comment explaining the previous paragraph.

... and also file bug 1133439 on improving it.

(In reply to Brian Birtles (:birtles) from comment #35) > One thing I'm not sure about, however, is why > nsSMILMappedAttribute::FlushChangesToTargetAttr() calls RestyleForAnimation > *without* passing eRestyle_SVGAttrAnimations. As we discussed in person, eRestyle_Self does a superset of the work done by these more specific hints, so that's fine, just less optimized. I didn't want to try to optimize it further since I didn't fully understand what was going on.

(In reply to Brian Birtles (:birtles) from comment #37) > Comment on attachment 8561620 [details] [diff] [review] > patch 7 - Use SetInAnimationOnlyStyleUpdate for ProcessPendingRestyles runs > that are only updating animation data > > >+ // If we don't have non-animation style updates, then we have queued > >+ // up animation style updates from the refresh driver tick. This > >+ // doesn't necessarily include *all* animation style updates, since > >+ // we might be suppressing main-thread updates for some animations, > >+ // so we don't want to call UpdateOnlyAnimationStyles, which updates > >+ // all animations. > >+ // > >+ // So we want to act here as though we're in an animation-only style > >+ // update and suppress changes to transitions, since we're really > >+ // doing *part* of UpdateOnlyAnimationStyles. > > I'm not sure if the connection between animations suppressed because they > are running on the compositor and suppressing changes to transitions is > obvious. This comment could possibly be more clear but I'm not quite sure > how. I rewrote it to be: + // If we don't have non-animation style updates, then we have queued + // up animation style updates from the refresh driver tick. This + // doesn't necessarily include *all* animation style updates, since + // we might be suppressing main-thread updates for some animations, + // so we don't want to call UpdateOnlyAnimationStyles, which updates + // all animations. In other words, the work that we're about to do + // to process the pending restyles queue is a *subset* of the work + // that UpdateOnlyAnimationStyles would do, since we're *not* + // updating transitions that are running on the compositor thread + // and suppressed on the main thread. + // + // But when we update those styles, we want to suppress updates to + // transitions just like we do in UpdateOnlyAnimationStyles. So we + // want to tell the transition manager to act as though we're in + // UpdateOnlyAnimationStyles.

(In reply to Brian Birtles (:birtles) from comment #39) > > // Stop any transitions for properties that are no longer in > >- // 'transition-property'. > >- // Also stop any transitions for properties that just changed (and are > >- // still in the set of properties to transition), but we didn't just > >- // start the transition because delay and duration are both zero. > >+ // 'transition-property', including finished transitions. > >+ // Also stop any transitions (and removed any finished transitions) > >+ // for properties that just changed (and are still in the set of > >+ // properties to transition), but we didn't just start the transition > >+ // because delay and duration are both zero. > > I don't understand this comment. Does stop here refer to cancelling? If so, > do we actually stop finished transitions? We remove them from the list of transitions; I'm not really sure what it means to stop them given that they're already stopped. > It seems like we don't call > Cancel() when IsFinishedTransition() is true. Is there a reason to call Cancel() on them? The old codepath that removed them would normally have removed them without calling Cancel(). > As discussed, the comment here is misleading. We don't actually check for a > zero delay and duration here. However, we *do* check this when triggering > new transitions and the spec also says we should ignore changes to delay and > duration once a transition has started. As a result, we shouldn't need to > check for a zero duration and delay for *existing* transitions. > > A further concern is that we might fail to cancel an existing transition if > the duration and delay were updated to zero that we might fail to cancel the > existing transition. ... > For now, we should update the comment about zero duration and delay and the > other comment described above. I'll update these comments with FIXMEs pointing to bug 1133375.

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #50) > I'll update these comments with FIXMEs pointing to bug 1133375. Actually, I think the comments make sense as-is (though I can make them a bit clearer), and I'm not sure why bug 1133375 is present. They reference delay and duration both being zero since that's the main reason we'd have a running transition with a destination other than the current value (i.e., we wouldn't have replaced that transition with a new one). I updated the spec in https://hg.csswg.org/drafts/rev/0170ef0d0817 .

(In reply to Brian Birtles (:birtles) from comment #41) > > + // Override the parent's transition with the child's as long > > + // as the child transition is still running. > > + if (property != "letter-spacing" && duration + delay > time) { > > Should we also be testing the behavior after the child transition has > finished (and when a new style change occurs)? Or is that tested elsewhere? We do test the behavior after the child transition has finished, including with unrelated style changes, in this test (this adjustment is needed to make the test continue to pass). In this test we don't test the behavior when a new *related* style change occurs. However, test_transitions_per_property.html has large numbers of tests that cover that case (which is very common), so I think we're fine.

(In reply to Brian Birtles (:birtles) from comment #42) > >This fixes a known failure in layout/style/test/test_animations.html and > >test_animations_omta.html (as visible in the patch). I haven't > >investigated why. FIXME: Do so, and at least comment in bug 978833. > > Did you work out what was going on here? I think I did, and I just changed the message to: This fixes a known failure in layout/style/test/test_animations.html and test_animations_omta.html (as visible in the patch). I believe this is because this patch changes us to compute keyframe values for animations on top of a style context *with* animation data rather than one without, which means what we're computing them on top of changes each time. (The purpose of patch 3 was to avoid this in the case where avoiding it matters, i.e., implicit 0% and 100% keyframes.) > >FIXME: Look for more code (e.g., function arguments, things to compute > >them) that can be removed. > > Did you still want to do this? There's a fair bit of simplification in this > patch already. Probably not in this bug. I guess if there's more, hopefully I'll find it later. > r=birtles but I'd like to know why nsStyleSet::ReparentStyleContext doesn't > need to handle SkipAnimationRules(). > > ::: layout/style/nsStyleSet.cpp > @@ +2161,5 @@ > > nsCSSPseudoElements::Type pseudoType = aStyleContext->GetPseudoType(); > > nsRuleNode* ruleNode = aStyleContext->RuleNode(); > > > > + NS_ASSERTION(!PresContext()->RestyleManager()->SkipAnimationRules(), > > + "we no longer handle SkipAnimationRules()"); > > I don't understand why this is ok. SkipAnimationRules is no longer true for an entire phase of restyling; it's something that's only true for special requests to get a specific style context. When it was true for an entire phase, we'd fall into ReparentStyleContext in many cases when recomputing style for descendants; now there's no need for that code.

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #50) > (In reply to Brian Birtles (:birtles) from comment #39) > > > // Stop any transitions for properties that are no longer in > > >- // 'transition-property'. > > >- // Also stop any transitions for properties that just changed (and are > > >- // still in the set of properties to transition), but we didn't just > > >- // start the transition because delay and duration are both zero. > > >+ // 'transition-property', including finished transitions. > > >+ // Also stop any transitions (and removed any finished transitions) > > >+ // for properties that just changed (and are still in the set of > > >+ // properties to transition), but we didn't just start the transition > > >+ // because delay and duration are both zero. > > > > I don't understand this comment. Does stop here refer to cancelling? If so, > > do we actually stop finished transitions? > > We remove them from the list of transitions; I'm not really sure what it > means to stop them given that they're already stopped. I misread the comment. I thought it was saying we stopped finished transitions but it doesn't. > > It seems like we don't call > > Cancel() when IsFinishedTransition() is true. > > Is there a reason to call Cancel() on them? The old codepath that removed > them would normally have removed them without calling Cancel(). No. I think if they're finished, they don't need to be cancelled. My question was just about whether the code matched the comment, and it seems it does. (Whether or not we cancel is observable since script can now use the DOM API to get back running transitions and hold onto them. If the transition is cancelled its current time will reset to null.)

Good. I've improved the confusing comments in patch 14 (and had birtles read them over my shoulder).

https://hg.mozilla.org/integration/mozilla-inbound/rev/84e9b82dab4f https://hg.mozilla.org/integration/mozilla-inbound/rev/3faad716fa52 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5fa940bfd9c https://hg.mozilla.org/integration/mozilla-inbound/rev/b36e2a0e902f https://hg.mozilla.org/integration/mozilla-inbound/rev/0288ff191edf https://hg.mozilla.org/integration/mozilla-inbound/rev/7d16f2fd8329 https://hg.mozilla.org/integration/mozilla-inbound/rev/a92f3bc5ecd3 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a314fd17c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/76167c597eb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/92029ebe8f74 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd59c46d30c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/e65ef694c410 https://hg.mozilla.org/integration/mozilla-inbound/rev/3b33596002e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ee72589c18 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ab4e7bf0d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/c723fa961299 https://hg.mozilla.org/integration/mozilla-inbound/rev/29d623ce3ed2 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4225263cb35 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d39286ee4a https://hg.mozilla.org/integration/mozilla-inbound/rev/dbab5416b769 https://hg.mozilla.org/integration/mozilla-inbound/rev/5be0a9b53dc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9dd7c71cc68

https://hg.mozilla.org/mozilla-central/rev/84e9b82dab4f https://hg.mozilla.org/mozilla-central/rev/3faad716fa52 https://hg.mozilla.org/mozilla-central/rev/d5fa940bfd9c https://hg.mozilla.org/mozilla-central/rev/b36e2a0e902f https://hg.mozilla.org/mozilla-central/rev/0288ff191edf https://hg.mozilla.org/mozilla-central/rev/7d16f2fd8329 https://hg.mozilla.org/mozilla-central/rev/a92f3bc5ecd3 https://hg.mozilla.org/mozilla-central/rev/c3a314fd17c4 https://hg.mozilla.org/mozilla-central/rev/76167c597eb8 https://hg.mozilla.org/mozilla-central/rev/92029ebe8f74 https://hg.mozilla.org/mozilla-central/rev/dd59c46d30c4 https://hg.mozilla.org/mozilla-central/rev/e65ef694c410 https://hg.mozilla.org/mozilla-central/rev/3b33596002e3 https://hg.mozilla.org/mozilla-central/rev/b2ee72589c18 https://hg.mozilla.org/mozilla-central/rev/f0ab4e7bf0d9 https://hg.mozilla.org/mozilla-central/rev/c723fa961299 https://hg.mozilla.org/mozilla-central/rev/29d623ce3ed2 https://hg.mozilla.org/mozilla-central/rev/a4225263cb35 https://hg.mozilla.org/mozilla-central/rev/c2d39286ee4a https://hg.mozilla.org/mozilla-central/rev/dbab5416b769 https://hg.mozilla.org/mozilla-central/rev/5be0a9b53dc2 https://hg.mozilla.org/mozilla-central/rev/a9dd7c71cc68

Is this anyhow relnote worthy? I'd say yes, however it may be too technical … How about "Updated CSS3 transitions to match the specification"?

It should be in "Firefox 38 for developers", which is a well-curated list of changes affecting developers, but it should not be in the release notes, since a random selection of developer content that people happen to nominate shouldn't be in the release notes, and I'm not aware of any organized effort to choose which developer content belongs in the release notes beyond honoring the nominations that happen to be made. For "Firefox 38 for developers", I'd suggest summarizing it as "The way CSS transitions start has been rewritten to match the current spec. This affects the way they interact with other types of declarative animations and the way CSS transitions work when running simultaneously on ancestors and descendants."