1369614 - stylo: Support animating between stroke-widths that have units and those that don't

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

In bug 1367327 we switched the type of the computed value for stroke-width to LengthOrPercentageOrNumber which is just a typedef for 'Either<Number, LengthOrPercentage>'. However, when we come to interpolate animation values, the definition for Either types is:

  impl<T, U> Animatable for Either<T, U>
          where T: Animatable + Copy, U: Animatable + Copy,
  {
      #[inline]
      fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Result<Self, ()> {
          match (*self, *other) {
              (Either::First(ref this), Either::First(ref other)) => {
                  this.add_weighted(&other, self_portion, other_portion).map(Either::First)
              },
              (Either::Second(ref this), Either::Second(ref other)) => {
                  this.add_weighted(&other, self_portion, other_portion).map(Either::Second)
              },
              _ => {
                  let result = if self_portion > other_portion {*self} else {*other};
                  Ok(result)
              }
          }
      }

i.e. we fall back to discrete interpolation if the two arguments don't match subtype.

That means we won't smoothly interpolate between '24px' and '36'. It also means we won't interpolate between '0' and '24px' making it hard to choose a suitable zero value.

The reason we made this change is because we prefer the greater precision of a number.

In Gecko we normalize to numbers when creating the animation values:

  http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/layout/style/StyleAnimationValue.cpp#4637

However, in Servo we mostly re-use computed values as animation values so we can't do that so easily. I think we need to introduce a new type for the computed value (or just the animated value?), rather than using Either so that we can implement the appropriate interpolation.

Interestingly, we had this exact same bug when we first implemented this in Gecko: bug 556441.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

FWIW Either will be dropped sooner or later.

http://logs.glob.uno/?c=mozilla%23servo&s=1+Jun+2017&e=1+Jun+2017&h=either#c686397

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The rounding issue which has been fixed by bug 1367327 did not happen during parsing, it happens when converting specified Length value to computed value Length(Au).  It's exactly the same issue what Boris found in bug 1361663 comment 1.

Comment 3

[Brian Birtles (:birtles)]

This is responsible for one of the failures in dom/smil/test/test_smilCSSFromTo.xhtml.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

I discussed with Hiro,

Maybe, We only need to change the stroke-width type to LengthOrPercentage. Length is factor, so we can treat the number.

We need to change following properties as well:
 - stroke-width (comment #0)
 - stroke-dashoffset
 - stroke-dasharray

And this type use in the 'transform' property's value[1], so we can change this code as well.
[1] http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/servo/components/style/properties/longhand/box.mako.rs#1600


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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Hmm, that is not what I meant, I meant we need the conversion in ToAnimatedValue.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Thanks hiro,
I rewrite the patch with ToAnimatedValue.

This patch fix 'stroke-dashoffset' and 'stroke-dasharray' properties as well.
The 'stroke-dasharray' is defined as a vector, So we need to add animated value for this.

WIP: 
https://hg.mozilla.org/try/rev/ad4a851944b2f1d5c4f75c10f200c074fed8d7ea
https://hg.mozilla.org/try/rev/bca2037bd976a8deb4cb3a05cffc2018e44e8fa3

This patch will occur difference behavior of serializing property. In Gecko, the result of serializing don't contain unit when we specify stroke-width with unitless(i.e. stroke-shadow: 3).  But this way contain the unit always (e.g. we specify the stroke-shadow: 3, the serialize result is '3px' always) because we convert the number value to length value.

However spec explain that 'For SVG-specific properties, the length unit identifier is optional.'[1], and another UA contains the unit always(except IE/Edge).
[1] https://www.w3.org/TR/SVG/types.html#DataTypeLength

I think that we need fix the gecko and current test data.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Thanks hiro,
> I rewrite the patch with ToAnimatedValue.
> 
> This patch fix 'stroke-dashoffset' and 'stroke-dasharray' properties as well.
> The 'stroke-dasharray' is defined as a vector, So we need to add animated
> value for this.
> 
> WIP: 
> https://hg.mozilla.org/try/rev/ad4a851944b2f1d5c4f75c10f200c074fed8d7ea
> https://hg.mozilla.org/try/rev/bca2037bd976a8deb4cb3a05cffc2018e44e8fa3
> 
> This patch will occur difference behavior of serializing property. In Gecko,
> the result of serializing don't contain unit when we specify stroke-width
> with unitless(i.e. stroke-shadow: 3).  But this way contain the unit always
> (e.g. we specify the stroke-shadow: 3, the serialize result is '3px' always)
> because we convert the number value to length value.
> 
> However spec explain that 'For SVG-specific properties, the length unit
> identifier is optional.'[1], and another UA contains the unit always(except
> IE/Edge).
> [1] https://www.w3.org/TR/SVG/types.html#DataTypeLength
> 
> I think that we need fix the gecko and current test data.

It is problem of serialization of computed value. If we animated this property, we will lost the number type information in previous patch. (i.e. LengthOrPercentageOrNumber[Specified] -> LengthOrPercentage[Animated] -> LengthOrPercentageOrNumber[Computed])


Umm, We had better to use the 'Percentage or Number' as animation value. Gecko will convert length value to float value, as mentioned by Brian on comment #0.
If we use PercentageOrNumber, animated value will be unitless always, but this behavior is same to gecko.

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #7)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> > Thanks hiro,
> > I rewrite the patch with ToAnimatedValue.
> > 
> > This patch fix 'stroke-dashoffset' and 'stroke-dasharray' properties as well.
> > The 'stroke-dasharray' is defined as a vector, So we need to add animated
> > value for this.
> > 
> > WIP: 
> > https://hg.mozilla.org/try/rev/ad4a851944b2f1d5c4f75c10f200c074fed8d7ea
> > https://hg.mozilla.org/try/rev/bca2037bd976a8deb4cb3a05cffc2018e44e8fa3
> > 
> > This patch will occur difference behavior of serializing property. In Gecko,
> > the result of serializing don't contain unit when we specify stroke-width
> > with unitless(i.e. stroke-shadow: 3).  But this way contain the unit always
> > (e.g. we specify the stroke-shadow: 3, the serialize result is '3px' always)
> > because we convert the number value to length value.
> > 
> > However spec explain that 'For SVG-specific properties, the length unit
> > identifier is optional.'[1], and another UA contains the unit always(except
> > IE/Edge).
> > [1] https://www.w3.org/TR/SVG/types.html#DataTypeLength
> > 
> > I think that we need fix the gecko and current test data.
> 
> It is problem of serialization of computed value. If we animated this
> property, we will lost the number type information in previous patch. (i.e.
> LengthOrPercentageOrNumber[Specified] -> LengthOrPercentage[Animated] ->
> LengthOrPercentageOrNumber[Computed])
> 
> 
> Umm, We had better to use the 'Percentage or Number' as animation value.
> Gecko will convert length value to float value, as mentioned by Brian on
> comment #0.
> If we use PercentageOrNumber, animated value will be unitless always, but
> this behavior is same to gecko.

This is my misunderstanding. We need to use LengthOrPercentage.

Currently, gecko will convert length to float when property has 'CSS_PROPERTY_NUMBERS_ARE_PIXELS' flag[1]. So we will lost the unit information if we animate with unit on these properties.

I investigate other UAs using with test case[2], Other UAs contains unit information even if animate these properties. (For interesting, webkit add the unit to specified value.)

I discussed with Brian, we need to add the unit when serializing the value even if animating these properties, so in this bug, we will fix the stylo side, and we will fix the gecko side on bug 1379908.


[1] http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/layout/style/StyleAnimationValue.cpp#4635
[2] https://jsfiddle.net/qto994s8/4/

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

The other problems of failing the wpt.

1) Interpolate different length of stroke-dasharray:

 If we specify the different length of stroke-dasharray(e.g. strokeDasharray['1, 2, 3', '1, 2, 3, 4']), the result of gecko is '1, 2, 3, 2.5, 1.5, 2.5, 2, 3, 2, 1.5, 2.5, 3.5', if the current time is half of duration. Gecko implementation is here [1]. We need to implement same interpolating of array.

[1] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/style/StyleAnimationValue.cpp#3154

2) Interpolate stroke-dasharray with 'none' keyword.
 Gecko will treat as discrete when specified the 'none'. Gecko will treat 'none' value as css value with eCSSUnit_None, but stylo will treat as empty vector. I think we need to return Err(()) when interpolating this vector if 'to' or 'from' value is empty.

I will modify these problem on this bug

WIP: https://hg.mozilla.org/try/rev/09adb6918d5b2edf8fd96853de4edfa444b5734d

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Servo PR, #17704

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

I discussed with Hiro:

9:01 AM <hiro> mantaroh: Did you confirm the behavior  of stroke-dasharray on other browsers?
9:01 AM <mantaroh> hiro: What kind of behavior? I tried basic case only.
9:01 AM <hiro> mantaroh: Also which test cases in wpt fail without the fix? You might need to leave a comment about the test case in the PR.
9:01 AM <hiro> mantaroh: interpolation from 'none' to others.
9:07 AM <mantaroh> hiro: OK, I'll update the comment. and 
9:09 AM <mantaroh> hiro: I checked this behavior gecko only.
9:09 AM <hiro> mantaroh: then we should check other browsers.
9:10 AM <mantaroh> hiro: Ok, I made the test case, http://output.jsbin.com/ximizof . The other UA doesn't discrete...
9:10 AM <mantaroh> hiro: sorry, second patch is wrong.
9:11 AM <hiro> mantaroh: yeah, that's what I guessed.

After this discussion, I investigated for this problem, I think that it is same to bug 1377053. This discrete behavior is expected result.
So I think that it is enough to using RepeatableListAnimatable.

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Umm,, But we get the different result of this discrete when we use the SMIL(i.e. use the <animate> or <set> element) on other browsers.

The example is here: https://jsfiddle.net/m2Ljyd0b/

Gecko: Same to CSS Animation result

  0px, 0px -> 10px, 20px at ~50%: 5, 10
  0px -> 10px, 20px at ~50%: 5, 10
  (none) -> 10px, 20px at ~50%: 10, 20
  'none' -> 10px, 20px at ~50%: 10, 20
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Blink: Several case doesn't discrete. (Same to Webkit)

  0px, 0px -> 10px, 20px at ~50%: 5px, 10px
  0px -> 10px, 20px at ~50%: 10px, 20px       <- here
  (none) -> 10px, 20px at ~50%: 5px, 10px     <- here
  'none' -> 10px, 20px at ~50%: 5px, 10px     <- here
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Edge: Doesn't support SMIL.

Stylo: Same to CSS Animation.

  0px, 0px -> 10px, 20px at ~50%: 5px, 10px
  0px -> 10px, 20px at ~50%: 5px, 10px
  (none) -> 10px, 20px at ~50%: 10px, 20px
  'none' -> 10px, 20px at ~50%: 10px, 20px
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Webkit:  Several case doesn't discrete. (Same to Blink)

  0px, 0px -> 10px, 20px at ~50%: 5px, 10px
  0px -> 10px, 20px at ~50%: 10px, 20px     <- Here
  (none) -> 10px, 20px at ~50%: 5px, 10px   <- Here
  'none' -> 10px, 20px at ~50%: 5px, 10px   <- Here
  10px, 20px -> (none) at ~50%: none
  10px, 20px -> 'none' at ~50%: none
  'none' -> 'none' at ~50%: none
  (none) -> 'none' at ~50%: none

Comment 13

[Brian Birtles (:birtles)]

I wouldn't worry about matching WebKit/Blink's SMIL behavior on this point. From my understanding their SMIL implementation is independent from their CSS implementation.

As I understand it, with regards to none interpolation from stroke-dasharray, currently we have:

* Stylo CSS animation == Blink CSS animation
* Stylo CSS animation =~ Gecko CSS animation with the exception of units being dropped in Gecko
* Stylo SMIL animation =~ Gecko SMIL animation with the exception of units being dropped in Gecko?
* Style CSS animation == Stylo SMIL animation

That seems pretty good to me.

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

nox's comment of PR:

>> Do you mean that we had better to introduce new value type?
> 
> Yes. My point is that maybe not all values which accept <length-percentage> | <number> animate with numbers interpreted
> as pixel lengths, so maybe we shouldn't implement ToAnimatedValue this way on this general-purpose type.

This patch will introduce SVGLengthOrPercentageOrNumber value. This value treat Length as Number in order to make animatable between stroke-* that have units and hose that don't.

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

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #14)
> nox's comment of PR:
> 
> >> Do you mean that we had better to introduce new value type?
> > 
> > Yes. My point is that maybe not all values which accept <length-percentage> | <number> animate with numbers interpreted
> > as pixel lengths, so maybe we shouldn't implement ToAnimatedValue this way on this general-purpose type.
> 
> This patch will introduce SVGLengthOrPercentageOrNumber value. This value
> treat Length as Number in order to make animatable between stroke-* that
> have units and hose that don't.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2011cafd5406c124925566e4581e1821149370b4

We expect that the current reftest of interpolating stroke-width with percentage will fail. But these test was pass on try server and my Linux environment. Even if I changed reference file to 'about:blank', test was pass.
macOS and Windows is expected result.

Rebased and retry it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0ec0eabf708d587341462067ff6aba06dc79fbb

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> Rebased and retry it:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b0ec0eabf708d587341462067ff6aba06dc79fbb

I pushed to try with wrong try-syntax. Correct is following :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2810897a4b40a667c1e665e7973f48e937297b

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #16)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> > Rebased and retry it:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=b0ec0eabf708d587341462067ff6aba06dc79fbb
> 
> I pushed to try with wrong try-syntax. Correct is following :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2f2810897a4b40a667c1e665e7973f48e937297b

I can reproduce this fail with reftest.compareStyloToGecko=true. This failure reason is related with bug 1344132 comment 22. I need add the !styloVsGecko condition to reftest.list in this case.

Previous this patch, following condition is true:
 1) gecko and stylo result is different
 2) stylo result is different from reftest reference file(anim-css-strokewidth-1-ref.svg in this case).

But after this patch, first condition will be false(i.e. gecko and stylo result will be same). As result, this reftests are failed like try server.

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

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

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1369614 - Update expectations for stroke-dasharray test.

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

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1369614 - Add stroke length type wpt.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888727 [details]
Bug 1369614 - Add stroke length type wpt.

https://reviewboard.mozilla.org/r/159752/#review165108

Could you please why we need these test cases part from positiveNumberType? Looks the same to me.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> Comment on attachment 8888727 [details]
> Bug 1369614 - Add stroke length type wpt.
> 
> https://reviewboard.mozilla.org/r/159752/#review165108
> 
> Could you please why we need these test cases part from positiveNumberType?
> Looks the same to me.

Could you please *explain*

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888726 [details]
Bug 1369614 - Update expectations for stroke-dasharray test.

https://reviewboard.mozilla.org/r/156738/#review165110

::: commit-message-3ba0e:1
(Diff revision 1)
> +Bug 1369614 - Remove unncessary test fail annotation of stroke-dasharray's wpt. r?hiro

'unnecessary' is bit...  It's been necessary actually.
I think just 'Update expectations for stroke-dasharray test' or something is reasonable.

Also, I am wondering we don't need a test case for interpolation for this bug?

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review165116

::: commit-message-7d2e8:1
(Diff revision 1)
> +Bug 1369614 - Add styloVsGecko condition to reftest of stroke-width which comparing with percentage. r?hiro

This commit message seems wrong. We do comparision?  I think we just interpolate or add?

Also I don't quite understand why styloVsGecko succeeds.  How do the results look like? What is the wrong in styloVsStylo cases?  We can't interpolate between percentage and px values on stylo?  If so why we don't fix the issue in this bug altogether?

Comment 25

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review165116

Thanks, Hiro!

> This commit message seems wrong. We do comparision?  I think we just interpolate or add?
> 
> Also I don't quite understand why styloVsGecko succeeds.  How do the results look like? What is the wrong in styloVsStylo cases?  We can't interpolate between percentage and px values on stylo?  If so why we don't fix the issue in this bug altogether?

Yes, current stylo can't interpolate between percentage and px values at all. So test result passed two condition(i.e. 1: different from reference file, 2: different from gecko's result. Note test annotation is 'fails', so test passed).

If apply the PR 17704 patch, stylo will interpolate as discrete since stylo use the NumberOrPercentage as computed value(i.e. stylo treat number as length value). It is same to gecko behavior, and test result was not pass the two condition.(1: To different from reference file is OK, 2: To different from gecko's result is NG). So test will fail.

I think it is same problem of bug 1344132 comment 22.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #25)
> Comment on attachment 8888725 [details]
> Bug 1369614 - Add styloVsGecko condition to reftest of stroke-width which
> comparing with percentage.
> 
> https://reviewboard.mozilla.org/r/156736/#review165116
> 
> Thanks, Hiro!
> 
> > This commit message seems wrong. We do comparision?  I think we just interpolate or add?
> > 
> > Also I don't quite understand why styloVsGecko succeeds.  How do the results look like? What is the wrong in styloVsStylo cases?  We can't interpolate between percentage and px values on stylo?  If so why we don't fix the issue in this bug altogether?
> 
> Yes, current stylo can't interpolate between percentage and px values at
> all. So test result passed two condition(i.e. 1: different from reference
> file, 2: different from gecko's result. Note test annotation is 'fails', so
> test passed).
> 
> If apply the PR 17704 patch, stylo will interpolate as discrete since stylo
> use the NumberOrPercentage as computed value(i.e. stylo treat number as
> length value). It is same to gecko behavior, and test result was not pass
> the two condition.(1: To different from reference file is OK, 2: To
> different from gecko's result is NG). So test will fail.

So, with the servo PR, the reftest results are not yet what they should be, right?  I am wondering why we don't fix the issue altogether, and if there are reasons (I assume there are) you should leave the reasons in the commit message.

Comment 27

[Mantaroh Yoshinaga[:mantaroh]]

Thanks.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #25)
> > Comment on attachment 8888725 [details]
> > Bug 1369614 - Add styloVsGecko condition to reftest of stroke-width which
> > comparing with percentage.
> > 
> > https://reviewboard.mozilla.org/r/156736/#review165116
> > 
> > Thanks, Hiro!
> > 
> > > This commit message seems wrong. We do comparision?  I think we just interpolate or add?
> > > 
> > > Also I don't quite understand why styloVsGecko succeeds.  How do the results look like? What is the wrong in styloVsStylo cases?  We can't interpolate between percentage and px values on stylo?  If so why we don't fix the issue in this bug altogether?
> > 
> > Yes, current stylo can't interpolate between percentage and px values at
> > all. So test result passed two condition(i.e. 1: different from reference
> > file, 2: different from gecko's result. Note test annotation is 'fails', so
> > test passed).
> > 
> > If apply the PR 17704 patch, stylo will interpolate as discrete since stylo
> > use the NumberOrPercentage as computed value(i.e. stylo treat number as
> > length value). It is same to gecko behavior, and test result was not pass
> > the two condition.(1: To different from reference file is OK, 2: To
> > different from gecko's result is NG). So test will fail.
> 
> So, with the servo PR, the reftest results are not yet what they should be,
> right?  I am wondering why we don't fix the issue altogether, and if there
> are reasons (I assume there are) you should leave the reasons in the commit
> message.

Umm, we can interpolate with this test internally current stylo. But rendering result was not interpolate. We pass the CalcLength to mStrokeWidth via set_stroke_width() after converting ClaclLength to nsStyleCoord_CalcValue.

We expected that the unit of mStrokeWidth is Factor or Coord or Percent, we didn't allow the Calc[1].
[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/svg/SVGContentUtils.cpp#834-835

Even if we add the CalcLength, I think we can't interpolate without modification to gecko. The stroke-dashoffset / stroke-dash is same as well.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

So once the gecko side is fixed (bug 594933?) the interpolation between px and percentage works fine on both of gecko and stylo, right?  So what should we do here in this bug?  We don't need to care about the calc at all?

Also what about the servo case?  I know servo does not support stroke-width yet, but when it supported, it seems to me that the interpolation will work fine without big changes since current code (LengthOrPercentage) is supposed be work with calc, no?

Comment 29

[Mantaroh Yoshinaga[:mantaroh]]

Thanks.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> So once the gecko side is fixed (bug 594933?) the interpolation between px
> and percentage works fine on both of gecko and stylo, right?  So what should
> we do here in this bug?  We don't need to care about the calc at all?
> 

It seems like to fix line-height only. I'm not sure but maybe stroke-* is bug 1258270?
If gecko work this behavior, I think reftest will fail. I tried test-case in gecko, the result is fail (i.e. gecko didn't interpolate between pct and px).

In gecko, I think that pct-px interpolation is failed since stroke-* properties has not 'CSS_PROPERTY_STORES_CALC' flag. Gecko used this flag when calling AddWeighted()[1]. So we will treat these units as discrete type.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/style/StyleAnimationValue.cpp#73

I think that I need to update gecko side(comment #27) in order to support interpolation between percentage and other types on stylo.

> Also what about the servo case?  I know servo does not support stroke-width
> yet, but when it supported, it seems to me that the interpolation will work
> fine without big changes since current code (LengthOrPercentage) is supposed
> be work with calc, no?

If we use LengthOrPercentage only, We can't specify the unit less value. For detail see bug 1367327.
So I think that I had better to add the CalcLenght to value of PR 17704 (i.e. CalcLenghtOrNumberOrPercentage?).

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #29)
> Thanks.
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> > So once the gecko side is fixed (bug 594933?) the interpolation between px
> > and percentage works fine on both of gecko and stylo, right?  So what should
> > we do here in this bug?  We don't need to care about the calc at all?
> > 
> 
> It seems like to fix line-height only. I'm not sure but maybe stroke-* is
> bug 1258270?

Oh, I referred the bug number from layout/reftests/svg/smil/style/reftest.list. I did not know the bug has been fixed. So we should update the bug number in the reftest.list.
Also, I am wondering line-height does not have the same problem on stylo?

> If gecko work this behavior, I think reftest will fail. I tried test-case in
> gecko, the result is fail (i.e. gecko didn't interpolate between pct and px).
> 
> In gecko, I think that pct-px interpolation is failed since stroke-*
> properties has not 'CSS_PROPERTY_STORES_CALC' flag. Gecko used this flag
> when calling AddWeighted()[1]. So we will treat these units as discrete type.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 3a3af33f513071ea829debdfbc628caebcdf6996/layout/style/StyleAnimationValue.
> cpp#73
>
> I think that I need to update gecko side(comment #27) in order to support
> interpolation between percentage and other types on stylo.

OK, so if we treated calc case properly in stylo, we just need to modify SVGContentUtils, right?

> > Also what about the servo case?  I know servo does not support stroke-width
> > yet, but when it supported, it seems to me that the interpolation will work
> > fine without big changes since current code (LengthOrPercentage) is supposed
> > be work with calc, no?
> 
> If we use LengthOrPercentage only, We can't specify the unit less value. For
> detail see bug 1367327.
> So I think that I had better to add the CalcLenght to value of PR 17704
> (i.e. CalcLenghtOrNumberOrPercentage?).

I am not confident it yet, I don't know all of things which are underlying this issue.

A question is, if we supported calc between length and unitless length, it will solve this issue?

Comment 31

[Mantaroh Yoshinaga[:mantaroh]]

Thanks, hiro

(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> Also, I am wondering line-height does not have the same problem on stylo?
> 

Stylo has LineHeight value and they convert specified percentage to FontRelativeLenght. So stylo doesn't use Calc.

> OK, so if we treated calc case properly in stylo, we just need to modify
> SVGContentUtils, right?
> 

Yes, I'll modify the SVGContentUtils only.

> I am not confident it yet, I don't know all of things which are underlying
> this issue.
> 
> A question is, if we supported calc between length and unitless length, it
> will solve this issue?

I'm going to store specified length value to NumberOrPercentage when converting to computed value. And CalcLength of computed value used by interpolation only.

[Specified Value] enum { Number, LengthOrPercentage }
[Computed  Value] enum { NumberOrPercentage, CalcLength }

I think it will solve pct-px interpolation problem.

Comment 32

[Mantaroh Yoshinaga[:mantaroh]]

Note to self:
 Current reftest of interpolating pct<->px doesn't specify the 'shape-rendering'. Other tests are fixed by bug 684790. So reftest will fail since rendering result contain the anti-aliasing.

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

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

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Looks like this isn't quite active at the moment. Since this is a P2, I'm going to have bug 1338764 block this instead so that I can unblock other things from there.

I'll leave the animation bits blank there for now.

Comment 35

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34)
> Looks like this isn't quite active at the moment. Since this is a P2, I'm
> going to have bug 1338764 block this instead so that I can unblock other
> things from there.
> 
> I'll leave the animation bits blank there for now.

I am sorry for bothering you during a time. I'm requesting review for this on PR #17704, and I'll contact again to someone who can review it.

Comment 36

[Mantaroh Yoshinaga[:mantaroh]]

Merged Xidorn's commit:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=f345eed7bc95500a386bd5b63a1f09dae325ae9b

As mentioned PR #17704, I separated the bug which interpolating between percentage and other values(bug 1386967).

Comment 37

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #36)
> Merged Xidorn's commit:
>  https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f345eed7bc95500a386bd5b63a1f09dae325ae9b
> 
> As mentioned PR #17704, I separated the bug which interpolating between
> percentage and other values(bug 1386967).

Correct URL:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fbe1a947e56c37aeed43388a6e6b4d9fb6c4a0d

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Thanks for working this, and sorry about any trouble because of that.

Comment 39

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

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

Comment 40

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888726 [details]
Bug 1369614 - Update expectations for stroke-dasharray test.

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

Comment 41

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review165116

> Yes, current stylo can't interpolate between percentage and px values at all. So test result passed two condition(i.e. 1: different from reference file, 2: different from gecko's result. Note test annotation is 'fails', so test passed).
> 
> If apply the PR 17704 patch, stylo will interpolate as discrete since stylo use the NumberOrPercentage as computed value(i.e. stylo treat number as length value). It is same to gecko behavior, and test result was not pass the two condition.(1: To different from reference file is OK, 2: To different from gecko's result is NG). So test will fail.
> 
> I think it is same problem of bug 1344132 comment 22.

I updated the commit message of reason of this change.
Could you confirm this patch again?

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48ed1d927191e6437b761ee6c7f42702e756ff04

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review170658

Thanks for updating the commit message! But still it's confusing..
I still don't quite understand what this commit message wants to say.

::: commit-message-7d2e8:3
(Diff revision 2)
> +Bug 1369614 - Add test fail condition to reftest of interpolating stroke-dasharray between px and percentage. r?hiro
> +
> +We use LengthOrPercentage for interpolating between length and percentage,

'We' is pretty confusing. I guess s/We use/Stylo uses/.
Also, more precisely, stylo currently uses LengthOrPercentage::Calc for interpolation between length and percentage values, right?

::: commit-message-7d2e8:4
(Diff revision 2)
> +Bug 1369614 - Add test fail condition to reftest of interpolating stroke-dasharray between px and percentage. r?hiro
> +
> +We use LengthOrPercentage for interpolating between length and percentage,
> +but gecko will not support the calc value for stroke-dasharray.(bug 1386967)

I think this 'gecko' should be 'we', because neither gecko nor stylo supports calc for stroke-dasharray because of bug 1258270, right? Oops, we shouldn't use 'we' here. We should say 'neither gecko nor stylo' instead.

::: commit-message-7d2e8:7
(Diff revision 2)
> +Bug 1369614 will change this interpolating type as discrete. So result will same
> +to gecko.

I guess you might want to say that 'in this bug we make stroke-dasharray interpolation between px and % as discrete type, as a result, some rendering results match to gecko until we properly support calc() for stroke-dasharray (bug 1386967 for stylo and bug 1258270 for gecko)'?

::: layout/reftests/svg/smil/style/reftest.list:117
(Diff revision 2)
>  == anim-css-strokewidth-1-to-no-no.svg        anim-css-strokewidth-1-ref.svg
>  
>  # 'stroke-width' property, from/by/to with percent values
>  # XXXdholbert the mixed pct + px tests fail right now, because we need calc()
>  # in order to interpolate between pct and non-pct values, and we don't yet
> -# support calc() for stroke-width & other SVG-specific properties (Bug 594933).
> +# support calc() for stroke-width & other SVG-specific properties (Bug 1258270).

We should also leave a bug number for stylo here.

Comment 43

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review170658

Thanks! hiro.

fixed some message, and added the bug number to reftest.list.

> 'We' is pretty confusing. I guess s/We use/Stylo uses/.
> Also, more precisely, stylo currently uses LengthOrPercentage::Calc for interpolation between length and percentage values, right?

Yes, stylo use the LengthOrPercentage::Calc when interpolating this types, and current gecko side(i.e. SVGContentUtils::CoordToFloat) doesn't support calc() value. As result, stylo doesn't interpolate stroke-dasharray in this case.

Comment 44

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

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

Comment 45

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888726 [details]
Bug 1369614 - Update expectations for stroke-dasharray test.

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

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

https://reviewboard.mozilla.org/r/156736/#review170696

::: commit-message-7d2e8:1
(Diff revision 3)
> +Bug 1369614 - Add test fail condition to reftest of interpolating stroke-dasharray between px and percentage. r?hiro

I just realized all mofified reftests are for stroke-width, not for stroke-dasharray, right?

::: commit-message-7d2e8:4
(Diff revision 3)
> +Bug 1369614 - Add test fail condition to reftest of interpolating stroke-dasharray between px and percentage. r?hiro
> +
> +Current stylo uses LengthOrPercentage::Calc for interpolation between length and
> +percentage, but neither gecko nor stylo will not support the calc value for

Drop 'not'.

::: commit-message-7d2e8:13
(Diff revision 3)
> +In current reftest.list, we specified 'fails == <test> <reference>' condition.
> +If we specified this condition, result will fail when rendering result of gecko
> +and stylo is same even if rendering result of <test> is different from
> +<reference> on stylo.
> +So we need to add 'fails-if(!styloVsGecko) == <test> <reference>' condition.

I don't think this paragraph is necessary.

Comment 47

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156736/diff/3-4/

Comment 48

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888726 [details]
Bug 1369614 - Update expectations for stroke-dasharray test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156738/diff/3-4/

Comment 49

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> Comment on attachment 8888725 [details]
> Bug 1369614 - Add test fail condition to reftest of interpolating
> stroke-width between px and percentage.
> 
> https://reviewboard.mozilla.org/r/156736/#review170696
> 

Thank you for reviewing it.

The previous PR rotted after landing bug 1374233.
I think that we might need to add two SVGLengh types to this PR(i.e. SVGLengthOrPercentageOrNumber and NonNegativeSVGLengthOrPercentageOrNumber).
So I will send PR after adding these value types.

Comment 50

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8888725 [details]
Bug 1369614 - Add test fail condition to reftest of interpolating stroke-width between px and percentage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156736/diff/4-5/

Comment 51

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #50)
> Comment on attachment 8888725 [details]
> Bug 1369614 - Add test fail condition to reftest of interpolating
> stroke-width between px and percentage.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/156736/diff/4-5/

Bug 1387939 turned off w-p-t, so I removed this patch from this bug.

Comment 52

[Mantaroh Yoshinaga[:mantaroh]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d092abc50f32a2e082bad409657d1a73b87f13&group_state=expanded

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc40bdb0052e
Add test fail condition to reftest of interpolating stroke-width between px and percentage. r=hiro

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/dc40bdb0052e