If the contents of a frameset is too large, the size of the frames inside are not reduced evenly to make the frames fit the frameset. An example of this is the fixed.html testcase. It contains two rows of 2000 pixels each. If the height of the window is just 1000px, both frames should be resized back to 500px each. Currently frames are not resized equally, the first frame takes as much space as possible (1000px) and the second frames gets the remaining space (0px). Current result: 100%, 0% Expected result: 50%, 50% Note: This works correctly in both IE and Gecko Similarly, if the contents of a frameset is too small, the size of the frames that have a size of 0% are not enlarged evenly. An example is the percentage testcase. If the height of the window is 1000px, both frames should be resized to 500px each. Currently frames are not enlarged equally, the first frame says 0% (0px), which the last frame is enlarged to cover the whole frameset (1000px). Current result: 0%, 100% Expected result: 50%, 50% Note: This works correctly in both IE and Gecko
Created attachment 2419 [details] Testcase (Fixed frames behavoir)
Created attachment 2420 [details] Testcase (0% frames behavoir)
I'm currently rewriting the algorithm that is used to calculate the width of frames inside a frameset. The new algorithm should fix this bug.
Tested on today's ToT, this bug is indeed there, making it p2 since this could make sites unfunctional. If this occurs on some important site, contact me, i'll increase the priority.
Created attachment 2458 [details] Rewrite of frame calculation
The new frame calculation algorithm fixes a number of problems with the older calculation algorithm: - If fixed frames are too large they are reduced to fit the surrounding frameset *evenly*. This means that 2000,2000 in a 1000 px frameset will be calculated as 500px,500px. Previously this was 1000px,0px. - If percentage frames are too large or too small they are scaled to fit the frameset *evenly*. This means that both 1%,500,1% and 100%,500,100% inside a 1000 px frameset will be scaled to 250px,500px,250px. Previously this was 0px,500px,0px and 500px,500px,0px - If percentage frames are used and *ALL* percentage frames are 0%, Webcore will now follow IE in its behavoir. Example are for a 1000px frameset. 0%,0% => 500px,500px, used to be 0px,0px 0%,200,0% => 0px,1000px,0px, used to be 0px,200px,0px - If there are any pixels remaining after calculation they will be added to the last relative frame, for example if the frameset is 299 pixels and we have the following cols definition: *,*,100, the resulting frames will be calculated as 99px,100px,100px. Previously this was 99px,99px,100px. - If there are any pixels remaining after calculation and there are no relative frames in the cols or rows definition they will be added to the last frame. For example if the frameset is 301 pixels and we have the following definition: 100,100,100, the resulting frames will be calculated as 100px,100px,101px. Previously this was 100px,100px,100px.
Created attachment 2497 [details] Rewrite of frame calculation Fixed some whitespace issues
Created attachment 2498 [details] layout-tests\fast\frames\resize.html
Comment on attachment 2497 [details] Rewrite of frame calculation I do not think this is ready for production use yet. Although everything looks great, I want to perform a lot more tests first.
Comment on attachment 2497 [details] Rewrite of frame calculation OK, sounds fine. Please re-set the review? flag when you think the patch, includings tests, is ready.
Created attachment 2525 [details] layout-tests\fast\frames\calculate-fixed.html
Created attachment 2526 [details] layout-tests\fast\frames\calculate-percentage.html
Created attachment 2527 [details] layout-tests\fast\frames\calculate-relative.html
Created attachment 2528 [details] layout-tests\fast\frames\calculate-order.html
Created attachment 2529 [details] layout-tests\fast\frames\calculate-round.html
Comment on attachment 2497 [details] Rewrite of frame calculation Patch seems fine in it's current form. Attached several layout-tests seperately.
This patch has been waiting for review for some time, probably because it's hard to judge if it is truly correct, given the patch and test cases. It is not very obvious looking at a test result whether it is right or not.
A website this affects is www.idokan.de (see http://bugs.kde.org/show_bug.cgi?id=60113)
Another site affected by this patch is a project management suite called Software Planner. It used to work correctly with WebKit up to 312, but doesn't work (in different ways) with 412.2 or ToT with this patch applied. Steps to reproduce: 1. Go to <http://www.pragmaticsw.com/SoftwarePlanner.asp> 2. Click on "Try Sample Database" 3. Click "Logon" Results: - (stock 412.2): the contents briefly renders correctly, then the left frame expands, completely covering the right one - (ToT with this patch): the contents briefly renders correctly, then the left frame expands, taking half of the page. Firefox works with this site OK.
I'm afraid I got it wrong - this patch doesn't seem to affect Software Planner (the change already happened in ToT). Filed as bug 4888.
Comment on attachment 2497 [details] Rewrite of frame calculation This deals w/ the render tree and is best handled by hyatt.
I developed this patch based on Konqueror on a Linux machine and ported it Webkit. Unfortunatly I did not own an OS X capable Mac, so I was not able to properly test this patch. Luckily I was able to order a brand new G5 earlier today. Once I receive it and find some time to properly configure it I will take a new look at this bug (sorry for the bugspam...).
Created attachment 4010 [details] Clean patch, including layout-tests I've cleaned up the patch a bit (some bitrot occurred since originally submitting it) and fixed one remaining problem. I've also generously added comments to make it more clear for review what I am doing precisely. The patch also includes layout-tests and the expected results. This patch breaks three other frame layout-tests. This is unavoidable because the algorithm that calculates the width and height of these frames is rewritten. New expected results for these tests are also included in the patch.
Comment on attachment 4010 [details] Clean patch, including layout-tests Dave Hyatt needs to review this.
I think the key here (besides the actual code review) is making sure that the results on the test cases match Windows Internet Explorer.
It is currently not possible to match the behavoir of IE completely, because we use integers and IE uses floats (bug #3591). Aside from this minor issue, the behavoir of this patch is as close as we are going to get to IE. There are no visual differences and all testcases are rendered in the same way. In one case, where IE and Firefox disagreed about a particular interpretation, I based the behavoir of the patch on IE.
Comment on attachment 4010 [details] Clean patch, including layout-tests Code looks ok, but we have to make sure the pixel differences in existing tests are accounted for. Can we compare with WinIE to make sure?
The changes to the existing layout tests can be easily explained and are the direct result of the improved frame calculation. The older values were simply incorrect and inconsistant with Internet Explorer. The new values are an improvement, but in some instances still different from IE. The reason for this is that we do our math with integers, while IE uses floats. A seperate bug was filed for this. The differences are usually not more than a single pixel, which is negligable because IE and Safari use different borders around the frames, which impacts the calculations. I'll try to explain the differences based on 'valid-expected.txt'. The explaination is also valid for 'invalid-expected.txt' because despite the differences in HTML code both files result in exactly the same expected results. @@ -68,21 +68,21 @@ layer at (0,0) size 800x600 Frameset definition: "1%,10%,100%" Previous calculation: 5px, 53px, 534px Current calculation: 4px, 53px, 535px Change: the first frame is one pixel less high, the last frame is one pixel higher. Reason: Values are rounded differently. The current value is different from IE, but so was the old value. Until we start using floats for calculating frames (like IE does) we will keep running into these little differences. @@ -112,9 +112,9 @@ layer at (0,0) size 800x600 Frameset definition: "*,*,*" Previous calculation: 197px, 197px, 197px Current calculation: 197px, 197px, 198px Change: the last frame is one pixel higher. Reason: the available space is 592px. Divided by three this results in three frames with a height of 197px. This means that there is one pixel left over. The old calculation didn't use that pixel. Just like IE we now add it to the last column. @@ -131,9 +131,9 @@ layer at (0,0) size 800x600 Frameset definition: "*,*,*" Previous calculation: 197px, 197px, 197px Current calculation: 197px, 197px, 198px Change: the last frame is one pixel higher. Reason: the available space is 592px. Divided by three this results in three frames with a height of 197px. This means that there is one pixel left over. The old calculation didn't use that pixel. Just like IE we now add it to the last column. @@ -175,28 +175,28 @@ layer at (0,0) size 800x600 Frameset definition: "*,*,*,*,*,*,*,*,*,*,*" Previous calculation: 11 frames of 69px Current calculation: 10 frames of 69px and 1 frame of 70px Change: the last frame is one pixel wider. Reason: the available space is 760px. Divided by 11 this results in 11 frames with a width of 69px. This means that there is one pixel left over. The old calculation didn't use that pixel. Just like IE we now add it to the last column.