1764768 - calc() has rounding errors with viewport relative length units because of (somewhat intentional) truncation

Status: ASSIGNED

(Core :: CSS Parsing and Computation, defect)

Description

[till toenges]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:99.0) Gecko/20100101 Firefox/99.0

Steps to reproduce:

Open Console. Make sure the body is exactly 1400px wide for this example. Execute the following code:

console.log(getComputedStyle(document.body).width);
document.body.style.fontSize = 'calc(100vw)';
console.log(getComputedStyle(document.body).fontSize);
document.body.style.fontSize = 'calc(100vw / 1920)';
console.log(getComputedStyle(document.body).fontSize);
document.body.style.fontSize = 'calc(1400px / 1920)';
console.log(getComputedStyle(document.body).fontSize);

Actual results:

Output is:

1400px
1400px
0.716667px
0.729167px

Expected results:

Expected output is:

1400px
1400px
0.729167px
0.729167px

This may not seem like a large difference, but if you are afterwards multiplying by large numbers the results are very significantly different. The CSS itself is very much able to handle the precision, as shown when using the 1400px value instead of 100vw, and it gets the correct value for 100vw as shown in the second line of output. It is really calc itself that introduces some weird rounding errors.

Other browsers do not have this problem. I do believe that this was working fine a few years ago, as this is not the first time I use this type of computation.

Comment 1

[till toenges]

BTW: For the exact numbers in the example the page should not have scroll bars visible, otherwise the computation will yield different, but still wrong results.

Comment 2

[till toenges]

Improved example that will work in a blank new tab due to setting padding and margin to 0, otherwise the same:

document.body.style.padding = '0px';
document.body.style.margin = '0px';
console.log(getComputedStyle(document.body).width);
document.body.style.fontSize = 'calc(100vw)';
console.log(getComputedStyle(document.body).fontSize);
document.body.style.fontSize = 'calc(100vw / 1920)';
console.log(getComputedStyle(document.body).fontSize);
document.body.style.fontSize = 'calc(1400px / 1920)';
console.log(getComputedStyle(document.body).fontSize);

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: (iframe for use in testcase)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: (iframe for use in testcase 2)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (using smaller values, and `width` on a helper-element, instead of `fontSize`)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to till toenges from comment #0)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:99.0) Gecko/20100101 Firefox/99.0

Steps to reproduce:

Open Console. Make sure the body is exactly 1400px wide for this example. Execute the following code:

For convenience, I've attached testcases where the test happens in an iframe of fixed-width (1400px wide in testcase 1).

Actual results:
[...]
0.716667px
0.729167px

Yeah, I can reproduce these results as the output of testcase 1; whereas Chrome gives 0.729167px for both values. FWIW our values here differ by 0.0125 , which happens to be 1/80. (Though I suspect really we're off by 1/60, given that internally we quantize to nscoord which is 1/60 of a CSS pixel as our unit of length. Perhaps one or both values here are undergoing a bit of additional float error/rounding which ends up muddying our amount of error so that it looks like it's 1/80.)

Testcase 2 gives these results in Firefox, which (as in testcase 1) you would probably expect to match (and the values do match in Chrome):

0.766667px
0.783333px

Here, the values are off by 0.016666 i.e. 1/60px, so it looks like this one is indeed a difference in rounding between two codepaths when converting a fractional value to a nscoord.

Comment 9

[Daniel Holbert [:dholbert]]

Looking at testcase 2 in rr, it looks like the rounding error happens pretty early, in the style system -- it's already encoded in to the StyleCSSPixelLength struct by the time we hit nsIFrame::ComputeSize for the frame in question.

If I put a breakpoint in StyleCSSPixelLength::ToAppUnits() and load testcase 2, and print out _0, it shows this value when we use the result of the calc(100vw / 192) expression in layout (where 100vw is in fact equivalent to 150px):

0.766666651

vs. this when we use the result of the calc(150px / 192) expression in layout:

0.78125

The former value just so happens to be ~exactly 46/60, so ToAppUnits() ends up returning 46.
The latter value is the "correct/precise" fractional representation, i.e. it's the actually-correct fractional answer to dividing 150/192. And when we multiply it by 60 to convert into app units, it produces 46.875 which we round up to 47 app units.

So we apparently are doing some early rounding on the style-system side, specifically in the calc(100vw/192) scenario. It looks like maybe we are doing our calc(vw)-simplification arithmetic in nscoord i.e. app-unit space, and flooring the result, and then we convert back into floating-point-css-pixels, and the result 0.766666651 ends up getting stored in our StyleCSSPixelLength._0 member-var.

emilio, do you know why/where that might be happening? Should we remove the early-rounding here so that we behave consistently?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Yeah viewport units are tricky. We go through app units because mostly legacy reasons that don't apply to calc(), see bug 1396045 and co.

Back then we represented all lengths in the style system as app units, which is no longer true, and we have made more consistent rounding decisions so we can try to change this.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1764768 - Don't truncate viewport units at computed value time. r=dholbert

Instead, truncate when converting to a length, matching the default behavior
for all <length-percentage>s.

Keep regular Length rounding by default.

Depends on D143856

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1764768 - Clean up a CSS length values test. r=dholbert

A previous iteration of the patch for this bug caused some failures
here, and these changes made them easier to debug.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1764768 - Don't go through app-units-ratio to convert absolute lengths. r=dholbert

This doesn't change behavior on its own, but the current code introduces
some minor floating point error which we can avoid, and which would
cause failures with the other patch on the bug.

Depends on D143941

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e796201e551
Clean up a CSS length values test. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0c35c52d51a0
Don't go through app-units-ratio to convert absolute lengths. r=dholbert

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/33683 for changes under testing/web-platform/tests

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7e796201e551
https://hg.mozilla.org/mozilla-central/rev/0c35c52d51a0

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7985a1fa0789
Don't truncate viewport units at computed value time. r=dholbert

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/33718 for changes under testing/web-platform/tests

Comment 20

[Noemi Erli[:noemi_erli]]

Backed out changeset 7985a1fa0789 (Bug 1764768) for causing multiple failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=7985a1fa0789fa8173f92f3a7ff18ec8bb46c8ce&selectedTaskRun=VtEZSkLrRimOzTP8LLrE6Q.0

Log: https://treeherder.mozilla.org/logviewer?job_id=375187774&repo=autoland&lineNumber=10136
https://treeherder.mozilla.org/logviewer?job_id=375187631&repo=autoland&lineNumber=9511
https://treeherder.mozilla.org/logviewer?job_id=375187647&repo=autoland&lineNumber=5035

Backout: https://hg.mozilla.org/integration/autoland/rev/22b9db7af0682d48b187af98287b8030d8b5e556

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 22

[Emilio Cobos Álvarez (:emilio)]

So the tests reveal a real issue which is that truncating isn't really always desired for other units like e.g., ch...

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

See above.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1764768 - Improve precision of viewport lengths. r=jfkthame,#style,#layout

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Grr, I failed to grep for one of the tests for bug 989802, which fails.