fix: do not round non-zero values down to zero - #264
Conversation
ludofischer
left a comment
There was a problem hiding this comment.
Could you explain the goal of this change? Right now I don't understand what's going on.
|
|
||
| test( | ||
| 'should not round a non-zero value down to zero', | ||
| testValue('calc(1/1000000)', '0.00001') |
There was a problem hiding this comment.
Why does dividing 1 by one million give 1e-06 instead of 1e-05? I guess it's related to the precision being 5 decimal digits, but I would appreciate some explanation or a link to the spec.
There was a problem hiding this comment.
You're right that 0.00001 (1e-5) was wrong: the true value is 0.000001 (1e-6), and the old code was clamping to "smallest value representable at 5 decimal places", which is a made-up magnitude (10× too large here, 30× for 1/3000000).
The precision isn't a spec rule. It's our own precision option (default 5), just for minified output. The spec (serialize a calculation tree) doesn't round at all. So rather than invent a number, when a value is too small for precision decimal places I now fall back to counting significant digits: 1/1000000 → 0.000001, 1/3000000 → 3.3333e-7. precision still bounds the detail, but the value stays true. The test now asserts 0.000001.
| } | ||
| const m = Math.pow(10, prec); | ||
| return Math.round(v * m) / m; | ||
| const rounded = Math.round(v * m) / m; |
There was a problem hiding this comment.
Should be some comment about explaining why the code is doing this or nobody will understand it in the future.
There was a problem hiding this comment.
Agreed. Added a comment on the noise floor and on round() itself
|
After some research, it looks like rounding a small value to non-zero is similar to the algorithm for |
The goal: when a The fix has two halves:
|
Short answer: no, generalizing line-width snapping would be incorrect which is why the current version doesn't do it. Line-width snapping is a property-specific rendering rule: it's scoped to line widths (border-width, outline-width, column-rule-width), it's applied by the UA at the actual device resolution, and it floors a positive sub-pixel value to 1 device pixel so hairlines stay visible. Three reasons it doesn't generalize:
The current version instead preserves the true magnitude and lets each property's own rules (the browser's line-width snap included) apply downstream. And to avoid leaking a phantom for expressions that are mathematically exactly zero (calc(0.07px * 1e7 - 700000px)), those now fold to 0 via a relative noise snap in sum.js. |
No description provided.