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.

(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.

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

Comment on attachment 8904192 [details] Bug 1392161 - Part 3: Add reftests for relative lengths. https://reviewboard.mozilla.org/r/175970/#review181146

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.

(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.

(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.

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.

Comment on attachment 8904188 [details] Bug 1392161 - Part 2: Replace Au with CSSPixelLength in CalcLengthOrPercentage. https://reviewboard.mozilla.org/r/175962/#review181794

Comment on attachment 8904189 [details] Bug 1392161 - Part 3: Use CSSPixelLength in LengthOrPercentage{*}. https://reviewboard.mozilla.org/r/175964/#review181796

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

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

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

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`?

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.

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 :)

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!

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.

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

https://hg.mozilla.org/mozilla-central/rev/747600ab0229 https://hg.mozilla.org/mozilla-central/rev/7ac6470c98a1 https://hg.mozilla.org/mozilla-central/rev/5872feaa0656