Closed Bug 1396692 Opened 2 years ago Closed 2 years ago

stylo: unit of absolute length needs to be preserved in calc()

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: boris)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
See Also: → 1350739
Priority: -- → P2
See Also: → 1392161
(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.
Assignee: nobody → boris.chiou
According to the discussion with emilio, we will resolve the mix units to a single pixel value, for specified value.
Status: NEW → ASSIGNED
See Also: 1392161
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
Attachment #8904950 - Flags: review?(xidorn+moz) → review+
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`.
Attachment #8904947 - Flags: review?(emilio)
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.
Attachment #8904948 - Flags: review?(emilio)
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`.
Attachment #8904949 - Flags: review?(emilio)
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.
Attachment #8904947 - Attachment is obsolete: true
Attachment #8904949 - Attachment is obsolete: true
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)
Attachment #8904948 - Flags: review?(emilio) → review+
(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!!
Attachment #8904948 - Attachment is obsolete: true
Attached file Servo PR, #18409
https://hg.mozilla.org/mozilla-central/rev/7217ebcfc33e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.