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

RESOLVED WORKSFORME

Status

()

defect
P3
normal
RESOLVED WORKSFORME
2 years ago
2 days ago

People

(Reporter: boris, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 affected)

Details

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
See Also: → 1392161
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.
Reporter

Comment 2

2 years ago
At lease I can fix (a) for "Servo" in Bug 1396692.
(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.)
Flags: needinfo?(boris.chiou)
Reporter

Comment 4

2 years ago
(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
Flags: needinfo?(boris.chiou)
Reporter

Comment 5

2 days ago

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

Status: NEW → RESOLVED
Last Resolved: 2 days ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.