Closed Bug 1390367 Opened 7 years ago Closed 7 years ago

stylo: Some SMIL tests fail due to lengths being serialized with px

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

test_smilCSSFromBy_xhtml_result.txt(applied bug 1369614)
13.86 KB, text/plain
Details
Servo PR, #18198
41 bytes, text/x-github-pull-request
Details | Review
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
(I thought we already had a bug on file for this but I can't seem to find it.)

e.g. in dom/smil/test/test_smilCSSFromBy.xhtml I get:

TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking that 'from' value is set at start of animation - got "0px", expected "0"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking value halfway through animation - got "4px", expected "4"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking that 'from' value is set at start of animation - got "0px", expected "0"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking value halfway through animation - got "4px", expected "4"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: [freeze-mode] checking that final value is set at end of animation - got "8px", expected "8"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: [freeze-mode] checking that final value is set after end of animation - got "8px", expected "8"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking that 'from' value is set at start of animation - got "1px", expected "1"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking value halfway through animation - got "6px", expected "6"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking that 'from' value is set at start of animation - got "1px", expected "1"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: checking value halfway through animation - got "6px", expected "6"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: [freeze-mode] checking that final value is set at end of animation - got "11px", expected "11"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | stroke-width: [freeze-mode] checking that final value is set after end of animation - got "11px", expected "11"

I'm pretty sure this shows up in other tests too.

I suspect what we want to do here is:

* Confirm that other browsers serialize with 'px' (I think they do?)
* Assuming they do either:
  a. Fix Gecko
  b. Make the test accept either
  c. Make the test expect 'px' and mark Gecko as failing

For either (b) or (c) we should file a bug blocking bug 1387951 (stylo-behavior-changes). (a) is preferred but (b) would be acceptable if (a) is time-consuming. We should probably avoid dropping test coverage of Gecko as per (c).
Some very large proportion of the failing tests are failing on stroke-width.
(In reply to Brian Birtles (:birtles) from comment #1)
> Some very large proportion of the failing tests are failing on stroke-width.

I tried my local environment(applied bug 1369614), this test passed(i.e. this serialize problem of stroke-width doesn't occur.)
The attachment is result of this test, clip / fill / lighting-color properties are failed, but this is another bug.

So I think that this bug will resolve after landing bug 1369614.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Created attachment 8897276 [details]
> test_smilCSSFromBy_xhtml_result.txt(applied bug 1369614)
> 
> (In reply to Brian Birtles (:birtles) from comment #1)
> > Some very large proportion of the failing tests are failing on stroke-width.
> 
> I tried my local environment(applied bug 1369614), this test passed(i.e.
> this serialize problem of stroke-width doesn't occur.)
> The attachment is result of this test, clip / fill / lighting-color
> properties are failed, but this is another bug.
> 
> So I think that this bug will resolve after landing bug 1369614.

Several tests are passed after landing bug 1369614. However the tests which determine the fill mode(i.e. [freeze-mode]) doesn't.
Those tests result contain the unit information.(Expected result doesn't contain unit.)
https://pastebin.mozilla.org/9030004

I'll looking into this bug.
Assignee: nobody → mantaroh
I had a quick look at this and my guess is we're hitting this code path:

  void
  nsSMILAnimationFunction::ComposeResult

  ...

    } else if (mLastValue) {

      // Sampling last value
      const nsSMILValue& last = values[values.Length() - 1];
      result = last;

    }

    aResult = Move(result)

That is, we just set the "12px" as-is.

Normally, however, in add_weighted for SvgLengthOrPercentageOrNumber we call convert_to_number_or_percentage so that lengths get normalized to numbers.

For the Gecko code path, we normalize lengths to numbers when we create the (wrapped) StyleAnimationValue based on whether the property has the CSS_PROPERTY_NUMBERS_ARE_PIXELS flag or not.

That is, for the Gecko code path, we do the normalizing when we create the animation values but for Servo we do it during interpolation. As a result the Servo code path fails when we skip interpolation.

Does that sound right? If so, it seems like normalizing when creating the values makes more sense. But I guess there are cases where we don't want to normalize?
(In reply to Brian Birtles (:birtles) from comment #4)
> Does that sound right? If so, it seems like normalizing when creating the
> values makes more sense. But I guess there are cases where we don't want to
> normalize?

I guess the important difference is that we have separate animated values for Gecko but not for Servo. Perhaps the best thing here is to introduce a separate animated value type for this case for Servo.
Thanks Brian.

(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > Does that sound right? If so, it seems like normalizing when creating the
> > values makes more sense. But I guess there are cases where we don't want to
> > normalize?
> 
> I guess the important difference is that we have separate animated values
> for Gecko but not for Servo. Perhaps the best thing here is to introduce a
> separate animated value type for this case for Servo.

I added animated value for this value type.

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

Note to self:
PR 18134 will change Animatable trait drastically. So I wait to merget PR 18134. This WIP code contain these PR 18134 changes and other un-landed code which related with this test failure(bug 1387986).
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Thanks Brian.
> 
> (In reply to Brian Birtles (:birtles) from comment #5)
> > (In reply to Brian Birtles (:birtles) from comment #4)
> > > Does that sound right? If so, it seems like normalizing when creating the
> > > values makes more sense. But I guess there are cases where we don't want to
> > > normalize?
> > 
> > I guess the important difference is that we have separate animated values
> > for Gecko but not for Servo. Perhaps the best thing here is to introduce a
> > separate animated value type for this case for Servo.
> 
> I added animated value for this value type.
> 
> WIP:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a4b2e678990d78424a6b261cb8ae212f8d736931
> 
> Note to self:
> PR 18134 will change Animatable trait drastically. So I wait to merget PR
> 18134. This WIP code contain these PR 18134 changes and other un-landed code
> which related with this test failure(bug 1387986).

I pushed with wrong try syntax. Retry:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74210586e08cf78446b59d67214ee803b456d4ff
Mantaroh, any updates on your Servo PR for this bug?
(In reply to Chris Peterson [:cpeterson] from comment #9)
> Mantaroh, any updates on your Servo PR for this bug?

Sorry, It's under the review now.
For now gecko will lost unit information
See Also: → 1379908
I discussed with Brian. First, I decided that we enable dom/smil/test, and then we should fix the following bugs:
 * Bug 1379908 : Gecko should add px unit to the serialized value.
 * this bug : Servo need to use Length type for <length> and <number>. (We should consider accuracy.)

The wrote patch will enable dom/smil/test except for this freeze fill mode tests, these tests result are different from Gecko and Servo for a present.

And I updated the interpolation of SVGPaintKind::None type since the fix of bug 1390364 cleared by the recent changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6563d779e5c05ffbbd29844a08a4e793f6cad8d
> Bug 1390367 - Don't allow interpolating 'fill:none' with 'fill:none'

Can we move this to a separate bug please? It's not really related to this bug.
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review180862

::: commit-message-8e052:1
(Diff revision 1)
> +Bug 1390367 - Enable dom/smil/test on Servo. r?birtles

This patch is doing a lot more than enabling tests. We should update the summary to reflect what the patch does.

::: dom/smil/test/db_smilCSSFromTo.js:111
(Diff revision 1)
>      new AnimTestcaseFromTo("0px", "12px", { fromComp: "0",
>                                              midComp:  "6",
> -                                            toComp:  "12"}),
> +                                            toComp:  isStylo ? "12px" : "12"}),

Rather than changing text expectations piecemeal, can we change the comparison function to allow either px or not for all appropriate properties (e.g. all stroke properties?)

(I'm not sure yet where the most appropriate place for that check would be. Unfortunately these SMIL tests are far too complicated.)

If we do that, we probably don't need to check for stylo or not either.

::: dom/smil/test/test_smilTextZoom.xhtml:67
(Diff revision 1)
>      svg.setCurrentTime(1.5);
>      verifyStyle(text, "font-size",    "30px");
>      verifyStyle(rect, "stroke-width", "30");
>      svg.setCurrentTime(2);
>      verifyStyle(text, "font-size",    "40px");
> -    verifyStyle(rect, "stroke-width", "40");
> +    verifyStyle(rect, "stroke-width", isStylo ? "40px" : "40");

In this case we should just change verifyStyle to allow 'px' or no 'px' (perhaps only for a subset of properties).
Attachment #8904109 - Flags: review?(bbirtles)
Comment on attachment 8904110 [details]
Bug 1390367 - Don't allow interpolating 'fill:none' with 'fill:none'.

https://reviewboard.mozilla.org/r/175878/#review180864

Clearing review for now, just because I think this should be a separate bug.

::: commit-message-b0794:3
(Diff revision 1)
> +Bug 1390364 fixed this type interpolating, but the recently changes[1]
> +uses Animate derive for this type(SVGPaintKind), then Servo can interpolate
> +this types(i.e. fill:none and fill:none).
> +
> +This patch add animation(error) hint to SVGPaintKind::None in order to
> +disallow this interpolation.
> +
> +[1] https://github.com/servo/servo/pull/18239/files

I think it would be useful to reference both the changeset that fixed it, and the changeset that regressed it.
Attachment #8904110 - Flags: review?(bbirtles)
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review180862

Thanks, Brian.

> This patch is doing a lot more than enabling tests. We should update the summary to reflect what the patch does.

OK, I'll update that and change the comparison function.
Comment on attachment 8904110 [details]
Bug 1390367 - Don't allow interpolating 'fill:none' with 'fill:none'.

https://reviewboard.mozilla.org/r/175878/#review180864

I separated this bug. (bug 1396483)

Thanks!
Attachment #8904110 - Attachment is obsolete: true
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review180920

I'll help tweak the comments later but for now I'd like to see how much simpler this becomes without the 'freeze' value and with the simple "strip the px" behavior.

::: dom/smil/test/smilTestUtils.js:117
(Diff revision 2)
> +  // This method return generated array which px value and non-px value
> +  // from specified value.
> +  // For example:
> +  //  Specified '20px', generated ['20px', '20']
> +  //  Specified '20', generated ['20px', '20']
> +  //  Specified '20%', generated ['20%']
> +  // It used by comparation to freeze fill mode tests since difference of Gecko
> +  // and Servo.
> +  getToleranceFreezeValue : function(aExpectedValue) {
> +    if (typeof aExpectedValue !== 'string') {
> +      return [aExpectedValue];
> +    }
> +    var unitlessVal = aExpectedValue.indexOf('px') == -1
> +                      ? aExpectedValue
> +                      : aExpectedValue.substring(0, aExpectedValue.indexOf('px'));
> +
> +    if (isNaN(Number(unitlessVal))) {
> +      return [aExpectedValue];
> +    }
> +    return [ unitlessVal + 'px', unitlessVal];
> +  },

I think we only need to do this for px right? So can we just make this strip 'px' from the passed-in value?

::: dom/smil/test/smilTestUtils.js:423
(Diff revision 2)
>      }
>      return this.buildSeekListAnimated(aAnimAttr, aBaseVal,
>                                        aTimeData, aIsFreeze)
>    },
>  
> -  seekAndTest : function(aSeekList, aTargetElem, aTargetAttr)
> +  seekAndTest : function(aSeekList, aTargetElem, aTargetAttr, aIsFreeze)

As discussed, I think we can drop the aIsFreeze parameter and apply the same treatment to all values.

::: dom/smil/test/smilTestUtils.js:430
(Diff revision 2)
>      var svg = document.getElementById("svg");
>      for (var i in aSeekList) {
>        var entry = aSeekList[i];
>        SMILUtil.getSVGRoot().setCurrentTime(entry[0]);
> +
> +      // TODO : We should remove this workaround code after fixed Bug 1390367

Bug 1390367 is this bug right? Which we are hoping to close? Perhaps just list bug 1379908.

::: dom/smil/test/test_smilTextZoom.xhtml:34
(Diff revision 2)
> +  if ('stroke-width' == aPropertyName) {
> +    var expectedVals = SMILUtil.getToleranceFreezeValue(aExpectedVal);
> +    var result = false;
> +    expectedVals.forEach(function(expected) {
> +      if(computedVal == expected) {
> +        result = true;
> +      }
> +    });
> +    ok(result, "computed value of " + aPropertyName);
> +    return;
> +  }

I think this will become a lot simpler once we just trip 'px' from the expected value.

That is, I'd suggest we make all these tests use 'px' for the expected value (since, long-term, that's what we want the result to be) and then just strip 'px' from both the expected and actual values before comparing here.
Attachment #8904109 - Flags: review?(bbirtles)
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review180920

Thanks, Brian.
I addressed it, and fixed some comments.
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review181126

::: commit-message-8e052:1
(Diff revision 3)
> +Bug 1390367 - Pass SMIL mochitest of stroke-* even if computed value has not px unit. r?birtles

Nit: s/not/no/

::: commit-message-8e052:3
(Diff revision 3)
> +Current Gecko removes px unit from stroke-* properties when creating animation
> +value (bug 1379908). However the behavior of another browser differ from
> +Gecko and Servo.
> +Servo removes px unit from this properties when interpolating this properties as
> +well, however Servo doesn't create an animation value(bug 1390367).
> +
> +So this patch will:
> + * make expected result of stroke-* computed value be px-unit value.
> + * allow computed value without px-unit for present.
> +   (This is until bug 1379908 and bug 1390367 is fixed.)

"Currently Gecko converts lengths in stroke-* properties to numbers when creating animation values and hence the result of animation is always a unitless value for these properties.

Servo, on the hand, converts lengths for these properties to numbers during interpolation and hence sometimes the result of animation is a unitless value, and other times (such as when we skip interpolation) it is not.

Other browsers produce lengths with px units and ultimately we should align with that behavior. As a result, this patch updates various SMIL mochitests to:

* Expect animation values with px unit
* Compare values by stripping px units as a temporary measure until we correctly serialize these values with px (bug 1379908)."


[ Note: as with the previous we should not be mentioning bug  1390367 here. This is bug 1390367. ]

::: dom/smil/test/smilTestUtils.js:123
(Diff revision 3)
> +  // If specified value has not px unit, return specified value.
> +  //
> +  // For example:
> +  //  Specified '20px', generated '20'
> +  //  Specified '20', generated '20'
> +  //  Specified '20%', generated '20%'

We're not stripping % right?

::: dom/smil/test/smilTestUtils.js:124
(Diff revision 3)
> +  getUnitlessLengthValue : function(aExpectedValue) {
> +    if (typeof aExpectedValue !== 'string' ||
> +        aExpectedValue.indexOf('px') == -1) {
> +      return aExpectedValue;
> +    }
> +    var unitlessVal = aExpectedValue.substring(0, aExpectedValue.indexOf('px'));
> +    if (isNaN(Number(unitlessVal))) {
> +      return aExpectedValue;
> +    }
> +    return unitlessVal;
> +  },

I think we can just make this:

  stripPx: str => str.replace(/px\s*$/,''),

And then update the comment to "Strip the 'px' unit from |str| if any."

::: dom/smil/test/smilTestUtils.js:427
(Diff revision 3)
> +      // TODO : This is workaround of bug 1390367 and bug 1379908.
> +      // The computed value of stroke-* properties should have px unit, but
> +      // current Gecko and Servo doesn't keep the px unit during animating it.
> +      // If we fixed both bugs, we need to remove this code.

This is bug 1390367. We only want to mention bug 1379907 here.

::: dom/smil/test/smilTestUtils.js:431
(Diff revision 3)
> +      if (['stroke-width',
> +           'stroke-dasharray',
> +           'stroke-dashoffset'].indexOf(aTargetAttr.attrName) != -1) {

I think you could just do:

  if (['stroke-width', ...].includes(aTargetAttr.attrName)) {

::: dom/smil/test/test_smilTextZoom.xhtml:34
(Diff revision 3)
>  // Helper function
>  function verifyStyle(aNode, aPropertyName, aExpectedVal)
>  {
>    var computedVal = SMILUtil.getComputedStyleSimple(aNode, aPropertyName);
> +
> +  // TODO : This is workaround of bug 1390367 and bug 1379908.

This is bug 1390367. We only want to mention bug 1379907 here.
Attachment #8904109 - Flags: review?(bbirtles)
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review181126

Thanks!

Updated comments and stripping code.

> We're not stripping % right?

Yes, this comment just example which we aren't stripping %. But this is unnecessary, So I removed it.
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review181148

::: commit-message-8e052:7
(Diff revision 4)
> +
> +Currently, Gecko converts lengths in stroke-* properties to numbers when
> +creating animation values and hence the result of animation is always a unitless
> +value for these properties.
> +
> +Servo, on the hand, converts lengths for these properties to numbers during

Sorry, my mistake: "on the other hand"

::: dom/smil/test/smilTestUtils.js:117
(Diff revision 4)
> +  // Return stripped px value from specified value.
> +  //
> +  // For example:
> +  //  Specified '20px', generated '20'
> +  //  Specified '20', generated '20'

(I don't think we really need the example here.)

::: dom/smil/test/smilTestUtils.js:415
(Diff revision 4)
> +      // TODO : This is workaround of bug 1379908.
> +      // The computed value of stroke-* properties should have px unit, but
> +      // current Gecko and Servo doesn't keep the px unit during animating it.
> +      // If we fixed both bugs, we need to remove this code.

Let's make this comment:

"Bug 1379908: The computed value of stroke-* properties should be serialized with px units, but currently Gecko and Servo don't do that when animating these values."

::: dom/smil/test/test_smilTextZoom.xhtml:34
(Diff revision 4)
> +  // TODO : This is workaround of bug 1379908.
> +  // The computed value of stroke-* properties should have px unit, but
> +  // current Gecko and Servo doesn't keep the px unit during animating it.
> +  // If we fixed both bugs, we need to remove this code.

Likewise we should update the comment to something similar to what I suggested above.
Attachment #8904109 - Flags: review?(bbirtles) → review+
Comment on attachment 8904109 [details]
Bug 1390367 - Pass SMIL mochitests of stroke-* even if a computed value has no px unit.

https://reviewboard.mozilla.org/r/175876/#review181148

Thanks!
I updated this patch.

> (I don't think we really need the example here.)

OK, I dropped this.
dom/smil test fails:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b167ffdd3b6a03f2edf04d2ed38793d0dfb9d618

The change of fill:none[1] doesn't merged to m-c yet. So I'll push this patch after change of fill:none merged to m-c.

[1] https://hg.mozilla.org/integration/autoland/rev/cc1dd06b7688
Status: NEW → ASSIGNED
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/09acc539e065
Pass SMIL mochitests of stroke-* even if a computed value has no px unit. r=birtles
https://hg.mozilla.org/mozilla-central/rev/09acc539e065
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: