1323735 - CSS calc() accumulates rounding error that makes lengths to small, and thus makes (100% - Npx) / 8 too big

Status: RESOLVED FIXED

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

Description

[Jacques]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

We created a calc() based grid system for fluid columns with fixed size gutters. One of the results as follows:

.column {
   width: calc((((100% - 35px ) / 8) * 1) + 0px);
   float: left;
   margin-right: 5px;
   margin-top: 0;
} 

... that is for a single column in an 8-column grid with 5px gutters.

The problem is that the last column falls below its siblings any uneven value gutter width (i.e. 1, 3, 5, 7, etc.).


Actual results:

In this scenario, 100% = 916px (it's a modal with a set max-width) and the last child has the right margin set to zero. 

Firefox calculates this to be 111px (web inspector) or 110.133px (Firebug) - if you take the values and add them up again, you get 923px(web inspector value) or 916.064px (Firebug value), which is wider than the parent. 


Expected results:

The correctly calculated value should be 110.125px

Comment 1

[Mats Palmgren (inactive)]

The reason the math doesn't work the way you expect is that it
assumes that Gecko represents CSS lengths with infinite (or at least
very high) precision.  This is not the case.  Pixel sizes are
represented internally as integers with a factor of 60, so 916px
becomes 54960, and 35px is 2100.  The first is divisible by 8,
but the second is not (262.5), leading to a rounding error (262).
So, the calc() expression is represented internally as "0.125*W - 262",
so with W=54960 we get: 6870 - 262 = 6608 (which in CSS pixels
corresponds to: 6608 / 60 = 110.1333333333).

We could of course use floating point numbers internally to improve
the precision, but that's too slow compared to doing layout using
integer math.

I guess we could use a larger factor to improve the precision but
I suspect 60 was chosen for a reason.
https://dxr.mozilla.org/mozilla-central/source/gfx/src/AppUnits.h#12

I'm guessing you have a rule that sets the margin-right of the last
column to zero?  One workaround would be to add to that rule:
"margin-left:-0.5px" or something like that.

Comment 2

[Mats Palmgren (inactive)]

I guess we could represent mLength for calc() expressions as a float
though to improve the precision for calc() math only, without too
much of performance loss.  Changing this to float:
http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/layout/style/nsStyleCoord.h#82
and then doing all calc() math using float types, and then change this:
http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/layout/style/nsRuleNode.cpp#763-764
to "NSToCoordFloorClamped(calc->mLength + aPercentageBasis * calc->mPercent)"

I think it would fix the reported case, because with "0.125*W - 262.5"
we'd get 6607.5 which NSToCoordFloorClamped turns into 6607, i.e.
6607 / 60 = 110.116666667.

David, do you think it's worth it for calc()s only?  Do you see
any problems?

I guess in some cases there might be a 1px gap at the end instead,
but that seems better than overflowing.

Comment 4

[Mats Palmgren (inactive)]

Attachment: WIP

A quick and dirty WIP patch confirms that it does fix the testcase in
this bug and in bug 1324109.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren (:mats) from comment #2)
> David, do you think it's worth it for calc()s only?  Do you see
> any problems?

I think it's probably reasonable, although perhaps I'd like to wait for a tad more evidence that it's a common problem.  (Why were two reports of the problem filed on two successive days?  Do we have other reports somewhere from other days?)

However, I think we'd probably want to switch to double rather than float, since using float will lose precision for large lengths, since float only has 24-bits of precision (including sign).

Comment 6

[Adam Schwartz]

> Pixel sizes are represented internally as integers with
> a factor of 60, so 916px becomes 54960, and 35px is 2100.

Mats, thank you so much for that explanation. Really clears up a lot for me.

> I guess in some cases there might be a 1px gap at the end
> instead, but that seems better than overflowing.

In the context which uncovered my initial report (https://bugzilla.mozilla.org/show_bug.cgi?id=1324109), it was important that the right column have its right edge flush right, so flooring would not have helped me. That being I would certainly agree that some cases would benefit, so flooring may reasonably be considered a lesser of evils.

The workaround I decided on was to increase the width of the columns’ parent by 1px using `width: calc(100% + 1px)` and then to subtract an additional 1px during the column width calculation. (For example, the column width used in this bug report would change from `calc((100% - 35px) / 8)` to `calc((100% - 36px) / 8)` [eight columns with seven 5px gutters]).

I’m also curious how WebKit is handling this such that they don’t run into the same precision issues. Are they using integers with a larger factor, for example?

Comment 7

[Jacques]

(In reply to Mats Palmgren (:mats) from comment #1)
> The reason the math doesn't work the way you expect is that it
> assumes that Gecko represents CSS lengths with infinite (or at least
> very high) precision.  This is not the case.  Pixel sizes are
> represented internally as integers with a factor of 60, so 916px
> becomes 54960, and 35px is 2100.  The first is divisible by 8,
> but the second is not (262.5), leading to a rounding error (262).
> So, the calc() expression is represented internally as "0.125*W - 262",
> so with W=54960 we get: 6870 - 262 = 6608 (which in CSS pixels
> corresponds to: 6608 / 60 = 110.1333333333).

Thank you for the explanation Mats - that clarifies the issue perfectly.

> I'm guessing you have a rule that sets the margin-right of the last
> column to zero?  One workaround would be to add to that rule:
> "margin-left:-0.5px" or something like that.

Correct on the zero margin of the last column.

The CSS for our grid columns is created via SASS mixin (hence the additional [apparently redundant] values at the end of the calc() ... so:

width: calc((((100% - 35px ) / 8) * 1) + 0px);

... is actually derived from:

width: calc(((([Parent width] - [Column count - 1 for total gutters]) / [Column count]) * [Column span]) + [Amount of missing cutters when having a span of more than 1 column]);

Within our SASS we then simply specify:

.column {
  @include gridColSpan(1 of 8, $gutter);
}

To expand on that with another modifier, based on a condition that catches only column counts that could be problematic, is a little too bespoke for us. We are already offsetting by a small amount to cover IE rounding issues, inside an IE-specific media query, no such query/hack exists for Firefox that I am aware of; else we'd bake it in.

Comment 8

[Adam Schwartz]

Jacques I believe you may be able to use this:

@media screen and (min--moz-device-pixel-ratio: 0) {
  /* ... */
}

Of course this won’t necessarily be future-proof.

Comment 9

[Jacques]

(In reply to Adam Schwartz from comment #8)
> Jacques I believe you may be able to use this:
> 
> @media screen and (min--moz-device-pixel-ratio: 0) {
>   /* ... */
> }
> 
> Of course this won’t necessarily be future-proof.

Thank you Adam - I reckon that future proofing won't be an issue; especially not if the rounding bug is fixed; we can just remove the hack if/when it is.

Comment 10

[qlum]

I also came across this bug although in my case it was easy to work around.
First of all I only used calc for visibility within the css and could just have put in the output of calc at any point. Anyway here is what I used:
calc(1100px / 961 * 783) which incorrectly turned into 900.45px. The workaround was knowing this bug to simply use px in the end so I ended up with: calc(1px*(1100 / 961 * 783)) which works correctly.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

This looks fixed by Stylo.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1323735: Test for calc() rounding error.

Review commit: https://reviewboard.mozilla.org/r/251228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251228/

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8985687 [details]
Bug 1323735: Test for calc() rounding error.

https://reviewboard.mozilla.org/r/251228/#review257564

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdac7a88a2c0
Test for calc() rounding error. r=mats

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/11540 for changes under testing/web-platform/tests

Comment 16

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/fdac7a88a2c0

Comment 18

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

Upstream PR merged