Closed Bug 1315874 Opened 3 years ago Closed 2 years ago

SMIL animations trigger CSS transitions

Categories

(Core :: CSS Parsing and Computation, defect, P1)

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mantaroh, Assigned: birtles)

References

(Blocks 2 open bugs, )

Details

(Keywords: regression, Whiteboard: [stylo])

Attachments

(9 files, 11 obsolete files)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
Spec is:
'... except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time. ' [1]

[1] https://drafts.csswg.org/css-transitions-1/#before-change-style

So I think that we should not create new transition when changing style due to SMIL.
However, after release Firefox 41, we create new transition in above condition.

For example, following example, we expect blue box.
----------------------------------------------------------
<svg>
 <style>
 rect#rec {
   fill: red;
   transition: fill 1s;
 }
 </style>
 <rect width="50" height="50" id="rec">
   <animate attributeName="fill" to="blue" dur="1s" fill="freeze"/>
 </rect>
</svg>

(https://people-mozilla.org/~mantaroh/svg/transition_svg.svg)
----------------------------------------------------------

The result of each version is as follow:
Before Firefox 40	: Work correctly.
Firefox 46 ~ 48		: Repeat the color between red and blue.
After Firefox 49	: Display red box. (and create the new transition after SMIL is finished.)
Sorry I wrote wrong comment.
The result of above example is as follow:

Before bug 1171966 :Work correctly.
After  bug 1171966 : Repeat the color between red and blue.
After  bug 1209405 : Display red box.

Perhaps, I think this bug contain two problems.
1) Transition will start when working reflow of SMIL.
2) SMIL's fill mode doesn't work.

I tried revert change of bug 1171996, then I confirmed that it work correctly.
Brian, it seems bug 1171966 caused this. Can you take a quick look, please?
Flags: needinfo?(bbirtles)
Mantaroh will look into this in for the next release.
Assignee: nobody → mantaroh
Flags: needinfo?(bbirtles)
Priority: -- → P2
Currently, we will create CSS-Transition always due to restyle from SMIL.
So Perhaps we will need to ignore creating the CSS-Transition when restyling from SMIL.

Local patch will fix this problem, I'm going to investigate impact of this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f239f1ee6ead5de9dfc1c9c252f22530e8947ab
We can remove the step of creating the transition when restyling with SVG Animations.
Currently, we've run this step always when restyling with SVG Animations.
As result, many transitions are created when restyling with SVG.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fce41069250d1e043527a40a357fb59e7a64bb99
Hi Brian,

I think that this phenomenon is simillar to bug 1062106.
I wonder if you can help me understanding the this problem.

If we use attributeType="CSS", this phenomenon is occurred.
In this case, |mInAnimationOnlyStyleUpdate| is false when restyling. [1]
So transition will create when restyling of SVG Animation.

I think that we will need to drop creating transition when restyle hint is StyleAttribute_Animations.

I hope that you can point out where my above understanding is wrong.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/nsTransitionManager.cpp#420
Attachment #8817354 - Flags: feedback?(bbirtles)
Attachment #8816957 - Attachment is obsolete: true
Comment on attachment 8817354 [details] [diff] [review]
Ignore the step of creating the transition when restyling of SVG Animations.

Makes sense to me.

The test might need some work though:
* The duration is too short
* I'm not sure what getAnimations() is supposed to be doing there

I assume the test fails without the code changes in this patch?

dbaron should probably review this once you've tidied up the test.
Attachment #8817354 - Flags: feedback?(bbirtles) → feedback+
Too late to fix in 50.1.0 release
(In reply to Brian Birtles (:birtles, away 13-16 Dec) from comment #7)
> Comment on attachment 8817354 [details] [diff] [review]
> dbaron should probably review this once you've tidied up the test.

Thanks, Brian,
I've fixed previous patch.
I think that we can use mochitest instead reftests using by getAnimations function.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2452d82add1482cd0f5b86cfc67296811e5302ef

dbaron,
Transition will create due to SMIL restyling.
I think that this phenomenon occur calling Element::SetSMILOverrideStyleDeclaration. 

We will skip creating transition when restyling of animation only update[1]. However SMIL will update style with eResytle_StyleAttribute and eRestyle_StyleAttribute_Animations.[2]

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/nsTransitionManager.cpp#420
[2] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/Element.cpp#1987

This patch will ignore creating transition when style hint is eRestyle_StyleAttribute_Animations.
Could you please review this patch?
Looks like a pretty simple patch with a test included. Seems worth keeping on the radar for possible uplift to 51 if it lands soon.
Comment on attachment 8818779 [details]
Bug 1315874 part 1 - Add mochitest for SMIL with CSS-Transition.

https://reviewboard.mozilla.org/r/98718/#review103112

::: dom/smil/test/test_smilWithTransition.html:22
(Diff revision 1)
> +  // Animation doesn't start until onload
> +  SimpleTest.waitForExplicitFinish();
> +  window.addEventListener("DOMContentLoaded", runTests, false);

Seems like you should have a test assertion here that getComputedStyle(rect).fill is red, both (a) to flush style and (b) to ensure that there's actually the style change you expect.

::: dom/smil/test/test_smilWithTransition.html:31
(Diff revision 1)
> +  function runTests() {
> +    var rect = document.getElementById("rect");
> +    requestAnimationFrame(function(time) {
> +      var anim = document.getAnimations()[0];
> +      todo(!anim, "CSS-Transition shouldn't created by restyling of SMIL.");
> +      SimpleTest.finish();

And also maybe worth asserting here that the rect is green.

(Also, is this guaranteed to be after onload?  Maybe the event listener should be for load rather than DOMContentLoaded?)
Attachment #8818779 - Flags: review?(dbaron) → review+
Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review103116

::: layout/base/RestyleManager.cpp:2968
(Diff revision 1)
> +      if (!(aRestyleHint & eRestyle_StyleAttribute_Animations)) {
> +        changedStyle = RestyleManager::TryInitiatingTransition(mPresContext,

So this isn't really an appropriate fix, because multiple style changes can be coalesced together.  So if you want to fix this bug at this level, your fix will either incorrectly suppress transitions that it should not (as this patch does) or fail to suppress transitions that it should.

I previously addressed this sort of thing by having separate animation and non-animation phases of restyling.  Those were removed in bug 960465, in favor of an UpdateOnlyAnimationStyles mechanism.

Patch 5 in bug 960465 (along with other parts of the bug) should have prevented this bug from existing.  Is it not working correctly?
Attachment #8818780 - Flags: review?(dbaron) → review-
Thanks dbaron,

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #14)
> I previously addressed this sort of thing by having separate animation and
> non-animation phases of restyling. Those were removed in bug 960465, in
> favor of an UpdateOnlyAnimationStyles mechanism.
> 
> Patch 5 in bug 960465 (along with other parts of the bug) should have
> prevented this bug from existing. Is it not working correctly?

In Element::SetSMILOverrideStyleDeclaration, we will add pending restyle task to PendingTracker with eRestyle_StyleAttribute_Animations when compoing attribute. [1][2] 
Then, this restyle task will process immediately. [2][3]

[1] https://hg.mozilla.org/mozilla-central/annotate/2977ca1224525680cbfb5c3ce3018818b6dfd8f2/dom/base/Element.cpp#l1983
[2] https://dxr.mozilla.org/mozilla-central/rev/2977ca1224525680cbfb5c3ce3018818b6dfd8f2/dom/smil/nsSMILCompositor.cpp#84
[3] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/dom/smil/nsSMILCSSProperty.cpp#105

Currently, In UpdateOnlyAnimationStyles, we will check nsSMILAnimationController::mightHavePendingStyleUpdates.[4] This flag will updated from composed attribute result when last process of DoSample. [5]

[4] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/layout/base/RestyleManager.cpp#911
[5] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/dom/smil/nsSMILAnimationController.cpp#455

As mentioned before, restyle will process immediately when composing attribute, so nsSMILAnimationController::mightHavePendingStyleUpdates doesn't update yet..

Perhaps, we will need to check that current phase is DoSample or not when add restyle task to PendingTrakcer.

WIP patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac79b5cb47b13a1235951cba76d62aa23818eca4
I changed the approach of this.

nsSMILCSSProperty::GetBaseValue will cause the restyling. So we will notify to the TransitionManager directly in this case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6ac7c0c92aa4025f4da4af0edd55d0f9d985c93
Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review108058

::: dom/smil/nsSMILCSSProperty.cpp:106
(Diff revision 2)
> +  // In GetCSSComputedValue(), we will update style. So we need to prevent
> +  // creating a CSS Transition.
> +  SetAnimationOnlyStyleUpdateToTransitionManager();
>    bool didGetComputedVal = GetCSSComputedValue(mElement, mPropID,
>                                                   computedStyleVal);
> +  UnsetAnimationOnlyStyleUpdateToTransitionManager();

I don't see why this is safe to do.  Can't there be other changes buffered that this will flush, that should start transitions?

::: dom/smil/nsSMILCSSProperty.cpp:286
(Diff revision 2)
> +void
> +nsSMILCSSProperty::SetAnimationOnlyStyleUpdateToTransitionManager() const
> +{
> +  nsTransitionManager* manager = GetTransitionManager();
> +  if (manager) {
> +    manager->SetInAnimationOnlyStyleUpdate(true);
> +  }
> +}

If you were to continue with this approach (which I'm skeptical of), you'd want:

 (a) to rename these methods (remove the "ToTransitionManager", although if it stayed it should have been "OnTransitionManager")
 
 (b) to have callers use them through an RAII helper class
 
 (c) probably to add more assertions to SetInAnimationOnlyStyleUpdate to assert that it's always flipping the value between true and false, and never leaving it the same
Attachment #8818780 - Flags: review?(dbaron) → review-
Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review108058

Thanks dbaron,

I have several questions about this.

> I don't see why this is safe to do.  Can't there be other changes buffered that this will flush, that should start transitions?

I think that GetCSSComputedValue will call FlushPendingNotification finally, and we will need to prevent start transition when composing SMIL attributes.

Here's what I gather and I hope you can point out where I'm wrong:
 a. GetCSSComputed will call the nsComputedDOMStyle::UpdateCurrentStyleSources with needLayoutFlash flag is false.[1]
 b. Near the above this line, ValueFromString will call the nsComputedDOMStyle::GetStyleContextForElement[2].
 c. a) and b) will cause to call the FlushPendingNotification.
 d. In the result of a)-c), Compositing of SMIL Attribute will occur the style flush.

[1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/layout/style/nsComputedDOMStyle.cpp#975
[2] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/dom/smil/nsSMILCSSValueType.cpp#364
Flags: needinfo?(dbaron)
None of that contradicts anything I said.  The problem is that the flush will flush all pending changes, some of which might be for other things that should start transitions.

It's also far from ideal to insert additional flushes in our own code.  I'm somewhat unhappy to see that SMIL code is triggering flushes at all.
Flags: needinfo?(dbaron)
I had a quick look into what we need to do here. I think we want to align our base value handling with what we
do in dom/animation since that should mean:

a) More code reuse / less SMIL-specific hooks into the style system we need to add to Stylo
b) It's easier to migrate SMIL to re-use other animation machinery in future

To recap what we do for base values in dom/animation we resolve base styles using
nsStyleSet::ResolveStyleByRemovingAnimation which we call in the following situations:

 i. When the style context on the target element changes. Specifically, from nsStyleSet::GetContext where we
    call EffectCompositor::UpdateEffectProperties.
ii. When an animation is created or modified in such a way that might cause us to start needing base values.
    Specifically, from KeyframeEffect::SetTarget and KeyframeEffect::SetComposite.

That is, we don't do it on each tick, but only when we detect we might need to.

Adapting a similar approach in SMIL woukd mean we basically want to update base values in the following
situations:

 α. When an new SVG Animation element is created. We might refine this further to new animation elements that
    have additive="add" or use to-animation (i.e. basically when its animation function returns true from
    WillReplace()).
 β. When the additive attribute changes or when we become a to-animation (e.g. the from attribute is dropped).

There are a few tricky bits here. We *could* try to detect changes to the animation sandwich and not update
base values when additive animations build on top of non-additive ones but I suspect that's too complex.

We could also limit this to when we're animating a CSS property but base values for attribute animations can
be dependent on context too. For example, length attributes uses nsSVGLength2 whose GetBaseValue method
returns a normalized value that is dependent on the current font size.

All that might mean we regress a few tests due to things like bug 1254424 (i.e. failing to respond to changes
to the style of ancestors) but that will simply mean SMIL behaves more like CSS animations/transitions so it
probably acceptable as temporary regression.

If there are more significant regressions we might need to do some special case handling so that we do less
aggressive caching of non-CSS base values (like we currently do).

As for how to store this, the natural place to use is nsSMILAnimationController::mLastCompositorTable.
Basically we have the following arrangement:

  nsSMILAnimationController::mLastCompositorTable - Store cached base values from the last sample. When we
    sample again, we copy the cached values across to the new compositor.
  nsSMILAnimationController::mAnimationElementTable - Stores the actual animation elements (e.g. <animate>) we
    need to sample.

The tricky party here is that mLastCompositor is a hashtable keyed-off a triple of (target element, attribute
name, attribute namespace).

This works fine for β above since animation elements only animate a single attribute so we can easily find the
corresponding compositor in mLastCompositorTable and update it.

The hard part is α since in that case we want to update the base values in mLastCompositorTable for
a particular *target element*. We *could* do that by iterating through |mAnimationElementTable| and skipping
any elements that don't target the target element. For the remaining elements, we can easily find their
corresponding compositor mLastCompositorTable and update them. The problem with that, however, is if we need
to restyle 100 elements each with their own animation element, we basically visit 100 animation elements 100
times, i.e. it's an O(n^2) operation.

Instead, I think we might want to just update all compositors at once whenever we decide we need to update one
of them. I think we could probably arrange that by using the animation generation on the restyle manager and
store the latest animation generation we updated the base styles for on the nsSMILAnimationController. I'm not
sure quite how that would work for regular attributes whose base values are context-sensitive, however.

They're my initial thoughts anyway. I think something like that would at least prevent us from flushing style
when we go to get the base style. I still need to work out if we can get rid of the other flush in DoSample.
Marking this P1 since I think we should do this as part of getting SMIL to work with Stylo.
Blocks: stylo-smil
Priority: P2 → P1
Summary: CSS-Transition will create when changing style due to SMIL. → SMIL animations trigger CSS transitions
Wow, that's funny. I looked into why we Flush during DoSample and it appears to be that we added it in bug 592477 because the flush in GetBaseValue was sometimes causing us to crash (see bug 592477 comment 25)!

So in theory, at least, if we remove the flush from GetBaseValue we might be able to remove the flush from DoSample.
Whiteboard: [stylo]
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> (https://people-mozilla.org/~mantaroh/svg/transition_svg.svg)
> ----------------------------------------------------------
> 
> The result of each version is as follow:
> Before Firefox 40	: Work correctly.
> Firefox 46 ~ 48		: Repeat the color between red and blue.
> After Firefox 49	: Display red box. (and create the new transition after
> SMIL is finished.)

Sorry, my people.mozilla account has changed.
I confirmed using the following example to find regression version range:
https://people-mozilla.org/~myoshinaga/svg/bug1315874/test-fill-withTransition.html
Attached patch WIP patch (obsolete) — Splinter Review
I had a go at fixing this. For some reason I tied skipping flushing styles to using the sort of out-of-band
updating base values that we use in dom/animation but I'm not completely sure they are both necessary.

This WIP patch mostly works but fails six tests that I'm aware of. There are two root causes for those
failures:

a) We fail to update base values when we update mapped attributes on the target element. This should be easy
   to fix. It basically just involves adding another method to nsSMILAnimationController that will update
   a single property if there is an existing cached base value (much like the current UpdateBaseValues but for
   a single value).

b) We fail to update base values when an SVG animatable type (e.g. SVGLength) is updated. We could fairly
   easily make all these types do the same as (a) but that sounds like a lot of code so I suspect we should
   actually handle base styles and base attribute values differently. It's not too costly to fetch these base
   values on each tick (which is what we currently do) so we should just do that. We still need to cache them
   (so we can compare and see if the base value has changed and we need to recomposite) but we don't need to
   do the caching out-of-band.

Stealing this for now as discussed with Mantaroh.
Assignee: mantaroh → bbirtles
Status: NEW → ASSIGNED
I made a start on this but there are two problems with my approach:

1) I tried to make updating the base style out-of-band (i.e. not during DoSample). As mentioned in comment 27,
   I'm not sure why I conflated updating the base style out-of-band and not flushing. DoSample doesn't run as
   part of restyling so, provided we are careful not to flush, we should be able to get a style context
   without triggering the sort of nasty recursive issues we hit when adding base styles to dom/animation. The
   out-of-band logic is fairly solid but since we don't resolve base values for attribute animations
   out-of-band, adding this logic means we end up with two code paths which complicates the code.

2) I tried to use mLastCompositorTable to store the base styles. This *almost* works. The trouble is
   animations that start in the future. In that case when we go to build up the current compositor table we
   don't add those animation functions (since they're not *active*) and as a result we can't easily determine
   if we should create a new compositor for storing the base value from mLastCompositorTable or not.

   We could work around this by adding a flag to nsSMILCompositor and simply set that if we have any animation
   elements targetting the element that are additive (even if they're not active). That would probably work
   without adding too much more complexity but I'm afraid we'd end up preserving that table forever. That's
   because unlike CSS/Web Animations which are run-once, SMIL animations can be scheduled to re-run many times
   so it's hard to determine when they'd no longer need that base style.

With that said, I'm going to try and just resolve the base styles as part of ComposeAttribute using some version of nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation/NoFlush and see how that goes.

I'd still like to post the patches for this first approach since, apart from (2) above, I think they're reasonably solid and we may decide to retry that approach. However Mercurial has been "bundling" the changes for about 2 hours now and now it's just sitting there doing nothing so I'll have to wait and see if it ever succeeds in pushing them to my user repo.
Previous approach finally pushed to user repo: https://hg.mozilla.org/users/bbirtles_mozilla.com/main/log/59c166953b5b
Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

This has already been reviewed but MozReview is confused.
Attachment #8853864 - Flags: review?(dbaron)
Attachment #8850864 - Attachment is obsolete: true
Attachment #8818780 - Attachment is obsolete: true
Attachment #8817354 - Attachment is obsolete: true
Attachment #8818779 - Attachment is obsolete: true
Comment on attachment 8853863 [details]
Bug 1315874 - Sort includes in nsSMILAnimationController.cpp;

https://reviewboard.mozilla.org/r/125896/#review128688

r=me
Attachment #8853863 - Flags: review?(dholbert) → review+
Comment on attachment 8853865 [details]
Bug 1315874 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

https://reviewboard.mozilla.org/r/125900/#review128692

r=me

::: dom/smil/nsSMILCompositor.h:76
(Diff revision 1)
>   private:
> -  // Create a nsISMILAttr for my target, on the heap.  Caller is responsible
> -  // for deallocating the returned object.
> +  // Create a nsISMILAttr for my target, on the heap.
> +  UniquePtr<nsISMILAttr> CreateSMILAttr();
> -  nsISMILAttr* CreateSMILAttr();

s/UniquePtr/mozilla::UniquePtr/

::: dom/smil/nsSMILCompositor.cpp:59
(Diff revision 1)
>    if (!mKey.mElement)
>      return;
>  
>    // FIRST: Get the nsISMILAttr (to grab base value from, and to eventually
>    // give animated value to)
> -  nsAutoPtr<nsISMILAttr> smilAttr(CreateSMILAttr());
> +  UniquePtr<nsISMILAttr> smilAttr{CreateSMILAttr()};

If you like, I'd suggest the standard assignment-as-constructor syntax here -- I believe it's equivalent, and I find it more readable:
  UniquePtr<nsISMILAttr> smilAttr = CreateSMILAttr();

...but I suppose this is fine too.

(If you do change this to use assignment syntax, be sure to update the extended commit message, where you have a note about {}.)

::: dom/smil/nsSMILCompositor.cpp:116
(Diff revision 1)
>  nsSMILCompositor::ClearAnimationEffects()
>  {
>    if (!mKey.mElement || !mKey.mAttributeName)
>      return;
>  
> -  nsAutoPtr<nsISMILAttr> smilAttr(CreateSMILAttr());
> +  UniquePtr<nsISMILAttr> smilAttr{CreateSMILAttr()};

Here as well, I'd lean towards assignment (if you do want to change this), but not a big deal.
Attachment #8853865 - Flags: review?(dholbert) → review+
Comment on attachment 8853866 [details]
Bug 1315874 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

https://reviewboard.mozilla.org/r/125902/#review128708

r=me with one suggested comment nit.  (Thanks for the nice explanatory extended-commit-message here, BTW!)

::: dom/smil/nsSMILCompositor.h:100
(Diff revision 1)
>    // Cached base value, so we can detect & force-recompose when it changes
> -  // from one sample to the next. (nsSMILAnimationController copies this
> +  // from one sample to the next. (nsSMILAnimationController moves this
>    // forward from the previous sample's compositor.)

Might be good to mention "StealCachedBaseValue" in the comment here, since that's what actually does the Move()'ing of this value.

e.g. at the end of the parenthetical, maybe add:
 ", via StealCachedBaseValue()."
Attachment #8853866 - Flags: review?(dholbert) → review+
Comment on attachment 8853867 [details]
Bug 1315874 - Factor out nsSMILCompositor::IsCSSPropertyAnimation helper method;

https://reviewboard.mozilla.org/r/125904/#review128712

r- since this probably merits another look after the feedback has been addressed.

::: dom/smil/nsSMILCompositor.h:75
(Diff revision 1)
> +  // Returns true if, given an attributeName and corresponding property ID,
> +  // an animation targetting |aTargetElement| will animate the corresponding CSS
> +  // property rather than an attribute.
> +  //

Two things:

(1) s/targetting/targeting/  (i.e. drop the double-"t")

(The two-t version is perhaps an obsolete Britishism, according to https://en.wiktionary.org/wiki/targetting

Also, http://www.future-perfect.co.uk/grammar-tip/is-it-targetted-or-targeted/ is a UK source which agrees on the single-"t" form, though it focuses primarily on "-ed" rather than "-ing".)


(2) The relationship between the attribute name and property ID is confusing here, particularly in the case where the attribute *has* no "corresponding property" -- in that case, "the corresponding property ID" is a meaningless concept, so it's unclear what aPropID is expected to contain/represent then.  How about just focus on the attribute-name arg in the first sentence/paragraph here, and then add an afterthought to mention the expectation/optimization about "aPropID"?

::: dom/smil/nsSMILCompositor.cpp:127
(Diff revision 1)
> +nsSMILCompositor::IsCSSPropertyAnimation(nsIAtom* aAttributeName,
> +                                         nsCSSPropertyID aPropID,
> +                                         mozilla::dom::Element* aTargetElement)
> +{
> +  MOZ_ASSERT(aAttributeName);
> +  MOZ_ASSERT(aTargetElement);

You should add an assertion about the expected relationship between aAttributeName and aPropID here.  (a debug-only call to LookupProperty, I think.)  This will help reinforce/document/verify that callers should pass these args in a consistent/coherent manner.

::: dom/smil/nsSMILCompositor.cpp:143
(Diff revision 1)
> +  bool animateAsAttr = (aAttributeName == nsGkAtoms::width ||
> +                        aAttributeName == nsGkAtoms::height) &&
> +                        aTargetElement->GetNameSpaceID() == kNameSpaceID_SVG &&
> +                        !aTargetElement->IsAttributeMapped(aAttributeName);
> +
> +  return !animateAsAttr;

This inverted-logic with the local var ("return !animateAsAttr") is kinda confusing.  Let's just rework the logic slightly to replace the confusing single-usage local-var with an early-return.  (This wasn't as straightforward in the old place where this code lived, but it's quite easy in this new refactored home.)

Specifically: let's replace the code starting at "bool animateAsAttr" with something like the following:
=====
  if (animating width or height on non-root element) {
    return false;
  }

  // Otherwise, we should treat this as an animation of the
  // corresponding CSS property.
  return true;
=====
Attachment #8853867 - Flags: review?(dholbert) → review-
Comment on attachment 8853868 [details]
Bug 1315874 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not;

https://reviewboard.mozilla.org/r/125906/#review128720

::: commit-message-d6d29:1
(Diff revision 1)
> +Bug 1315874 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not; r=dholbert

s/when decided/when deciding/

::: dom/smil/nsSMILCompositor.cpp:161
(Diff revision 1)
>    nsCSSPropertyID propID =
>      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
>                                 CSSEnabledState::eForAllContent);
>  
> -  if (IsCSSPropertyAnimation(mKey.mAttributeName, propID, mKey.mElement)) {
> +  if (IsCSSPropertyAnimation(mKey.mAttributeNamespaceID, mKey.mAttributeName,

So now we'll be calling nsCSSProps::LookupProperty on the attribute-name, and then later (inside of this helper function) we check the namespace.

This feels a little backwards. Shouldn't we be checking the namespace ID *before* we bother calling nsCSSProps::LookupProperty? If there's a namespace, then clearly it's not a valid CSS property and there are no circumstances under which we should care about what nsCSSProps::LookupProperty says, I think.

So from a cart-before-the-horse perspective, as well as from an efficiency perspective, I tend to think the (fast) namespace check should happen first & should prevent us from ever calling nsCSSProps::LookupProperty.
Attachment #8853868 - Flags: review?(dholbert) → review-
Comment on attachment 8853867 [details]
Bug 1315874 - Factor out nsSMILCompositor::IsCSSPropertyAnimation helper method;

https://reviewboard.mozilla.org/r/125904/#review128730

::: dom/smil/nsSMILCompositor.cpp:160
(Diff revision 1)
>  {
>    nsCSSPropertyID propID =
>      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
>                                 CSSEnabledState::eForAllContent);
> -  if (nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
> -    // If we are animating the 'width' or 'height' of an outer SVG
> +
> +  if (IsCSSPropertyAnimation(mKey.mAttributeName, propID, mKey.mElement)) {

OK, I have an idea to restructure this which I think would help sort out some of my comments here & for the next patch.

How about:
 (1) we make the new helper-method *perform the propID lookup itself*, and then *return the property ID as its actual return type*. (rather than a boolean value)
 (2) In cases where your current method should return false, it should return the sentinel value eCSSProperty_UNKNOWN.  So notably, width/height would usually return eCSSProperty_UNKNOWN from this method (except for on the root element, in which case they'd return the expected property ID)
 (3) The helper-method can do the namespace lookup (like you've got it doing in your next patch), BUT that should happen before it calls nsCSSProps::LookupProperty.
 (4) It should also just be a normal method (not a static method), since AFAICT all of the callers pass in the same member-data, which a legit method could simply read directly.

This removes some awkwardness that the current setup is running into, I think.  What do you think?
Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

https://reviewboard.mozilla.org/r/125910/#review128726

Not r+'ing this one, since I think it'll change soon, per comment 50.

Minor comment nit, though:

::: dom/smil/nsSMILCompositor.h:92
(Diff revision 1)
>   private:
>    // Create a nsISMILAttr for my target, on the heap.
>    UniquePtr<nsISMILAttr> CreateSMILAttr();
>  
> +  // Returns true if we might need to refer to base styles (i.e. we are
> +  // targetting a style property and have one or more animation functions that

s/targetting/targeting/

s/style property/CSS property/
Thanks Daniel. I'll go and rework that awkwardness. It mostly is just left over from the previous attempt I made at this bug where we really did want those methods to be public static and really did want to pass in that property separately. Now that I've simplified the patch series, what you suggest seems much better.
Depends on: 1353208
Tidy up patches have been split off into bug 1353208.
Attachment #8853862 - Attachment is obsolete: true
Attachment #8853862 - Flags: review?(mantaroh)
Attachment #8853863 - Attachment is obsolete: true
Attachment #8853865 - Attachment is obsolete: true
Attachment #8853866 - Attachment is obsolete: true
Attachment #8853867 - Attachment is obsolete: true
Attachment #8853868 - Attachment is obsolete: true
Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

(This patch has already been reviewed.)
Attachment #8853864 - Flags: review?(dbaron) → review+
Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

https://reviewboard.mozilla.org/r/125910/#review128988
Attachment #8853870 - Flags: review?(dholbert) → review+
Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

https://reviewboard.mozilla.org/r/125908/#review128992

::: dom/smil/nsSMILCSSValueType.cpp:428
(Diff revision 2)
> +  nsAutoString valueAsString;
> +  DebugOnly<bool> uncomputeResult =
> +    StyleAnimationValue::UncomputeValue(aPropID, aValue, valueAsString);
> +  // |aValue| is typically created using
> +  // StyleAnimationValue::ExtractComputedValue so we should be able to
> +  // serialize it.
> +  MOZ_ASSERT(uncomputeResult,
> +             "Should be able to serialize provided StyleAnimationValue");
> +
> +  nsIDocument* doc = aTargetElement->GetUncomposedDoc();
> +  if (doc && !nsStyleUtil::CSPAllowsInlineStyle(nullptr,
> +                                                doc->NodePrincipal(),
> +                                                doc->GetDocumentURI(),
> +                                                0, valueAsString, nullptr)) {
> +    return result;

I initially assumed this patch was meant to be "shortcut" to avoid serialization/deserialization. But it seems we're still stringifying here, so it doesn't seem like we save much after all. :(

HOWEVER, do we really have to stringify? It looks like it's only for the CSPAllowsInlineStyle call, and it's only there "for reporting violations":
https://dxr.mozilla.org/mozilla-central/rev/b5d8b27a753725c1de41ffae2e338798f3b5cacd/layout/style/nsStyleUtil.h#184-185

And I kinda doubt that an intermediate CSS value would be very useful for CSP-violation-error reporting anyway. (depending on how this is used).

Could we just skip the stringification here, and instead pass in something like NS_LITERAL_STRING("[SVG animation of CSS]")?
(In reply to Daniel Holbert [:dholbert] from comment #64)
> Comment on attachment 8853869 [details]
> Bug 1315874 - Add a method to nsSMILCSSValueType that takes a
> StyleAnimationValue;
> I initially assumed this patch was meant to be "shortcut" to avoid
> serialization/deserialization. But it seems we're still stringifying here,
> so it doesn't seem like we save much after all. :(

We still save the parse/style-resolution/conversion steps which I think is the more significant part here.

> HOWEVER, do we really have to stringify? It looks like it's only for the
> CSPAllowsInlineStyle call, and it's only there "for reporting violations":
> https://dxr.mozilla.org/mozilla-central/rev/
> b5d8b27a753725c1de41ffae2e338798f3b5cacd/layout/style/nsStyleUtil.h#184-185
> 
> And I kinda doubt that an intermediate CSS value would be very useful for
> CSP-violation-error reporting anyway. (depending on how this is used).
> 
> Could we just skip the stringification here, and instead pass in something
> like NS_LITERAL_STRING("[SVG animation of CSS]")?

Sure, sounds good to me!
Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

https://reviewboard.mozilla.org/r/125912/#review129206

::: layout/style/nsComputedDOMStyle.h:111
(Diff revision 3)
> +                                              aStyleType,
> +                                              eWithAnimation);
> +  }
>  
>    static already_AddRefed<nsStyleContext>
> -  GetStyleContextForElementNoFlush(mozilla::dom::Element* aElement,
> +  GetNonAnimationStyleContextNoFlush(mozilla::dom::Element* aElement,

All the other GetStyleContext* functions have "ForElement" in the name, so it's a shame this one is inconsistent.  Can you make this one consistent, either by adding "ForElement" or (and I think I prefer this) removing "ForElement" from the names of the others?
Attachment #8853871 - Flags: review?(cam) → review+
Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

https://reviewboard.mozilla.org/r/125914/#review129212

::: layout/style/nsComputedDOMStyle.cpp:616
(Diff revision 3)
> +        // The existing style context may have animation styles so check if we
> +        // need to remove them.
> +        if (aAnimationFlag == eWithoutAnimation) {
> +          nsPresContext* presContext = presShell->GetPresContext();
> +          MOZ_ASSERT(!presContext || presContext->StyleSet()->IsGecko(),
> +                     "Stylo: Need ResolveStyleByRemovingAnimation for stylo");

Nit: we tend to use lowercase "stylo:" prefixes.

::: layout/style/nsComputedDOMStyle.cpp:617
(Diff revision 3)
> +        // need to remove them.
> +        if (aAnimationFlag == eWithoutAnimation) {
> +          nsPresContext* presContext = presShell->GetPresContext();
> +          MOZ_ASSERT(!presContext || presContext->StyleSet()->IsGecko(),
> +                     "Stylo: Need ResolveStyleByRemovingAnimation for stylo");
> +          if (presContext && presContext->StyleSet()->IsGecko()) {

I'm not sure whether it's possible for an element to have a frame (and thus an existing style context) if the pres shell has no pres context.  It looks like we have assertions in PresShell::Init that the pres context is non null, and in non-DEBUG builds we'll just return early.  Notwithstanding the fact that there's an existing null check of the pres context later in the function.

But if this case can come up, I wonder whether it makes more sense to return nullptr than return the frame's existing styles, which might have animations in them anyway.
Attachment #8853872 - Flags: review?(cam) → review+
Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

https://reviewboard.mozilla.org/r/125916/#review129220
Attachment #8853873 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #74)
> Comment on attachment 8853871 [details]
> Bug 1315874 - Make GetStyleContextForElementWithoutAnimation not flush;
> 
> https://reviewboard.mozilla.org/r/125912/#review129206
> 
> ::: layout/style/nsComputedDOMStyle.h:111
> (Diff revision 3)
> > +                                              aStyleType,
> > +                                              eWithAnimation);
> > +  }
> >  
> >    static already_AddRefed<nsStyleContext>
> > -  GetStyleContextForElementNoFlush(mozilla::dom::Element* aElement,
> > +  GetNonAnimationStyleContextNoFlush(mozilla::dom::Element* aElement,
> 
> All the other GetStyleContext* functions have "ForElement" in the name, so
> it's a shame this one is inconsistent.  Can you make this one consistent,
> either by adding "ForElement" or (and I think I prefer this) removing
> "ForElement" from the names of the others?

I've added the ForElement but the name is now really long: GetNonAnimationStyleContextForElementNoFlush

Doing s/NonAnimation/Unanimated/, i.e. GetUnanimationStyleContextForElementNoFlush would save 2 characters and seems more readable to me. What do you think?
Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

https://reviewboard.mozilla.org/r/125918/#review129242
Attachment #8853874 - Flags: review?(cam) → review+
Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

https://reviewboard.mozilla.org/r/125920/#review129244
Attachment #8853875 - Flags: review?(cam) → review+
Comment on attachment 8854711 [details]
Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

https://reviewboard.mozilla.org/r/126670/#review129260

::: layout/style/nsComputedDOMStyle.h:87
(Diff revision 1)
>    static already_AddRefed<nsStyleContext>
> -  GetStyleContextForElement(mozilla::dom::Element* aElement, nsIAtom* aPseudo,
> +  GetStyleContext(mozilla::dom::Element* aElement, nsIAtom* aPseudo,
>                              nsIPresShell* aPresShell,
>                              StyleType aStyleType = eAll);
>  

Oops, looks like I forgot to fix the indentation here.
Comment on attachment 8854711 [details]
Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

https://reviewboard.mozilla.org/r/126670/#review129272
Attachment #8854711 - Flags: review?(cam) → review+
Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

https://reviewboard.mozilla.org/r/125908/#review129600

::: dom/smil/nsSMILCSSValueType.cpp:429
(Diff revisions 2 - 3)
> +  // We'd like to avoid serializing |aValue| if possible, and since the
> +  // string passed to CSPAllowsInlineStyle is only used for reporting violations
> +  // and an intermediate CSS value is not likely to be particularly useful
> +  // in that case, we just use a generic placeholder string instead.
> +  static const nsLiteralString kPlaceholderText =
> +    NS_LITERAL_STRING("[SVG animation of CSS]");

(On top of what you're saying here: I suspect that if the CSP would block this style, then it would have already blocked the animation earlier, e.g. in our first entry point at the string-accepting ValueFromString method.  So we would never even get here in that scenario, probably.  So, I don't expect this string will ever actually be used, in practice.  If that turns out to be wrong, we might want to consider making this something localized/etc [or falling back to just serializing the actual CSS like you originally did] -- but for now I'm not concerned about this being used particularly often.)
Attachment #8853869 - Flags: review?(dholbert) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d2b2c67b7f
Add test that SMIL does not trigger CSS Transitions; r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f233c0466f6
Add a method to nsSMILCSSValueType that takes a StyleAnimationValue; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf852fc7100f
Add MightNeedBaseStyle helper method; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b653d2bd81e
Drop 'ForElement' from GetStyleContextForElementXXX methods; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/db648d833b24
Make GetStyleContextWithoutAnimation not flush; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa276da14f2
Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/62092e494572
Tweak some line wrapping in DoGetStyleContextNoFlush; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d766681e346e
Pass a base style context to nsSMILCSSProperty; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/42be15d3ed1b
Use base style context to resolve base values in nsSMILCSSProperty; r=heycam
I assume we're letting this ride the trains? :)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #98)
> I assume we're letting this ride the trains? :)

Yes, this doesn't seem to be particularly critical.
Flags: needinfo?(bbirtles)
Blocks: 1354393
Depends on: 1357979
Depends on: 1402547
See Also: → 1538489
You need to log in before you can comment on or make changes to this bug.