1397146 - The length part of calc() should not be rounded to nscoord (integer) for computed transform

Status: RESOLVED WORKSFORME

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

Description

[Boris Chiou [:boris]]

According to Bug 1392161 Comment 15, there is an example:
a)
> transform: scale(100000) translate(calc(0.001px)) scale(0.00001);
b)
> transform: scale(100000) translate(0.001px) scale(0.00001);
c)
> transform: translate(100px);

The results of the above three lines should be identical.
However, the actual result:
Gecko: (b) and (c) are correct. (a) is failed.
Stylo: (b) will be fixed by Bug 1392161. (c) is correct. (a) is failed.
Blink and Webkit: all are correct.

I just tried to fix (a) for Stylo, but unfortunately, even if I change the length type of computed::CalcLengthOrPercentage to CSSFloat, this bug is still there because Gecko stores it as an integer (i.e. nsStyleCoord::CalcValue::mLength is nscoord [1]). Therefore, in order to fix the extremely small value for transform, we have to fix it on Gecko first. I guess this needs much work because lots of codes use nsStyleCoord::CalcValue, and I'm not sure changing it to float is a good idea on Gecko now. Maybe we need to add a different CalcValue type for transform, if needed? Anyway, I'm not sure how to fix this for now.

[1] http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/layout/style/nsStyleCoord.h#93

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Yeah, I think we need to make App unit having more accurate value both in Gecko and Servo. Or just adding a new struct instead of App unit.

Comment 2

[Boris Chiou [:boris]]

At lease I can fix (a) for "Servo" in Bug 1396692.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Boris Chiou [:boris] from comment #0)
> Stylo: (b) will be fixed by Bug 1392161. (c) is correct. (a) is failed.
[ ... ]
(In reply to Boris Chiou [:boris] from comment #2)
> At lease I can fix (a) for "Servo" in Bug 1396692.

Per above ^^, it sounds like all cases here are correct now in stylo-enabled Firefox -- is that right?

If so: can we close this bug as FIXED with dependency on bug 1243581 (stylo) and your helper bugs noted above?  Or is there remaining work left to be done here?  (I don't think we need to bother fixing obsoleted-by-stylo correctness issues at this point.)

Comment 4

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (In reply to Boris Chiou [:boris] from comment #0)
> > Stylo: (b) will be fixed by Bug 1392161. (c) is correct. (a) is failed.
> [ ... ]
> (In reply to Boris Chiou [:boris] from comment #2)
> > At lease I can fix (a) for "Servo" in Bug 1396692.
> 
> Per above ^^, it sounds like all cases here are correct now in stylo-enabled
> Firefox -- is that right?

Unfortunately, No. All cases are correct now in pure "Servo" browser. Stylo have to convert the Servo style types into Gecko style structs, so even if we use f32 in Rust now, we still convert it into integer (app units) and store it into Gecko style struct. [1]

> Or is there remaining work left to be done here?  (I don't think we need to bother fixing obsoleted-by-stylo
> correctness issues at this point.)

So I think this bug is the remaining work left to be done. If we could fix this on Gecko side (i.e. use "float" as the length part of calc in Gecko), we can update the conversion in [1], and both Gecko and Stylo could pass the example (a).


[1] http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/servo/components/style/gecko/conversions.rs#32

Comment 5

[Boris Chiou [:boris]]

I cannot reproduce this bug. We use LengthPercent for TransformOperation, and LengthPercent uses StyleCSSFloat to store the length data, so the calculation is correct right now. (Besides, we don't use nscoord for computed transform now.)