Closed Bug 1369614 Opened 7 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
This is responsible for one of the failures in dom/smil/test/test_smilCSSFromTo.xhtml.
Priority: -- → P2
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
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.
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.
(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.
(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/
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
Attached file Servo PR, #17704
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.
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
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.
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
(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
(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
(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 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 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?
Attachment #8888726 - Flags: review?(hikezoe) → review+
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?
Attachment #8888725 - Flags: review?(hikezoe)
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.
(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.
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?
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?
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.
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.
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.
No longer blocks: 1338764
Depends on: 1338764
(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.
Priority: P2 → --
Priority: -- → P2
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).
(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
Thanks for working this, and sorry about any trouble because of that.
Attachment #8888727 - Attachment is obsolete: true
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 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.
Attachment #8888725 - Flags: review?(hikezoe)
Blocks: 1387982
Blocks: 1387080
Blocks: 1387986
No longer blocks: 1387982
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 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.
Attachment #8888725 - Flags: review?(hikezoe) → review+
(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.
Attachment #8888726 - Attachment is obsolete: true
(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.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dc40bdb0052e
Add test fail condition to reftest of interpolating stroke-width between px and percentage. r=hiro
https://hg.mozilla.org/mozilla-central/rev/dc40bdb0052e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: