After interpreting the length of a frame, it is stored in an integer, which means only whole numbers are supported. Decimal fractions are dropped. Current behavoir: 50.2% is interpreted as 50% 50.4% is interpreted as 50% 50.6% is interpreted as 50% 50.8% is interpreted as 50% Expected behavoir: The W3C HTML spec is clear about this issue: n% is valid, n.n% is not. However, IE does support decimal fractions and if the testcase attached to this bug is rendered in IE, it shows a slightly increasing frame height in each column, while the increase is less than 1% overall. We have three options: - Keep the current behavoir (it is spec compliant) - Follow IE and implement support for decimal fractions - Instead of simply dropping the decimal fractions, round it to the nearest integer.
Created attachment 2431 [details] Testcase
Created attachment 2432 [details] Screenshot of IE behavoir
I will confirm this as an issue, it's no bug, but i would go for mimicing IE behavior here cause 1% is a lot on a 1600x1200 display....
> The W3C HTML spec is clear about this issue: n% is valid, n.n% is not. http://www.w3.org/TR/REC-html40/types.html#type-multi-length Looking at the above document, it says that pixels have to be integer, and relative lengths are integers, but it doesn't actually say that a percentage is an integer. And to further muddy the waters, if you look at the DTD itself: http://www.w3.org/TR/REC-html40/sgml/dtd.html It has an example of using "0.5*" as a relative value for a %MultiLength attribute (COL width). IE also allows & interprets decimal fractions for relative values as well as percentage values.
Created attachment 22819 [details] Interpret fractional-percentage MultiLength values. This patch fixes the problem by interpreting fractional percentages for all %MultiLength values. It does not change the behavior for relative values - fractions are still ignored there. Fixing that will involve a larger change.
Comment on attachment 22819 [details] Interpret fractional-percentage MultiLength values. You'll need to use run-webkit-tests to generate the expected results and add them to your patch. Your test is layout-dependant, but not font-dependent. Since it's layout dependent, run-webkit-tests will automatically generate the results under: LayoutTests/platform/mac/fast/frames/frame-length-fractional-expected.txt However since your test doesn't depend on the user's fonts, you can move this file into LayoutTests/fast/frames/rame-length-fractional-expected.txt before calling svn add (and adding it to your patch). I wondered first if your backup case here: + if (ok) + return Length(r, Percent); + return Length(1, Relative); should try to do an int conversion first before returning Length(1, Relative)? But it appears that charactersToDouble returns !ok for the same cases charactersToInt does. It would be nice to validate this with a test case, but such could be relatively complicated for such a simple fix. I think generally we use: unsigned intLength = i; instead of: unsigned intLength(i); But I don't have strong opinions on that subject. We could make a fancier test case for this, but I think this is OK-as is. Hyatt should probably sanity check this, but assuming the layout tests pass this looks fine to me. r- due to lack of expected results for you test case (if this wasn't your first patch, or you were a commiter, I'd just r+ and leave you to make the missing results as you landed). Oh, if you're interested in knowing how I might have made a fancier test case: We have a nifty system for creating js-only test cases using some custom assert-like macros. Basically you copy this file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/TEMPLATE.html into fast/frame/resources/ and then add a frame-length-fractional.js file to fast/frame/resources/ with the following content: description("My fancy test case for bug 3591"); var iframe = document.createElement("iframe"); iframe.style.width = "1000px"; iframe.style.height = "200px; document.body.appendChild(iframe); var frameset = document.createElement("frameset"); iframe.contentDocument.appendChild(frameset); var frame1 = document.createElement("frame"); frameset.appendChild(frame1); var frame2 = document.createElement("frame"); frameset.appendChild(frame2); frameset.setAttribute("rows", "50%,*"); shouldBe(frame1.contentWidth, 500); frameset.setAttribute("rows", "50.3%,*"); shouldBe(frame1.contentWidth, 503); frameset.setAttribute("rows", "50.7%,*"); shouldBe(frame1.contentWidth, 507); frameset.setAttribute("rows", "51%,*"); shouldBe(frame1.contentWidth, 510); frameset.setAttribute("rows", "51 %,*"); shouldBe(frame1.contentWidth, 510); // And then you could add a bunch of failure cases which touch the code relating to your change: frameset.setAttribute("rows", "51.3*"); shouldBe(frame1.contentWidth, 51/52 * 1000); ... etc. Heck, I'd probably even have made my own shoudBe abstraction: fuction testParsing(string, expectedWidth) { frameset.setAttribute("rows", string); shouldBe(frame1.contentWidth, expectedWidth); } If you're interested in trying to add such a test case, be my guest. It's not required for this patch, but I figure the information is useful to you for future patches anyway.
Created attachment 22939 [details] Updated patch incorporating feedback Thanks for the feedback, Eric. I've punted on the fancier test for now. I kept having issues with trying to manipulate the contents of the iframe before it was loaded. I actually got things to update correctly from within a setTimeout, but then I don't believe the test results get reported correctly. So I went back to the simple test & included the expected results this time.
Comment on attachment 22939 [details] Updated patch incorporating feedback Thanks for taking this on. The patch looks good. Some comments about things that are not new to your patch. I think it's strange that this code is also used for HTMLAreaElement. Are these changes all OK for that case too? I think the name of this function is unfortunate since it's not suitable for general length parsing and has quirks that are quite specific to the use parsing frame sizes. The use of Unicode::isDigit seems wrong here. Do we really support non-ASCII digits? I think it should be isASCIIDigit instead. + /* IE Quirk: accept decimal fractions for percentages */ We normally use // comments (despite the IE quirk comment a few lines earlier). r=me
http://trac.webkit.org/changeset/37524