Bug 5164 - Non-integer percentages are incorrectly rounded down in Safari
Summary: Non-integer percentages are incorrectly rounded down in Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 412
Hardware: Macintosh OS X 10.4
: P2 Major
Assignee: Nobody
URL: http://www.dougweb.org/wp/?page_id=121
Keywords: GoogleBug, HasReduction, InRadar
: 11052 11653 (view as bug list)
Depends on:
Blocks: 5531
  Show dependency treegraph
 
Reported: 2005-09-28 06:30 PDT by Doug Wright
Modified: 2007-01-03 21:44 PST (History)
9 users (show)

See Also:


Attachments
testcase (545 bytes, text/html)
2005-10-11 11:29 PDT, j.j.
no flags Details
Another illustration of the issue. (1.12 KB, text/html)
2005-10-25 07:19 PDT, Steve Melenchuk
no flags Details
[Do not commit] Increase percent precision; no change log or layout tests. (41.96 KB, patch)
2007-01-02 01:43 PST, mitz
no flags Details | Formatted Diff | Diff
Increase percent precision (198.41 KB, patch)
2007-01-03 11:22 PST, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Wright 2005-09-28 06:30:02 PDT
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.
Comment 1 j.j. 2005-10-11 11:29:58 PDT
Created attachment 4316 [details]
testcase
Comment 2 j.j. 2005-10-11 11:35:03 PDT
Same problem with padding and margin, see testcase above.
Comment 3 Doug Wright 2005-10-11 11:37:05 PDT
xref: http://bugs.kde.org/show_bug.cgi?id=113630
Comment 4 Steve Melenchuk 2005-10-25 07:19:47 PDT
Created attachment 4472 [details]
Another illustration of the issue.

Here's another illustration of the issue, which might make more sense.
Comment 5 Daniel Udey 2005-10-25 07:36:43 PDT
Bug confirmed in ToT (Oct 24th nightly).
Comment 6 Eric Seidel (no email) 2005-12-27 15:27:03 PST
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.
Comment 7 Eric Seidel (no email) 2005-12-27 15:28:50 PST
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.
Comment 8 Alexey Proskuryakov 2006-09-26 21:28:51 PDT
*** Bug 11052 has been marked as a duplicate of this bug. ***
Comment 9 David Kilzer (:ddkilzer) 2006-09-27 08:56:37 PDT
Per Bug 11052:  rdar://problem/4752325
Comment 10 mitz 2006-11-20 03:26:59 PST
*** Bug 11653 has been marked as a duplicate of this bug. ***
Comment 11 mitz 2006-12-04 14:24:16 PST
(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 ;-)
Comment 12 Eric Seidel (no email) 2006-12-04 23:09:30 PST
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.
Comment 13 Dave Hyatt 2006-12-04 23:11:30 PST
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.
Comment 14 mitz 2007-01-01 16:19:35 PST
(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).
Comment 15 mitz 2007-01-02 01:43:41 PST
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.
Comment 16 Dave Hyatt 2007-01-02 16:53:37 PST
Comment on attachment 12157 [details]
[Do not commit] Increase percent precision; no change log or layout tests.

r=me
Comment 17 mitz 2007-01-02 16:57:07 PST
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!
Comment 18 David Kilzer (:ddkilzer) 2007-01-02 21:47:24 PST
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).
Comment 19 David Kilzer (:ddkilzer) 2007-01-02 21:49:15 PST
(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.
Comment 20 mitz 2007-01-03 11:22:49 PST
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.
Comment 21 Dave Hyatt 2007-01-03 14:06:35 PST
Comment on attachment 12194 [details]
Increase percent precision

r=me
Comment 22 David Kilzer (:ddkilzer) 2007-01-03 21:44:40 PST
Committed revision 18574.