Bug 5164

Summary: Non-integer percentages are incorrectly rounded down in Safari
Product: WebKit Reporter: Doug Wright <apple>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ddkilzer, emmanuel.gomez, eric, ian, mitz, moz, safari-bugs2, sam, stuartmorgan
Priority: P2 Keywords: GoogleBug, HasReduction, InRadar
Version: 412   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.dougweb.org/wp/?page_id=121
Bug Depends on:    
Bug Blocks: 5531    
Attachments:
Description Flags
testcase
none
Another illustration of the issue.
none
[Do not commit] Increase percent precision; no change log or layout tests.
none
Increase percent precision hyatt: review+

Doug Wright
Reported 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.
Attachments
testcase (545 bytes, text/html)
2005-10-11 11:29 PDT, j.j.
no flags
Another illustration of the issue. (1.12 KB, text/html)
2005-10-25 07:19 PDT, Steve Melenchuk
no flags
[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
Increase percent precision (198.41 KB, patch)
2007-01-03 11:22 PST, mitz
hyatt: review+
j.j.
Comment 1 2005-10-11 11:29:58 PDT
Created attachment 4316 [details] testcase
j.j.
Comment 2 2005-10-11 11:35:03 PDT
Same problem with padding and margin, see testcase above.
Doug Wright
Comment 3 2005-10-11 11:37:05 PDT
Steve Melenchuk
Comment 4 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.
Daniel Udey
Comment 5 2005-10-25 07:36:43 PDT
Bug confirmed in ToT (Oct 24th nightly).
Eric Seidel (no email)
Comment 6 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.
Eric Seidel (no email)
Comment 7 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.
Alexey Proskuryakov
Comment 8 2006-09-26 21:28:51 PDT
*** Bug 11052 has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 9 2006-09-27 08:56:37 PDT
mitz
Comment 10 2006-11-20 03:26:59 PST
*** Bug 11653 has been marked as a duplicate of this bug. ***
mitz
Comment 11 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 ;-)
Eric Seidel (no email)
Comment 12 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.
Dave Hyatt
Comment 13 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.
mitz
Comment 14 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).
mitz
Comment 15 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.
Dave Hyatt
Comment 16 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
mitz
Comment 17 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!
David Kilzer (:ddkilzer)
Comment 18 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).
David Kilzer (:ddkilzer)
Comment 19 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.
mitz
Comment 20 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.
Dave Hyatt
Comment 21 2007-01-03 14:06:35 PST
Comment on attachment 12194 [details] Increase percent precision r=me
David Kilzer (:ddkilzer)
Comment 22 2007-01-03 21:44:40 PST
Committed revision 18574.
Note You need to log in before you can comment on or make changes to this bug.