Closed Bug 1057231 Opened 10 years ago Closed 10 years ago

put SMIL/SVG Animation rules in their own level of the CSS cascade

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(4 files, 1 obsolete file)

patch 1 - Add a rule processor class for SVG Animation rules
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
patch 2 - Add SVG Animation rule processor to the document
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
patch 4 - Move SVG Animation rules from the pres hint level to their own cascade level
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
patch 3 - Add new cascade level for rules from SMIL Animation of SVG attributes to the style set
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review 
      No description provided.
Blocks: 977991
Summary: remove the animation phase check from the addition of SMIL/SVG Animation rules → put SMIL/SVG Animation rules in their own level of the CSS cascade
I want to put SMIL Animation style rules, currently added at the end of
nsSVGElement::WalkContentStyleRules, in their own level of the CSS
cascade.

This is needed so that they can participate in the mechanism for
updating only animation styles to the present time without touching any
other styles.

In theory, this should have been done as part of 996796 or even as part
of prior work on off main thread animations, except that it doesn't seem
possible to have SMIL Animations and CSS Transitions interacting on the
same element and have that element's style out of date, which means it
wasn't necessary for the existing code to update only animation styles.

However, bug 960465 will use this similar mechanism as the new mechanism
for making sure transitions start in a reasonable way (replacing the
mechanism of phases, going from style without animation to style with
animation via nsPresContext::IsProcessingAnimationStyleChange).  This
will require that, for SMIL Animations to continue interacting correctly
with CSS Transitions, that they have their own cascade level so that
they can participate in RestyleManager::UpdateOnlyAnimationStyles.

Additionally, this is needed even sooner, for bug 977991, which needs
(temporarily, until bug 960465 lands) a mechanism for updating the style
attribute style *and* all animation-phase-dependent styles.
Blocks: 1058346
This patch series (to move SVG Animation rules to their own level of the
cascade) is needed so that they can participate in the mechanism for
updating only animation styles to the present time without touching any
other styles.

In theory, this should have been done as part of 996796 or even as part
of prior work on off main thread animations, except that it doesn't seem
possible to have SVG Animations and CSS Transitions interacting on the
same element and have that element's style out of date, which means it
wasn't necessary for the existing code to update only animation styles.

However, bug 960465 will use this similar mechanism as the new mechanism
for making sure transitions start in a reasonable way (replacing the
mechanism of phases, going from style without animation to style with
animation via nsPresContext::IsProcessingAnimationStyleChange).  This
will require that, for SVG Animations to continue interacting correctly
with CSS Transitions, that they have their own cascade level so that
they can participate in RestyleManager::UpdateOnlyAnimationStyles.

Additionally, this is needed even sooner, for bug 977991, which needs
(temporarily, until bug 960465 lands) a mechanism for updating the style
attribute style *and* all animation-phase-dependent styles.

Yes, it's a little bit annoying to have to have another class, and
another object per-document for this, but that's currently what comes
with being a level of the cascade.  But it's one class and one object
per document, and I believe there will be substantial benefits to having
all rule processor levels be only-animation-related or
not-animation-related.
Attachment #8478781 - Flags: review?(dholbert)

        Attachment #8478782 -
        Flags: review?(dholbert)

    
    

    
  


  
I will fix the indentation of gCascadeLevels in bug 997991 patch 2,
which adds a member to each element of the array.

        Attachment #8478783 -
        Flags: review?(dholbert)

    
    

    
  


  
This is only a very slight reordering of their position in the cascade,
since they were previously walked at the end of
nsSVGElement::WalkContentStyleRules, which was called near the end of
nsHTMLStyleSheet::RulesMatching.  So the only change should be that they
now take priority over the xml:lang rule added by nsHTMLStyleSheet, a
rule with which they do not interact.

        Attachment #8478784 -
        Flags: review?(dholbert)

    
    

    
  
Also, I'm curious if you think "SVG Animation" or "SMIL Animation" is the better term in the code.
Flags: needinfo?(dholbert)
Flags: needinfo?(birtles)

    
    

    
  
(or maybe SVG Animation Attributes or some shorter version thereof?)

    
    

    
  
I'd lean towards "SMIL Animation", since:
 (a) It's less ambiguous (since there are multiple ways of animating SVG content)

 (b) It's what we tend to use for this form of animation elsewhere in the code. (nsSMILAnimationFunction, nsSMILAnimationController, nsISMILAttr, nsISMILType, etc.)
Flags: needinfo?(dholbert)

    
    

    
  
Does "SMILPresHint" work?  (It's worth noting that this is only the attribute animation and not the CSS property animation.)

    
    

    
  
Ah, right. So this is for e.g. animations of the "fill" _attribute_, as opposed to the "fill" CSS property.

i.e. this is for <animate attributeName="fill" attributeType="XML">
and not for      <animate attributeName="fill" attributeType="CSS">

Given that, we should have "attr" or "mapped attr" in the naming.  So, maybe "SMILAttrAnim" or "SMILMappedAttrAnim"?  (if that's not too long)

("Attr" (without "Mapped") could be a little confusing since there are SMIL animations of non-CSS attributes like <rect> x,y,width,height.  It's probably fine, though, because those attributes clearly aren't relevant to the CSS cascade.)

    
    

    
  
Hmmm... after a bit more thought, I suppose this is *SMIL Animation*, but the attributes are *SVG attributes* (and not "SMIL attributes"; so despite what I said in my previous comment, I'm now leaning against "SMILAttr"-based naming here).

So, I'm leaning towards some shortened version of either "SMIL-Animated Attribute" or "SVG Attribute Animations".

My current preverence is for "SVGAttrAnimation", so e.g. we'd have "eSVGAttrAnimationSheet" in patch 3. But I'm open to other alternatives.

    
    

    
  
Comment on attachment 8478781 [details] [diff] [review]
patch 1 - Add a rule processor class for SVG Animation rules

Review of attachment 8478781 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/SVGAnimationRuleProcessor.cpp
@@ +85,5 @@
> +  return 0; // SVGAnimationRuleProcessors are charged to the DOM, not layout
> +}
> +
> +size_t
> +SVGAnimationRuleProcessor::DOMSizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const

It looks like this function (DOMSizeOfIncludingThis) is what we're relying on for memory reporting.  But, does it get called anywhere?

It looks like it's based on nsHTMLStyleSheet::DOMSizeOfIncludingThis, which is explicitly called for nsDocument's nsHTMLStyleSheet instance, here in nsDocument::DocAddSizeOfExcludingThis():
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=68d28e7fb162&mark=12089-12089#12087

Presumably we need to add a similar call there for nsDocument's SVGAnimationRuleProcessor instance? (in a later patch, maybe)

::: layout/style/SVGAnimationRuleProcessor.h
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* style rule processor for SVG Animation (SMIL Animation) rules */

This comment (both in the .h and the .cpp file) probably needs updating to say something more mapped-attribute-specific, like:

/* style rule processor to handle SMIL animation of SVG mapped attributes
   (attributes whose values are mapped into style) */

@@ +14,5 @@
> +class nsIDocument;
> +
> +namespace mozilla {
> +
> +class SVGAnimationRuleProcessor MOZ_FINAL : public nsIStyleRuleProcessor

(Per comment 12, the class name & file name will probably want to be "SVGAttrAnimationRuleProcessor", using whatever label we end up settling on here).

        Attachment #8478781 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8478782 [details] [diff] [review]
patch 2 - Add SVG Animation rule processor to the document

Review of attachment 8478782 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this patch is probably where we should be adding a call to the new class's DOMSizeOfIncludingThis method, from inside nsDocument::DocAddSizeOfExcludingThis().

        Attachment #8478782 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8478783 [details] [diff] [review]
patch 3 - Add new cascade level for SVG Animation (SMIL Animation) rules to the style set

Review of attachment 8478783 [details] [diff] [review]:
-----------------------------------------------------------------

Two notes on the commit message:
> Bug 1057231 patch 3 - Add new cascade level for SVG Animation (SMIL Animation) rules to the style set

(This should probably say "SMIL animation of SVG attributes" (or maybe "of SVG mapped attributes"))

> I will fix the indentation of gCascadeLevels in bug 997991 patch 2,

Wrong bug number there, I think. Perhaps you meant bug 977991?

::: layout/style/nsStyleSet.cpp
@@ +189,5 @@
>    mRuleTree = nsRuleNode::CreateRootNode(aPresContext);
>  
>    GatherRuleProcessors(eAnimationSheet);
>    GatherRuleProcessors(eTransitionSheet);
> +  GatherRuleProcessors(eSVGAnimationSheet);

Do we need this call?

I'm unclear why we have explicit GatherRuleProcessors() calls for eAnimationStyleSheet and eTransitionSheet (and now maybe (eSVGAnimationSheet), but not for eStyleAttrSheet or ePresHintSheet (though the latter enum values have code to set them up in GatherRuleProcessors(); hence, presumably they're invoked indirectly somehow).

And if we already will end up getting an indirect call to GatherRuleProcessors() for this new enum type like we do for eStyleAttrSheet or ePresHintSheet, maybe we don't need a direct call here? (not sure)

::: layout/style/nsStyleSet.h
@@ +274,5 @@
>    enum sheetType {
>      eAgentSheet, // CSS
>      eUserSheet, // CSS
>      ePresHintSheet,
> +    eSVGAnimationSheet,

The comment at the bottom of this enum list says we might need to update NS_RULE_NODE_LEVEL_MASK if we add new values here... do we?  (Right now it's 0xf0000000, and I'm not clear on what sort of update would be needed, if any.)

        Attachment #8478784 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #7)
> Also, I'm curious if you think "SVG Animation" or "SMIL Animation" is the
> better term in the code.

I agree with Daniel's comments, i.e. "SVG Attribute Animation" or something along those lines seems good.

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Ah, right. So this is for e.g. animations of the "fill" _attribute_, as
> opposed to the "fill" CSS property.
> 
> i.e. this is for <animate attributeName="fill" attributeType="XML">
> and not for      <animate attributeName="fill" attributeType="CSS">

I didn't realise this actually made a difference. Is it important to make this distinction?

I'd like to make attributeType obsolete in the future. In SVG we're promoting most animatable attributes to properties. So hopefully the set of attributes we need will be quite small in the future and the chance of clashing with a CSS property name will be reduced. (Some of the commonly animated attributes that aren't going to be promoted to properties include "class", "href" and "d").
Flags: needinfo?(birtles) → needinfo?(dholbert)

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #16)
> > i.e. this is for <animate attributeName="fill" attributeType="XML">
> > and not for      <animate attributeName="fill" attributeType="CSS">
> 
> I didn't realise this actually made a difference. Is it important to make
> this distinction?

This is a bit outside the scope of this bug, but I'm open to removing the distinction. I implemented it that way because otherwise, "attributeType" is just meaningless.  (The idea is, attributeType="XML" is conceptually equivalent to animating the XML attribute (where it applies in the cascade, etc.) and attributeType="CSS" is conceptually equivalent to an animation of the contents of the style="..." attribute.  I'm happy for this distinction to go away, though.  See bug 534028 comment 7 for some discussion about this. (It references some spec text that agrees with you, but that spec-text is too hand-wavy to be really meaningful.)
Flags: needinfo?(dholbert)

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #17)
> (In reply to Brian Birtles (:birtles) from comment #16)
> > > i.e. this is for <animate attributeName="fill" attributeType="XML">
> > > and not for      <animate attributeName="fill" attributeType="CSS">
> > 
> > I didn't realise this actually made a difference. Is it important to make
> > this distinction?
> 
> This is a bit outside the scope of this bug, but I'm open to removing the
> distinction.

I filed bug 1062106 for this.

    
    

    
  


  
I will fix the indentation of gCascadeLevels in bug 977991 patch 2,
which adds a member to each element of the array.

Note that this bumps the maximum sheetType from 8 to 9 (and number of
them from 9 to 10), which does not require updating
NS_RULE_NODE_LEVEL_MASK, since NS_RULE_NODE_LEVEL_MASK currently has 4
bits and allows a maximum of 15.

        Attachment #8488851 -
        Flags: review?(dholbert)

    
  

        Attachment #8478783 -
        Attachment is obsolete: true

        Attachment #8478783 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8488851 [details] [diff] [review]
patch 3 - Add new cascade level for rules from SMIL Animation of SVG attributes to the style set

Review of attachment 8488851 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, modulo nits below.  One high-level thought, though -- I wonder if it'd be better to not fix this after all, and just fix bug 1062106? (treating all mapped attributes all as if they had attributeType="CSS", basically)  It seems like there's brooder agreement about attributeType being ignorable (among SVGWG members & other rendering engines) than there was at the time that I added support for this sort of animation in bug 534028.

If we land this bug's patches and then subsequently fix bug 1062106, it seems like this bug's code would become unnecessary at that point... and this is a decently complex thing to land, with the intention of reverting in the near future.

r=me, with that reservation.

::: layout/style/nsStyleSet.cpp
@@ +189,5 @@
>    mRuleTree = nsRuleNode::CreateRootNode(aPresContext);
>  
> +  // Make an explicit GatherRuleProcessors call for the levels that
> +  // don't have style sheets.  The other levels will have their calls
> +  // triggered by DirtyRuleProcessors).  (We should probably convert the

Straggling close-paren (w/o matching open-paren) after "DirtyRuleProcessors".

@@ +385,3 @@
>        return NS_OK;
> +    case eSVGAttrAnimationSheet:
> +      MOZ_ASSERT(mSheets[aType].Count() == 0);

Maybe use .IsEmpty()?

        Attachment #8488851 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #20)
> Looks good, modulo nits below.  One high-level thought, though -- I wonder
> if it'd be better to not fix this after all, and just fix bug 1062106?
> (treating all mapped attributes all as if they had attributeType="CSS",
> basically)  It seems like there's brooder agreement about attributeType
> being ignorable (among SVGWG members & other rendering engines) than there
> was at the time that I added support for this sort of animation in bug
> 534028.
> 
> If we land this bug's patches and then subsequently fix bug 1062106, it
> seems like this bug's code would become unnecessary at that point... and
> this is a decently complex thing to land, with the intention of reverting in
> the near future.
> 
> r=me, with that reservation.

I think I'd rather land this, given that there's a bunch of other stuff depending on this, and I'd rather not make that all depend on behavior changes that could lead to breakage; it has enough dependencies already.  Reverting it is pretty straightforward if that's the right thing.

    
    

    
  
OK, that sounds good then.

    
  
Depends on: 1066911

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: