Bug 3587 – Frameset size calculation does not resize or distribute evenly

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Niels Leenheer (HTML5test)]

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

[Niels Leenheer (HTML5test)]

Created attachment 2419 [details]
Testcase (Fixed frames behavoir)

[Niels Leenheer (HTML5test)]

Created attachment 2420 [details]
Testcase (0% frames behavoir)

[Niels Leenheer (HTML5test)]

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.

[Joost de Valk (AlthA)]

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. 

[Niels Leenheer (HTML5test)]

Created attachment 2458 [details]
Rewrite of frame calculation

[Niels Leenheer (HTML5test)]

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.

[Niels Leenheer (HTML5test)]

Created attachment 2497 [details]
Rewrite of frame calculation

Fixed some whitespace issues

[Niels Leenheer (HTML5test)]

Created attachment 2498 [details]
layout-tests\fast\frames\resize.html

[Niels Leenheer (HTML5test)]

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.

[Darin Adler]

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.

[Niels Leenheer (HTML5test)]

Created attachment 2525 [details]
layout-tests\fast\frames\calculate-fixed.html

[Niels Leenheer (HTML5test)]

Created attachment 2526 [details]
layout-tests\fast\frames\calculate-percentage.html

[Niels Leenheer (HTML5test)]

Created attachment 2527 [details]
layout-tests\fast\frames\calculate-relative.html

[Niels Leenheer (HTML5test)]

Created attachment 2528 [details]
layout-tests\fast\frames\calculate-order.html

[Niels Leenheer (HTML5test)]

Created attachment 2529 [details]
layout-tests\fast\frames\calculate-round.html

[Niels Leenheer (HTML5test)]

Comment on attachment 2497 [details]
Rewrite of frame calculation

Patch seems fine in it's current form. Attached several layout-tests
seperately.

[Maciej Stachowiak]

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.

[Maks Orlovich]

A website this affects is www.idokan.de (see 
http://bugs.kde.org/show_bug.cgi?id=60113) 
 

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

Comment on attachment 2497 [details]
Rewrite of frame calculation

This deals w/ the render tree and is best handled by hyatt.

[Niels Leenheer (HTML5test)]

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...).

[Niels Leenheer (HTML5test)]

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.

[Darin Adler]

Comment on attachment 4010 [details]
Clean patch, including layout-tests

Dave Hyatt needs to review this.

[Maciej Stachowiak]

I think the key here (besides the actual code review) is making sure that the results on the test cases 
match Windows Internet Explorer.

[Niels Leenheer (HTML5test)]

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.

[Dave Hyatt]

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?

[Niels Leenheer (HTML5test)]

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.