1390384 - stylo: test_smilKeyTimes.xhtml fails due to failing to fall back to calcMode=discrete

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

test_smilKeyTimes.xhtml fails with the following errors:

TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilKeyTimes.xhtml | Test case [non-interpolatable fallback] (keyTimes: '0;.2;1' calcMode: linear), t=1.9: Unexpected sample value: - got "round", expected "butt"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilKeyTimes.xhtml | Test case [non-interpolatable fallback] (keyTimes: '0;.2;1' calcMode: linear), t=9.9: Unexpected sample value: - got "square", expected "round"

From my initial analysis, in nsSMILAnimationFunction::InterpolateResult we should get an error result when we call `from->Interpolate(*to, intervalProgress, aResult)` (i.e. nsSMILCSSValueType::Interpolate) and that will make us fall back to calcMode=discrete which has special behavior when keyTimes are used.

However, in add_weighted for `AnimationValue` we do the discrete interpolation and then return Ok(). Perhaps in SMIL-land we need to call Servo_Property_IsDiscreteAnimatable is return an error in that case?

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

We can easily fix this on the SMIL side, however I wonder if Servo_AnimationValues_IsInterpolable and Servo_AnimationValues_Interpolate should actually return false / fail for discrete properties? The only trouble is that in order to do that we'd either need to:

a) Add an extra method to AnimationValue
b) Pass an extra property parameter to these methods

(a) seems bad from a code size point of view. (b) seems a bit messy and unnecessary for transitions (which check for discretely animatable properties earlier on).

For now I'm just going to fix this on the SMIL side.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1390384 - Apply SMIL's calcMode=discrete handling to discretely animatable properties in Stylo;

In SMIL if a discrete calcMode is used, the keyTimes attribute may be used to
specify the times at which the various animation values are used, overriding the
regular 50% flip behavior (see nsSMILAnimationFunction::ScaleSimpleProgress).

However, this behavior is only applied when the calcMode is known to be
discrete. If the author specifies calcMode="linear" but
attributeType="stroke-linecap" then SMIL should fall back to discrete calcMode
including applying the special keyTimes behavior.

For the Servo backend, Servo_AnimationValues_Interpolate does not return an
error for discretely animated properties so SMIL does not recognize when we fall
back to discrete calcMode. This patch adds a check before performing
interpolation that tests if the property is a discretely interpolable one and,
if it is, returns an error so that we run SMIL's special discrete calcMode
handling.

Review commit: https://reviewboard.mozilla.org/r/168600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168600/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897298 [details]
Bug 1390384 - Apply SMIL's calcMode=discrete handling to discretely animatable properties in Stylo;

https://reviewboard.mozilla.org/r/168600/#review173834

::: dom/smil/nsSMILCSSValueType.cpp:565
(Diff revision 1)
> +    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty,
> +                                         CSSEnabledState::eForAllContent) {
> +      // If the shorthand has one or more non-discrete components, it should not
> +      // be treated as discrete.
> +      if (!Servo_Property_IsDiscreteAnimatable(*p)) {
> +        result = false;

Nit: This should be resultAccordingToServo?

::: dom/smil/test/mochitest.ini
(Diff revision 1)
>  [test_smilGetStartTime.xhtml]
>  [test_smilHyperlinking.xhtml]
>  [test_smilInvalidValues.html]
>  [test_smilKeySplines.xhtml]
>  [test_smilKeyTimes.xhtml]
> -skip-if = stylo

Yay!

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> ::: dom/smil/nsSMILCSSValueType.cpp:565
> (Diff revision 1)
> > +    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty,
> > +                                         CSSEnabledState::eForAllContent) {
> > +      // If the shorthand has one or more non-discrete components, it should not
> > +      // be treated as discrete.
> > +      if (!Servo_Property_IsDiscreteAnimatable(*p)) {
> > +        result = false;
> 
> Nit: This should be resultAccordingToServo?

Oh, good catch!

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

After making the review change and running a few more tests I discovered that 'text-decoration' is not discrete (since the underline color can be interpolated) but 'marker' is. I'm less sure about the special shorthand handling after all.

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

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8897298 [details]
Bug 1390384 - Apply SMIL's calcMode=discrete handling to discretely animatable properties in Stylo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168600/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6116b4a41649
Apply SMIL's calcMode=discrete handling to discretely animatable properties in Stylo; r=hiro

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6116b4a41649