For example, "calc(10in)" should be serialized to "calc(10in)" for specified value. All major browser engines agree on this. When the calculation becomes a bit more complicated, though, the result differs. For "calc(10in + 10in)", Gecko and Edge serialize it as is, while Blink serializes it as "calc(20in)". For mixed unit like "calc(10in + 10cm)", Blink resolves to a single pixel value, while Gecko and Edge still serialize as is. I guess we can do either way in Stylo, but we should at least match other browsers for the basic single value case. (FWIW, there are web-platform tests testing the Blink behavior, although the spec doesn't say anything for serialization of calc() for now.) This accounts for several stylo-specific failures in some web-platform tests.

Bug 1350739 may also be related, since it is likely that Servo's simplification on calc() value regressed this.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0) > For example, "calc(10in)" should be serialized to "calc(10in)" for specified > value. All major browser engines agree on this. > > When the calculation becomes a bit more complicated, though, the result > differs. For "calc(10in + 10in)", Gecko and Edge serialize it as is, while > Blink serializes it as "calc(20in)". For mixed unit like "calc(10in + > 10cm)", Blink resolves to a single pixel value, while Gecko and Edge still > serialize as is. > > I guess we can do either way in Stylo, but we should at least match other > browsers for the basic single value case. > > (FWIW, there are web-platform tests testing the Blink behavior, although the > spec doesn't say anything for serialization of calc() for now.) > > This accounts for several stylo-specific failures in some web-platform tests. And there is another issue related to this bug: Bug 1392161 Comment 15. It seems we shouldn't store the absolute length as Au for specified value. For computed value, we probably could store it as a pixel value (CSS Float) to avoid any rounding issue, and convert it into Au or pixel value depended on the case.

According to the discussion with emilio, we will resolve the mix units to a single pixel value, for specified value.

Comment on attachment 8904947 [details] Bug 1396692 - Part 1: Add a method for FontRelativeLength to return pixel value. https://reviewboard.mozilla.org/r/176736/#review181708 Actually, this is a patch from Bug 1392161. I'm not sure which one would be landed first, so still need this patch. In other words, we can reuse this function in this bug and Bug 1392161.

Comment on attachment 8904950 [details] Bug 1396692 - Update wpt expectation. https://reviewboard.mozilla.org/r/176742/#review181712

Comment on attachment 8904947 [details] Bug 1396692 - Part 1: Add a method for FontRelativeLength to return pixel value. https://reviewboard.mozilla.org/r/176736/#review181786 I reviewed this on github. I still don't get the point of `FontReferenceSize`.

Comment on attachment 8904948 [details] Bug 1396692 - Part 1: Keep unit of the serialization of specified::CalcLengthOrPercentage. https://reviewboard.mozilla.org/r/176738/#review181788 The approach looks good, but this has a few unrelated changes, doesn't it? Why is the `CSSFloat` change necessary for this bug? ::: servo/components/style/values/computed/length.rs:73 (Diff revision 1) > #[cfg_attr(feature = "servo", derive(HeapSizeOf))] > #[derive(Clone, Copy, Debug, PartialEq, ToAnimatedZero)] > pub struct CalcLengthOrPercentage { > #[animation(constant)] > pub clamping_mode: AllowedLengthType, > - length: Au, > + length: CSSFloat, This seems completely unrelated to this PR, isn't it? I'd prefer this to be in another bug. ::: servo/components/style/values/computed/length.rs:257 (Diff revision 1) > } > > /// Compute font-size or line-height taking into account text-zoom if necessary. > pub fn to_computed_value_zoomed(&self, context: &Context, base_size: FontBaseSize) -> CalcLengthOrPercentage { > - self.to_computed_value_with_zoom(context, |abs| context.maybe_zoom_text(abs.into()).0, base_size) > + self.to_computed_value_with_zoom(context, > + |abs| context.maybe_zoom_text( Indentation is off here, plus this defeats the point of the previous change noted above. Let's keep the `Au -> CSSFloat` change in another bug / PR, or at least in another part. ::: servo/components/style/values/specified/calc.rs:123 (Diff revision 1) > }; > } > > + macro_rules! serialize_abs { > + ( $( $val:ident ),+ ) => { > + if let Some(val) = self.absolute { `if let Some(AbsoluteLength::$val(v)) = self.absolute {` ::: servo/components/style/values/specified/length.rs:345 (Diff revision 1) > + const PX_PER_IN: CSSFloat = 96.; > + const PX_PER_CM: CSSFloat = PX_PER_IN / 2.54; > + const PX_PER_MM: CSSFloat = PX_PER_IN / 25.4; > + const PX_PER_Q: CSSFloat = PX_PER_MM / 4.; > + const PX_PER_PT: CSSFloat = PX_PER_IN / 72.; > + const PX_PER_PC: CSSFloat = PX_PER_PT * 12.; I've also reviewed this on github fwiw.

Comment on attachment 8904949 [details] Bug 1396692 - Part 3: Make computed::CalcLengthOrPercentage return pixel value as default. https://reviewboard.mozilla.org/r/176740/#review181790 I don't understand how this is related to this bug at all, care to explain? Also, it seems inconsistent to use `CSSFloat` as the lengths in `computed::CalcLengthOrPercentage` but keep all the `computed::Length` values as `Au`.

In particular, I think that if we want to do address this precision issue, it's better to do it everywhere, and just use `Au::from_f32_px` in the properties that need it. That would avoid duplicating all the code special-casing for transform in https://github.com/servo/servo/pull/18381

Comment on attachment 8904947 [details] Bug 1396692 - Part 1: Add a method for FontRelativeLength to return pixel value. https://reviewboard.mozilla.org/r/176736/#review181786 OK, I just need a type to distinguish some special cases from normal cases, and yes, we cannot reuse it, so I will udpate this according to your suggestion.

Comment on attachment 8904948 [details] Bug 1396692 - Part 1: Keep unit of the serialization of specified::CalcLengthOrPercentage. https://reviewboard.mozilla.org/r/176738/#review181788 You are right. I should split it into other patch or do it in another bug. Thanks. > This seems completely unrelated to this PR, isn't it? I'd prefer this to be in another bug. OK. I will do that in another bug. > Indentation is off here, plus this defeats the point of the previous change noted above. > > Let's keep the `Au -> CSSFloat` change in another bug / PR, or at least in another part. I see. Thanks.

Comment on attachment 8904949 [details] Bug 1396692 - Part 3: Make computed::CalcLengthOrPercentage return pixel value as default. https://reviewboard.mozilla.org/r/176740/#review181790 Looks like changing all Au into CSSFloat mighe make sense after we use CSSPixelLength, so the code coule be consistent with other part. I will drop this, and do this in another bug. Thanks.

Comment on attachment 8904948 [details] Bug 1396692 - Part 1: Keep unit of the serialization of specified::CalcLengthOrPercentage. https://reviewboard.mozilla.org/r/176738/#review182250 r=me, thanks! (Had forgot to publish apparently, sorry for the delay)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21) > Comment on attachment 8904948 [details] > Bug 1396692 - Part 1: Keep unit of the serialization of > specified::CalcLengthOrPercentage. > > https://reviewboard.mozilla.org/r/176738/#review182250 > > r=me, thanks! > > (Had forgot to publish apparently, sorry for the delay) Thanks!!

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7217ebcfc33e Update wpt expectation. r=xidorn