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

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
2 months ago
9 hours ago

People

(Reporter: birtles, Assigned: mantaroh)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 months ago
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.
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
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.
(Reporter)

Comment 3

2 months ago
This is responsible for one of the failures in dom/smil/test/test_smilCSSFromTo.xhtml.
Priority: -- → P2
(Assignee)

Updated

a month ago
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
(Assignee)

Comment 4

18 days ago
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
Hmm, that is not what I meant, I meant we need the conversion in ToAnimatedValue.
(Assignee)

Comment 6

15 days ago
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.
(Assignee)

Comment 7

14 days ago
(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.
(Assignee)

Comment 8

13 days ago
(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/
(Assignee)

Comment 9

13 days ago
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
(Assignee)

Comment 10

12 days ago
Created attachment 8885955 [details] [review]
Servo PR, #17704
(Assignee)

Comment 11

12 days ago
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.
(Assignee)

Comment 12

11 days ago
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
(Reporter)

Comment 13

11 days ago
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.
Blocks: 1338764
(Assignee)

Comment 14

5 days ago
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
(Assignee)

Comment 15

4 days ago
(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
(Assignee)

Comment 16

4 days ago
(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
(Assignee)

Comment 17

3 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

3 days ago
mozreview-review
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.
Attachment #8888727 - Flags: review?(hikezoe)
(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

3 days ago
mozreview-review
Comment on attachment 8888726 [details]
Bug 1369614 - Remove unncessary test fail annotation of stroke-dasharray's wpt.

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?
Attachment #8888726 - Flags: review?(hikezoe) → review+

Comment 24

3 days ago
mozreview-review
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

::: 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?
Attachment #8888725 - Flags: review?(hikezoe)
(Assignee)

Comment 25

18 hours ago
mozreview-review-reply
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.

I think it is same problem of bug 1344132 comment 22.
(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.
(Assignee)

Comment 27

12 hours ago
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.
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?
(Assignee)

Comment 29

11 hours ago
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?).
(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?
You need to log in before you can comment on or make changes to this bug.