Closed Bug 1392161 Opened 7 years ago Closed 7 years ago

stylo: Length values should not be rounded to Au for computed transform

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: boris)

References

Details

Attachments

(4 files, 3 obsolete files)

So web-platform-test css/css-transforms-1/transform-rounding-001.html fails because it applies a transform "transform: scale(100000) translate(0.001px) scale(0.00001)", which should result to "translate(100px)" in theory, but stylo produces no transform at all.

This is because Servo round the "0.001px" into zero au during computation.
Gecko currently preserve the specified value done to use time. Its approach isn't perfect either, it actually only works for px unit because in this case the float value from specified value is used directly [1], while other units would go through nsRuleNode::CalcLength's method and convert into app units as well.

That says, if we change the transform slightly, say
> font-size: 1px;
> transform: scale(100000) translate(0.001em) scale(0.00001);
Gecko would fail as well. But this probably doesn't matter as much.

To fix this, we would probably need to make Servo's transform also use specified value as computed value, and do the matrix computation based on the float number directly rather than convert them through au.

This does sound like some non-trivial work to do, for Servo side, and I'm not sure whether it is that important. Probably P3 as far as it doesn't show up on any real site?


[1] https://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/layout/style/nsStyleTransformMatrix.cpp#161
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> To fix this, we would probably need to make Servo's transform also use
> specified value as computed value, and do the matrix computation based on
> the float number directly rather than convert them through au.

Agree. I guess this might also fix some precision issues we met before in test_animation_omta.html while comparing the result in Servo with that in Gecko.
Or in-short-term, we might introduce a new type to Translate. For example, something like TranslateLength or TranslateLengthOrPercentage, to replace computed::LengthOrPercentage and computed::Length. Or Make Length or LengthOrPercentage generic, so we can replace the type of Length.
(In reply to Boris Chiou [:boris] from comment #3)
> Or in-short-term, we might introduce a new type to Translate. For example,
> something like TranslateLength or TranslateLengthOrPercentage, to replace
> computed::LengthOrPercentage and computed::Length. Or Make Length or
> LengthOrPercentage generic, so we can replace the type of Length.

This idea also makes ComputeSquaredDistance happy because we need pixel values, instead of Au, to compute the distance.
And we need to handle this carefully to make sure ProcessTranslatePart() also work. Take this for now.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
(In reply to Boris Chiou [:boris] from comment #5)
> And we need to handle this carefully to make sure ProcessTranslatePart()
> also work. Take this for now.

OK, the spec [1] says:
  Computed value: 	As specified, but with relative lengths converted into absolute lengths.

Therefore, we should convert the lengths into absolute lengths in ToComputedValue for |TransformLengthOePerecentage| and |TransformLength|, and it seems we don't need to do anything in ProcessTranslatePart() because the nsCSSValue which we pass in is an absolute value. Maybe we can just convert all length units into "px" for translate in ToComputedValue? I think this may make things easier.

[1] https://www.w3.org/TR/css-transforms-1/#transform-property
For calc type, I notice that we also use "Au" in its specified value (i.e. specified::CalcLengthOrPercentage). In other words, we parse it as CSSFloat, but merge the calc expression as Au (for absolute value). I'm not sure how many things we need to do to make sure specified::CalcLengthOrPercentage always use CSSFloat, so in this bug, I will only fix the "Length" part. i.e. Use CSSFloat, instead of Au for Length. We could fix CalcLengthOrPercentage in another bug.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> That says, if we change the transform slightly, say
> > font-size: 1px;
> > transform: scale(100000) translate(0.001em) scale(0.00001);
> Gecko would fail as well. But this probably doesn't matter as much.

I will fix this on Servo, and add a test which marks Gecko is failed.
See Also: → 1396535
We have to file another bug to fix this for calc expression because this case is still failed (on both Gecko and Stylo):

> transform: scale(100000) translate(calc(0.001px)) scale(0.00001);
vs
> transform: translate(100px);
Comment on attachment 8904191 [details]
Bug 1392161 - Part 2: Update test expectation.

https://reviewboard.mozilla.org/r/175968/#review181144
Attachment #8904191 - Flags: review?(xidorn+moz) → review+
See Also: → 1396692
Comment on attachment 8904192 [details]
Bug 1392161 - Part 3: Add reftests for relative lengths.

https://reviewboard.mozilla.org/r/175970/#review181146
Attachment #8904192 - Flags: review?(xidorn+moz) → review+
This looks like a lot of code to avoid a rounding issue in transform(). I'd be opposed to landing 400+ lines of code for this. 

Why is this a problem specific to transform? Also, can you send this patches as a GitHub PR? That way other people can also take a look.
Flags: needinfo?(boris.chiou)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> This looks like a lot of code to avoid a rounding issue in transform(). I'd
> be opposed to landing 400+ lines of code for this. 
> 
> Why is this a problem specific to transform?

For transform, the computed value is the same as the specified value, only convert relative lengths into absolute lengths. And for computation and rendering, we convert the transform list into a matrix, which is a float-base computation, so there are some rounding issues if we keep using Au.
e.g. transform has a scale function, which may scale the matrix (e.g. scale(10000)), so translate(0.001px) makes sense in this case.

> Also, can you send this patches
> as a GitHub PR? That way other people can also take a look.

OK. Sure.
Flags: needinfo?(boris.chiou)
Attached file Servo PR, #18381
(In reply to Boris Chiou [:boris] from comment #15)
> We have to file another bug to fix this for calc expression because this
> case is still failed (on both Gecko and Stylo):
> 
> > transform: scale(100000) translate(calc(0.001px)) scale(0.00001);
> vs
> > transform: translate(100px);

Filed Bug 1397146 for this.
See Also: 13966921397146
Comment on attachment 8904187 [details]
Bug 1392161 - Part 1: Update Servo binding functions for CSSPixelLength.

https://reviewboard.mozilla.org/r/175960/#review181792

I reviewed this on GitHub. I think we should just change to use CSSFloat as the computed value, and use `Au::from_f32_px` just in the relevant places. Does that seem reasonable? That doesn't need to block bug 1396692.
Attachment #8904187 - Flags: review?(emilio)
Comment on attachment 8904188 [details]
Bug 1392161 - Part 2: Replace Au with CSSPixelLength in CalcLengthOrPercentage.

https://reviewboard.mozilla.org/r/175962/#review181794
Attachment #8904188 - Flags: review?(emilio)
Comment on attachment 8904189 [details]
Bug 1392161 - Part 3: Use CSSPixelLength in LengthOrPercentage{*}.

https://reviewboard.mozilla.org/r/175964/#review181796
Attachment #8904189 - Flags: review?(emilio)
Comment on attachment 8904190 [details]
Bug 1392161 - Part 4: Fix extremely small pixel value for transform on Servo.

https://reviewboard.mozilla.org/r/175966/#review181800
Attachment #8904190 - Flags: review?(emilio)
Thanks, emilio. I'm working this based on your suggestion. :)
According to emilio's suggestion [1], let's start to use computed::CSSPixelLength as the computed value of specified::Length.

[1] https://github.com/servo/servo/pull/18381#issuecomment-327495125
Comment on attachment 8904187 [details]
Bug 1392161 - Part 1: Update Servo binding functions for CSSPixelLength.

https://reviewboard.mozilla.org/r/175960/#review183244

::: servo/components/gfx/font_context.rs:214
(Diff revision 2)
>  
>          if !cache_hit {
>              let template_info = self.font_cache_thread.last_resort_font_template(desc.clone());
>              let layout_font = self.create_layout_font(template_info.font_template,
>                                                        desc.clone(),
> -                                                      style.font_size.0,
> +                                                      style.font_size.into(),

Could we convert these `into` calls into `Au::from`?

I think long term we want to have most of these stored as `Au` in Servo's structs.

That's probably blocked in a fair amount of work, but it'd be nice to not have magic conversions that end up being redundant in the end, so making it more obvious is better I think.

::: servo/components/layout/display_list_builder.rs:1601
(Diff revision 2)
>                                                      style: &ComputedValues,
>                                                      bounds: &Rect<Au>,
>                                                      clip: &Rect<Au>) {
>          use style::values::Either;
>  
> -        let width = style.get_outline().outline_width.0;
> +        let width= style.get_outline().outline_width.into();

nit: Whitespace.

::: servo/components/style/values/computed/length.rs:691
(Diff revision 2)
> +}
> +
> +impl ToCss for CSSPixelLength {
> +    #[inline]
> +    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +        self.0.to_css(dest)

Shouldn't this also serialize `px`?

::: servo/components/style/values/computed/length.rs:739
(Diff revision 2)
> +
> +impl NonNegativeLength {
> +    /// Create a NonNegativeLength.
> +    #[inline]
> +    pub fn new(px: CSSFloat) -> Self {
> +        NonNegative::<Length>(Length::new(px.max(0.)))

You can omit the `::<Lenght>` I think.

::: servo/components/style/values/computed/length.rs:757
(Diff revision 2)
> +    }
> +
> +    /// Scale this NonNegativeLength.
> +    #[inline]
> +    pub fn scale_by(&self, factor: f32) -> Self {
> +        // scale this by zero if factor is negative.

This is somewhat magical, please document it in the doc comment at least. (I know it's pre-existing).

::: servo/components/style/values/computed/length.rs:765
(Diff revision 2)
> +}
> +
> +impl From<Length> for NonNegativeLength {
> +    #[inline]
> +    fn from(len: Length) -> Self {
> +        NonNegative::<Length>(len)

ditto.

::: servo/components/style/values/specified/mod.rs:555
(Diff revision 2)
>      type ComputedValue = super::computed::ClipRect;
>  
>      #[inline]
>      fn to_computed_value(&self, context: &Context) -> super::computed::ClipRect {
>          super::computed::ClipRect {
> -            top: self.top.as_ref().map(|top| top.to_computed_value(context)),
> +            top: self.top.as_ref().map(|top| top.to_computed_value(context).into()),

Should `computed::ClipRect` be switched to `CSSPixelLength` instead of adding all these `into`?
Attachment #8904187 - Flags: review?(emilio) → review+
Comment on attachment 8904188 [details]
Bug 1392161 - Part 2: Replace Au with CSSPixelLength in CalcLengthOrPercentage.

https://reviewboard.mozilla.org/r/175962/#review183250

Looks fine, with the same caveats as the previous patch.
Attachment #8904188 - Flags: review?(emilio) → review+
Comment on attachment 8904189 [details]
Bug 1392161 - Part 3: Use CSSPixelLength in LengthOrPercentage{*}.

https://reviewboard.mozilla.org/r/175964/#review183252

Nice to see this patch being negative in size :)
Attachment #8904189 - Flags: review?(emilio) → review+
Comment on attachment 8904190 [details]
Bug 1392161 - Part 4: Fix extremely small pixel value for transform on Servo.

https://reviewboard.mozilla.org/r/175966/#review183254

Neat!
Attachment #8904190 - Flags: review?(emilio) → review+
Comment on attachment 8904187 [details]
Bug 1392161 - Part 1: Update Servo binding functions for CSSPixelLength.

https://reviewboard.mozilla.org/r/175960/#review183654

::: servo/components/style/gecko/media_queries.rs:725
(Diff revision 2)
>                  one.to_computed_value(&context)
> -                    .cmp(&other.to_computed_value(&context))
> +                    .partial_cmp(&other.to_computed_value(&context)).unwrap()

Oops. This causes some test failures because we try to compare two floats for media queries, especially when we use units other than pixel, e.g. mm. I think we still need to convert this into app_unit (integer) to compare.

::: servo/components/style/values/computed/mod.rs:150
(Diff revision 2)
>          &self.builder
>      }
>  
>      /// Apply text-zoom if enabled.
>      #[cfg(feature = "gecko")]
> -    pub fn maybe_zoom_text(&self, size: NonNegativeAu) -> NonNegativeAu {
> +    pub fn maybe_zoom_text(&self, size: NonNegativeLength) -> NonNegativeLength {

I'd like to update the function signature of maybe_zoom_text to use CSSPixelLength. It is possible to pass a negative CSSPixelLength into this function (, especially for the length part of CalcLengthOrPercentage), so we shouldn't use NonNegativeLength.
Comment on attachment 8904187 [details]
Bug 1392161 - Part 1: Update Servo binding functions for CSSPixelLength.

https://reviewboard.mozilla.org/r/175960/#review183244

> Could we convert these `into` calls into `Au::from`?
> 
> I think long term we want to have most of these stored as `Au` in Servo's structs.
> 
> That's probably blocked in a fair amount of work, but it'd be nice to not have magic conversions that end up being redundant in the end, so making it more obvious is better I think.

Sure. Either way is ok to me, and I'm not pretty sure which way is preferable in rust. Thanks for the suggestion, I will use ```Au::from```.

> Shouldn't this also serialize `px`?

Yes... I forgot it. Thanks.

> Should `computed::ClipRect` be switched to `CSSPixelLength` instead of adding all these `into`?

OK, I will also use ```CSSPixelLength``` for ```computed::ClipRect```. Thanks.
Attachment #8904188 - Attachment is obsolete: true
Attachment #8904189 - Attachment is obsolete: true
Attachment #8904190 - Attachment is obsolete: true
For now, we only fix the extremely small pixel value and font relative value for transform. Viewport length (Bug 1396535 or another bug for stylo) and Calc expression (Bug 1397146) are still failed. Let's wait for try result.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/747600ab0229
Part 1: Update Servo binding functions for CSSPixelLength. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7ac6470c98a1
Part 2: Update test expectation. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5872feaa0656
Part 3: Add reftests for relative lengths. r=xidorn
You need to log in before you can comment on or make changes to this bug.