Bug 5164 – Non-integer percentages are incorrectly rounded down in Safari

• Component: CSS
• Status: RESOLVED FIXED

[Doug Wright]

See URL for testcase

Basically if I specify a width of 99.999%, Safari treats it as 99.00000%. This bug is also present in 
Konquerer, so I suspect it's inherited behaviour.

[j.j.]

Created attachment 4316 [details]
testcase

[j.j.]

Same problem with padding and margin, see testcase above.

[Doug Wright]

xref: http://bugs.kde.org/show_bug.cgi?id=113630

[Steve Melenchuk]

Created attachment 4472 [details]
Another illustration of the issue.

Here's another illustration of the issue, which might make more sense.

[Daniel Udey]

Bug confirmed in ToT (Oct 24th nightly).

[Eric Seidel (no email)]

This is a simple int vs. float rounding issue.  RenderStyle holds width as a "Length" which uses an int to 
store its intermediate value (in this case 99.999).  That's wrong.  It should use a float.  That could be a 
serious performance issue however and would need to be well tested.  Should be a pretty simple bug for 
someone to fix however.

[Eric Seidel (no email)]

This will also be important for SVG, as SVG needs sub-pixel positioning of all sorts of elements.  Currently 
we do our own "Length" style calculations, but we would like to move to a shared system and would 
already have this same bug in the areas (such as width/height) where we already use Length.

[Alexey Proskuryakov]

*** Bug 11052 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

Per Bug 11052:  rdar://problem/4752325

*** Bug 11653 has been marked as a duplicate of this bug. ***

(In reply to comment #6)
> This is a simple int vs. float rounding issue.  RenderStyle holds width as a
> "Length" which uses an int to 
> store its intermediate value (in this case 99.999).  That's wrong.  It should
> use a float. 
> 

You can use the existing int (minus 3 bits actually) but store 2 digits after the decimal point, i.e. 12.34% would be represented by the integer 1234. This would solve the problem in 99.99% of the cases ;-)

[Eric Seidel (no email)]

Well, SVG requires double precision to be compliant.  Not sure if that's going to matter in the end.  But small rounding errors are quickly exaggerated when you allow for transforms.  (Look at any large SVG on the web to see what I mean.)

Right now SVG has its own SVGLength class, which it uses to all of its layout in the DOM.  The goal (in the short term!) is to get rid of this class, and instead use some sort of length class on the Renderers.  It would be nice to unify with Length, but if that would cause a major slowdown for HTML, then I guess SVG will have to do its own thing.

I'm sure with all the brains behind WebKit you all will come up with with a smart solution which works for both cases.

[Dave Hyatt]

Short term we should just come up with a fix for HTML.  It's a far more important issue atm than getting SVG's precision right.  Longer term we can evaluate a better solution to unify if necessary, but we should do the short term hack in order to get something in before Leopard.

(In reply to comment #11)
> You can use the existing int (minus 3 bits actually) but store 2 digits after
> the decimal point, i.e. 12.34% would be represented by the integer 1234. This
> would solve the problem in 99.99% of the cases ;-)
> 

I am working on a patch that increases the precision of percent Lengths. Currently it makes 5 layout tests fail (although the new results are probably correct).

Created attachment 12157 [details]
[Do not commit] Increase percent precision; no change log or layout tests.

I'm not sure about any of the changes to Length.h, i.e. the Length interfaces, but they allowed me to map out all uses of Length and make them "scaled percent"-safe. So basically after the final Length interfaces are decided, it should be easy to go back to all these places and adapt them if necessary.

Affected layout tests (previous figure of 5 was for scaling by 10, with 128 there's two more):
css1/box_properties/acid_test
css2.1/t09-c5526c-display-00-e
css2.1/t0804-c5507-padn-r-00-c-ag
css2.1/t0804-c5509-padn-l-00-b-ag
fast/block/basic/011
tables/mozilla/bugs/bug4385
tables/mozilla_expected_failures/core/col_span2

All seem like progressions.

[Dave Hyatt]

Comment on attachment 12157 [details]
[Do not commit] Increase percent precision; no change log or layout tests.

r=me

Comment on attachment 12157 [details]
[Do not commit] Increase percent precision; no change log or layout tests.

Thanks for the review! Going to post a complete patch including change log and layout tests. Don't commit this!

[David Kilzer (:ddkilzer)]

Comment on attachment 12157 [details]
[Do not commit] Increase percent precision; no change log or layout tests.

>Index: WebCore/rendering/AutoTableLayout.cpp
>===================================================================
>--- WebCore/rendering/AutoTableLayout.cpp	(revision 18517)
>+++ WebCore/rendering/AutoTableLayout.cpp	(working copy)
>@@ -86,9 +86,10 @@
>                     }
> 
>                     Length w = cell->styleOrColWidth();
>-                    if (w.value() > 32760)
>-                        w.setValue(32760);
>-                    if (w.value() < 0)
>+                    // FIXME: What is this arbitrary value?
>+                    if (w.rawValue() > 32760)
>+                        w.setRawValue(32760);
>+                    if (w.isNegative())
>                         w.setValue(0);
>                     switch(w.type()) {
>                     case Fixed:

I believe the arbitrary 32760 values represent the maximum number of columns allowed in a table.  I think it matches up to what FireFox allows, but I don't remember where I heard about it (in #webkit or in a bug or comment somewhere).

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> I believe the arbitrary 32760 values represent the maximum number of columns
> allowed in a table.  I think it matches up to what FireFox allows, but I don't
> remember where I heard about it (in #webkit or in a bug or comment somewhere).

Or rather, the maximum width of a table column.  Hyatt may know more about this magic number.

Created attachment 12194 [details]
Increase percent precision

Added layout test and updated results. Added change log. Removed a few ASSERTs that I added in the previous version of the patch since they served no purpose beyond the ASSERTs in Length.h. Most of them could not be hit anyway, and for those that could I filed bug 12104.

[Dave Hyatt]

Comment on attachment 12194 [details]
Increase percent precision

r=me

[David Kilzer (:ddkilzer)]

Committed revision 18574.