David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Description • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 1 • 10 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 2 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 3 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 4 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 5 • 9 years ago

... which is bug 1125455.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 6 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 7 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 8 • 9 years ago

This is used in patch 2.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 9 • 9 years ago

This is used in patch 3.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 10 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 11 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 12 • 9 years ago

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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 18 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 19 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 21 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 23 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 24 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 27 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 30 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 31 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 32 • 9 years ago

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"?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 33 • 9 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 34 • 9 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 35 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 36 • 9 years ago

(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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 37 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 38 • 9 years ago

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;

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 39 • 9 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 40 • 9 years ago

(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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 41 • 9 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 42 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 43 • 9 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 44 • 9 years ago

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"

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 45 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 46 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 47 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 48 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 49 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 50 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 51 • 9 years ago

(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 .

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 52 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 53 • 9 years ago

(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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 54 • 9 years ago

(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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 55 • 9 years ago

Good.

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 56 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 9 years ago

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

	Florian Bender
	Comment 58 • 9 years ago

Is this anyhow relnote worthy? I'd say yes, however it may be too technical … 

How about "Updated CSS3 transitions to match the specification"?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 59 • 9 years ago

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."