Closed Bug 1355348 Opened 3 years ago Closed 3 years ago

stylo: Get non-additive SMIL animations to work

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 6 obsolete files)

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
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
41 bytes, text/x-github-pull-request
Details | Review
3.07 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
I'm currently trying to work out what to do with shorthand values. SMIL currently just stores them in a StyleAnimationValue as a string and then fails to interpolate them.

For the following test case we fall back to discrete animation (as required by SMIL when interpolation fails):

  <svg>
    <text dy=1em id=text>Abc</text>
    <animate href="#text" dur=2s attributeName=font
             to="40px serif"
             fill="freeze"/>
  </svg>

Neither Chrome nor Safari animate this.

However, we have a couple of tests that rely on animating shorthands (using discrete interpolation). I think we could do one of the following:

a) Use a PropertyDeclarationBlock for shorthands and do discrete interpolation for them
b) Expand shorthands and store an array of AnimationValue objects and interpolate each pair
c) Deprecate animating shorthands in SMIL

(b) Is probably my ideal option. It would likely involve building up a Servo PropertyDeclarationBlock in nsSMILCSSValueType::ValueToString using the interpolated longhand components and then calling Servo_DeclarationBlock_SerializeOneValue much like we do in KeyframeEffectReadOnly::GetKeyframes. It will take a bit of work, however, to make everything in nsSMILCSSValueType and nsSMILCSSProperty able to deal with an array of values.

(c) is also an attractive option since it would actually improve interop.

For now, I'll leave dealing with this to a separate bug where we either deprecate SMIL animation of shorthand properties in the Gecko code path, or do (b) from above.
Depends on: 1359658
Attachment #8857768 - Attachment is obsolete: true
Comment on attachment 8861751 [details]
Bug 1355348 - Make Servo_NoteExplicitHints allow multiple animation restyle hints, including SMIL;

https://reviewboard.mozilla.org/r/133744/#review136620

::: servo/ports/geckolib/glue.rs:1799
(Diff revision 1)
> +    debug_assert!(!restyle_hint.intersects(RestyleHint::for_animations()) ||
> +                  !restyle_hint.intersects(!RestyleHint::for_animations()),

I think RestyleHint::for_animations().contains(restyle_hint) should work.
Attachment #8861751 - Flags: review?(hikezoe) → review+
Comment on attachment 8861755 [details]
Bug 1355348 - Move ServoComputedValuesWithParent to separate file;

https://reviewboard.mozilla.org/r/133752/#review136622
Attachment #8861755 - Flags: review?(hikezoe) → review+
Comment on attachment 8861756 [details]
Bug 1355348 - Add constructor to AnimationValue;

https://reviewboard.mozilla.org/r/133754/#review136626
Attachment #8861756 - Flags: review?(hikezoe) → review+
Comment on attachment 8861748 [details]
Bug 1355348 - Add SMIL override cascade level;

https://reviewboard.mozilla.org/r/133738/#review136618

::: servo/components/style/rule_tree/mod.rs:268
(Diff revision 1)
>          for node in iter {
> -            if node.cascade_level() != CascadeLevel::Animations &&
> +            if node.cascade_level() != CascadeLevel::SMILOverride &&
> +               node.cascade_level() != CascadeLevel::Animations &&
>                 node.cascade_level() != CascadeLevel::Transitions {
>                  children.push((node.get().source.clone().unwrap(), node.cascade_level()));
>              }
>              last = node;
>          }

Can we add a CascadeLevel::is_animation function for checking these three values?

::: servo/components/style/rule_tree/mod.rs:789
(Diff revision 1)
> -            .any(|node| matches!(node.cascade_level(), CascadeLevel::Animations | CascadeLevel::Transitions))
> +            .any(|node| matches!(node.cascade_level(),
> +                                 CascadeLevel::SMILOverride |
> +                                 CascadeLevel::Animations |
> +                                 CascadeLevel::Transitions))

And we could use that function here too, in place of matches!.
Attachment #8861748 - Flags: review?(cam) → review+
Comment on attachment 8861749 [details]
Bug 1355348 - Rename remove_animation_and_transition_rules to just remove_animation_rules;

https://reviewboard.mozilla.org/r/133740/#review136628

I almost suggested this myself, on the previous patch, but convinced myself that in "animations_or_transitions" the "animations" could cover both CSS Animations and SMIL Animations. :-)
Attachment #8861749 - Flags: review?(cam) → review+
Comment on attachment 8861750 [details]
Bug 1355348 - Add restyle hint for SMIL animations;

https://reviewboard.mozilla.org/r/133742/#review136630

::: servo/components/style/matching.rs:1082
(Diff revision 1)
> +                if hint.contains(RESTYLE_SMIL) {
> +                    replace_rule_node(CascadeLevel::SMILOverride,
> +                                      self.get_smil_override(),
> +                                      primary_rules);
> +                }
> +

Nit: I would move this down below the replace_rule_node_for_animation declaration, since that function feels more like a prelude for the real work of the function, which begins with replacing the SMIL rule node.

::: servo/components/style/restyle_hints.rs:110
(Diff revision 1)
>      pub fn for_self() -> Self {
> -        RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS
> +        RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | RESTYLE_SMIL | RESTYLE_CSS_ANIMATIONS |
> +            RESTYLE_CSS_TRANSITIONS
>      }
>  
>      /// The subset hints that are used for animation restyle.
>      pub fn for_animations() -> Self {
> -        RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS
> +        RESTYLE_SMIL | RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS
>      }

While you're here, consider adding #[inline] to these, and making for_self() be:

  RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | Self::for_animations()
Attachment #8861750 - Flags: review?(cam) → review+
Comment on attachment 8861757 [details]
Bug 1355348 - Factor out method to remove minus sign from property values;

https://reviewboard.mozilla.org/r/133756/#review136632

::: dom/smil/nsSMILCSSValueType.cpp:339
(Diff revision 1)
>    }
>    nsIPresShell* shell = doc->GetShell();
>    return shell ? shell->GetPresContext() : nullptr;
>  }
>  
> -// Helper function to parse a string into a StyleAnimationValue
> +static nsDependentSubstring

const nsDependentSubstring?

::: dom/smil/nsSMILCSSValueType.cpp:381
(Diff revision 1)
>    if (!styleContext) {
>      return false;
>    }
> -  nsDependentSubstring subString(aString, subStringBegin);
> +
> +  bool isNegative = false;
> +  nsDependentSubstring subString =

Likewise.
Attachment #8861757 - Flags: review?(hikezoe) → review+
Comment on attachment 8861752 [details]
Bug 1355348 - Add FFI for getting SMIL override styles;

https://reviewboard.mozilla.org/r/133746/#review136634
Attachment #8861752 - Flags: review?(cam) → review+
Comment on attachment 8861753 [details]
Bug 1355348 - Move IsFlattenedTreeDescendantOf helper to nsContentUtils;

https://reviewboard.mozilla.org/r/133748/#review136636
Attachment #8861753 - Flags: review?(cam) → review+
Comment on attachment 8861759 [details]
Bug 1355348 - Convert stylo assert in nsComputedDOMStyle::DoGetStyleContextNoFlush to a warning;

https://reviewboard.mozilla.org/r/133760/#review136638

::: commit-message-3370a:3
(Diff revision 1)
> +This allows us to run SMIL tests that include a combination of additive and
> +non-additive animation.

Yay!
Attachment #8861759 - Flags: review?(hikezoe) → review+
Comment on attachment 8861754 [details]
Bug 1355348 - Add SMIL restyles in the stylo pretraverse;

https://reviewboard.mozilla.org/r/133750/#review136642

::: dom/smil/nsSMILAnimationController.cpp:736
(Diff revision 1)
> +  return PreTraverseInSubtree(nullptr);
> +}
> +
> +bool
> +nsSMILAnimationController::PreTraverseInSubtree(Element* aRoot)
> +{

Let's assert we're on the main thread, like EffectCompositor::PreTraverseInSubtree does.
Attachment #8861754 - Flags: review?(cam) → review+
Comment on attachment 8861758 [details]
Bug 1355348 - Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue;

https://reviewboard.mozilla.org/r/133758/#review136646

::: dom/smil/nsSMILCSSProperty.cpp:70
(Diff revision 1)
> +    MOZ_ASSERT(mBaseStyleContext->StyleSource().IsServoComputedValues(),
> +               "Base style context should have servo style source");
> +    const ServoComputedValues* currentStyle =
> +      mBaseStyleContext->StyleSource().AsServoComputedValues();

The AsServoComputedValues() call will assert, so I think we can just skip the assertion here, unless you think it adds more context for the failure.

::: dom/smil/nsSMILCSSProperty.cpp:74
(Diff revision 1)
> +    computedValue.mServo =
> +      Servo_ComputedValues_ExtractAnimationValue(currentStyle, mPropID)
> +      .Consume();

Do we need to check whether this return null, and return baseValue if so?

::: dom/smil/nsSMILCSSValueType.cpp:51
(Diff revision 1)
>  {
> -  static const StyleAnimationValue
> +  static const AnimationValue sZeroCoord(

Maybe add a comment in here saying that we just initialize these with StyleAnimationValues, since we don't support additive animation in stylo yet?

::: dom/smil/nsSMILCSSValueType.cpp:469
(Diff revision 1)
> +  // Get a suitable style context for Servo
> +  const ServoComputedValues* currentStyle =
> +    aStyleContext->StyleSource().AsServoComputedValues();
> +  const ServoComputedValues* parentStyle =
> +    aStyleContext->GetParentAllowServo()
> +    ? aStyleContext->GetParentAllowServo()->StyleSource()
> +      .AsServoComputedValues()
> +    : nullptr;
> +  const ServoComputedValuesWithParent servoStyles =
> +    { currentStyle, parentStyle };

GetParentAllowServo() will go away at some point.  If there is a bug on file for it, please mention it here.

::: dom/smil/nsSMILCSSValueType.cpp:512
(Diff revision 1)
> +  Keyframe keyframe;
> +  keyframe.mPropertyValues.AppendElement(Move(propValuePair));
> +  AutoTArray<Keyframe, 1> keyframes;
> +  keyframes.AppendElement(Move(keyframe));

What about:

  AutoTArray<Keyframe, 1> keyframes;
  keyframes.AppendElement()->mPropertyValues.AppendElement(...);

to avoid copying the Keyframe.

::: dom/smil/nsSMILCSSValueType.cpp:631
(Diff revision 1)
>  bool
>  nsSMILCSSValueType::ValueToString(const nsSMILValue& aValue,
>                                    nsAString& aString)
>  {
>    MOZ_ASSERT(aValue.mType == &nsSMILCSSValueType::sSingleton,
>               "Unexpected SMIL value type");
>    const ValueWrapper* wrapper = ExtractValueWrapper(aValue);
> -  return !wrapper ||
> -    StyleAnimationValue::UncomputeValue(wrapper->mPropID,
> -                                        wrapper->mCSSValue, aString);
> +  if (!wrapper) {
> +    return true;
> +  }
> +
> +  wrapper->mCSSValue.SerializeSpecifiedValue(wrapper->mPropID, aString);
> +  return true;
>  }

Make this return void since it always returns true now?
Attachment #8861758 - Flags: review?(cam) → review+
Comment on attachment 8861760 [details]
Bug 1355348 - Don't return early when sampling SMIL animations with Servo backend;

https://reviewboard.mozilla.org/r/133762/#review136652
Attachment #8861760 - Flags: review?(cam) → review+
Comment on attachment 8861747 [details]
Bug 1355348 - Drop extra eRestyle_StyleAttribute hint from Element::SetSMILOverrideStyleDeclaration;

https://reviewboard.mozilla.org/r/133736/#review136660
Attachment #8861747 - Flags: review?(dbaron) → review+
Comment on attachment 8861751 [details]
Bug 1355348 - Make Servo_NoteExplicitHints allow multiple animation restyle hints, including SMIL;

https://reviewboard.mozilla.org/r/133744/#review136620

> I think RestyleHint::for_animations().contains(restyle_hint) should work.

Yeah. We still want to test the negative version, though, i.e.

  RestyleHint::for_animations().contains(restyle_hint) ||
  !RestyleHint::for_animations().intersects(restyle_hint)
Comment on attachment 8861750 [details]
Bug 1355348 - Add restyle hint for SMIL animations;

https://reviewboard.mozilla.org/r/133742/#review137070

::: servo/components/style/matching.rs:1082
(Diff revision 1)
> +                if hint.contains(RESTYLE_SMIL) {
> +                    replace_rule_node(CascadeLevel::SMILOverride,
> +                                      self.get_smil_override(),
> +                                      primary_rules);
> +                }
> +

Oh really, I thought the opposite :)

My reading of this function is:

 1. Do SMIL replacements
 2. Define helper function for dealing with CSS Transitions and CSS Animations
 3. Do CSS Transition replacements
 4. Do CSS Animation replacements

Since (2) is tightly coupled with (3) and (4) they should stick together. Sticking SMIL inbetween seems odd to me?
Comment on attachment 8861758 [details]
Bug 1355348 - Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue;

https://reviewboard.mozilla.org/r/133758/#review137090

::: dom/smil/nsSMILCSSValueType.cpp:51
(Diff revision 1)
>  {
> -  static const StyleAnimationValue
> +  static const AnimationValue sZeroCoord(

This is a Gecko-specific method since it takes a StyleAnimationValue::Unit parameter. I went to add a comment but in light of that I think it makes it more confusing.
Comment on attachment 8861750 [details]
Bug 1355348 - Add restyle hint for SMIL animations;

https://reviewboard.mozilla.org/r/133742/#review137070

> Oh really, I thought the opposite :)
> 
> My reading of this function is:
> 
>  1. Do SMIL replacements
>  2. Define helper function for dealing with CSS Transitions and CSS Animations
>  3. Do CSS Transition replacements
>  4. Do CSS Animation replacements
> 
> Since (2) is tightly coupled with (3) and (4) they should stick together. Sticking SMIL inbetween seems odd to me?

I can see it from both angles. :-)  Anyway, fine to leave it as is, thanks.
Attachment #8861747 - Attachment is obsolete: true
Attachment #8861748 - Attachment is obsolete: true
Attachment #8861749 - Attachment is obsolete: true
Attachment #8861750 - Attachment is obsolete: true
Attachment #8861751 - Attachment is obsolete: true
Attached file Servo PR #16625
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc594f991dc2
Add FFI for getting SMIL override styles; r=heycam
https://hg.mozilla.org/integration/autoland/rev/5881280a872d
Move IsFlattenedTreeDescendantOf helper to nsContentUtils; r=heycam
https://hg.mozilla.org/integration/autoland/rev/f4de7213d853
Add SMIL restyles in the stylo pretraverse; r=heycam
https://hg.mozilla.org/integration/autoland/rev/a1e5a034eefb
Move ServoComputedValuesWithParent to separate file; r=hiro
https://hg.mozilla.org/integration/autoland/rev/7dbc4fb20502
Add constructor to AnimationValue; r=hiro
https://hg.mozilla.org/integration/autoland/rev/e5f624959d3d
Factor out method to remove minus sign from property values; r=hiro
https://hg.mozilla.org/integration/autoland/rev/d3553e38c07d
Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue; r=heycam
https://hg.mozilla.org/integration/autoland/rev/f1bd37850558
Convert stylo assert in nsComputedDOMStyle::DoGetStyleContextNoFlush to a warning; r=hiro
https://hg.mozilla.org/integration/autoland/rev/33f064e9137d
Don't return early when sampling SMIL animations with Servo backend; r=heycam
This is the least risky patch I can think of to fix the hazard bustage. I would push this to autoland but it is currently closed.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/32598e58ad1f
Fix hazard bustage by duplicating code in Gecko_GetStyleAttrDeclarationBlock and Gecko_GetSMILOverrideDeclarationBlock on a CLOSED TREE; r=me a=bustage-fix
Depends on: 1363880
Blocks: 1363880
No longer depends on: 1363880
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8900bc3ec0
Drop extra eRestyle_StyleAttribute hint from Element::SetSMILOverrideStyleDeclaration; r=dbaron
I think when I split the patch series into Servo patches and Gecko patches this first patch got left out somehow.
You need to log in before you can comment on or make changes to this bug.