Bug 3587 - Frameset size calculation does not resize or distribute evenly
Summary: Frameset size calculation does not resize or distribute evenly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 412
Hardware: All All
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-06-17 00:55 PDT by Niels Leenheer (HTML5test)
Modified: 2005-10-24 20:45 PDT (History)
1 user (show)

See Also:


Attachments
Testcase (Fixed frames behavoir) (97 bytes, text/html)
2005-06-17 00:58 PDT, Niels Leenheer (HTML5test)
no flags Details
Testcase (0% frames behavoir) (93 bytes, text/html)
2005-06-17 00:58 PDT, Niels Leenheer (HTML5test)
no flags Details
Rewrite of frame calculation (6.42 KB, patch)
2005-06-18 06:55 PDT, Niels Leenheer (HTML5test)
no flags Details | Formatted Diff | Diff
Rewrite of frame calculation (6.43 KB, patch)
2005-06-20 06:20 PDT, Niels Leenheer (HTML5test)
no flags Details | Formatted Diff | Diff
layout-tests\fast\frames\resize.html (960 bytes, text/html)
2005-06-20 06:21 PDT, Niels Leenheer (HTML5test)
no flags Details
layout-tests\fast\frames\calculate-fixed.html (1.81 KB, text/html)
2005-06-21 04:12 PDT, Niels Leenheer (HTML5test)
no flags Details
layout-tests\fast\frames\calculate-percentage.html (1.22 KB, text/html)
2005-06-21 04:13 PDT, Niels Leenheer (HTML5test)
no flags Details
layout-tests\fast\frames\calculate-relative.html (1020 bytes, text/html)
2005-06-21 04:13 PDT, Niels Leenheer (HTML5test)
no flags Details
layout-tests\fast\frames\calculate-order.html (628 bytes, text/html)
2005-06-21 04:14 PDT, Niels Leenheer (HTML5test)
no flags Details
layout-tests\fast\frames\calculate-round.html (1.56 KB, text/html)
2005-06-21 04:15 PDT, Niels Leenheer (HTML5test)
no flags Details
Clean patch, including layout-tests (83.71 KB, patch)
2005-09-22 12:23 PDT, Niels Leenheer (HTML5test)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Niels Leenheer (HTML5test) 2005-06-17 00:55:18 PDT
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
Comment 1 Niels Leenheer (HTML5test) 2005-06-17 00:58:01 PDT
Created attachment 2419 [details]
Testcase (Fixed frames behavoir)
Comment 2 Niels Leenheer (HTML5test) 2005-06-17 00:58:44 PDT
Created attachment 2420 [details]
Testcase (0% frames behavoir)
Comment 3 Niels Leenheer (HTML5test) 2005-06-17 00:59:51 PDT
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.
Comment 4 Joost de Valk (AlthA) 2005-06-17 01:11:30 PDT
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. 
Comment 5 Niels Leenheer (HTML5test) 2005-06-18 06:55:09 PDT
Created attachment 2458 [details]
Rewrite of frame calculation
Comment 6 Niels Leenheer (HTML5test) 2005-06-20 06:14:27 PDT
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.
Comment 7 Niels Leenheer (HTML5test) 2005-06-20 06:20:45 PDT
Created attachment 2497 [details]
Rewrite of frame calculation

Fixed some whitespace issues
Comment 8 Niels Leenheer (HTML5test) 2005-06-20 06:21:59 PDT
Created attachment 2498 [details]
layout-tests\fast\frames\resize.html
Comment 9 Niels Leenheer (HTML5test) 2005-06-20 06:25:11 PDT
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 10 Darin Adler 2005-06-20 08:51:31 PDT
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.
Comment 11 Niels Leenheer (HTML5test) 2005-06-21 04:12:49 PDT
Created attachment 2525 [details]
layout-tests\fast\frames\calculate-fixed.html
Comment 12 Niels Leenheer (HTML5test) 2005-06-21 04:13:28 PDT
Created attachment 2526 [details]
layout-tests\fast\frames\calculate-percentage.html
Comment 13 Niels Leenheer (HTML5test) 2005-06-21 04:13:57 PDT
Created attachment 2527 [details]
layout-tests\fast\frames\calculate-relative.html
Comment 14 Niels Leenheer (HTML5test) 2005-06-21 04:14:36 PDT
Created attachment 2528 [details]
layout-tests\fast\frames\calculate-order.html
Comment 15 Niels Leenheer (HTML5test) 2005-06-21 04:15:05 PDT
Created attachment 2529 [details]
layout-tests\fast\frames\calculate-round.html
Comment 16 Niels Leenheer (HTML5test) 2005-06-27 01:01:10 PDT
Comment on attachment 2497 [details]
Rewrite of frame calculation

Patch seems fine in it's current form. Attached several layout-tests
seperately.
Comment 17 Maciej Stachowiak 2005-07-24 16:47:07 PDT
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.
Comment 18 Maks Orlovich 2005-08-21 16:11:51 PDT
A website this affects is www.idokan.de (see 
http://bugs.kde.org/show_bug.cgi?id=60113) 
 
Comment 19 Alexey Proskuryakov 2005-08-22 10:39:39 PDT
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.
Comment 20 Alexey Proskuryakov 2005-09-08 11:39:45 PDT
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 21 Eric Seidel (no email) 2005-09-17 14:13:13 PDT
Comment on attachment 2497 [details]
Rewrite of frame calculation

This deals w/ the render tree and is best handled by hyatt.
Comment 22 Niels Leenheer (HTML5test) 2005-09-19 06:33:39 PDT
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...).
Comment 23 Niels Leenheer (HTML5test) 2005-09-22 12:23:36 PDT
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 24 Darin Adler 2005-09-23 09:08:30 PDT
Comment on attachment 4010 [details]
Clean patch, including layout-tests

Dave Hyatt needs to review this.
Comment 25 Maciej Stachowiak 2005-09-23 18:21:15 PDT
I think the key here (besides the actual code review) is making sure that the results on the test cases 
match Windows Internet Explorer.
Comment 26 Niels Leenheer (HTML5test) 2005-10-06 00:42:27 PDT
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 27 Dave Hyatt 2005-10-17 20:41:07 PDT
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?
Comment 28 Niels Leenheer (HTML5test) 2005-10-18 03:51:46 PDT
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.