Cannot resize frames because frameborder=0

• Component: Frames
• Status: RESOLVED FIXED

[jasneet]

I Steps:
you need to have a valid email account or contact www.ipswitch.com

II Issue:
when logged in to your email account, you can not change the sizing on 
the panes : up/down

III Conclusion:
I noticed that when the frameborder=0, the frame cannot be resized in webkit although scrolling is set to "auto". If I change the code to have a frameborder of say 1, then the frame becomes resizable.

IV Other Browsers:
IE7: ok
FF3: ok

V Nightly tested: 40471

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=5448

[jasneet]

Created attachment 27336 [details]
testcase

[Swapna]

Created attachment 138944 [details]
Patch to allow resize of frames even in case of frameborder=0.

Hi,
In this patch removed check for border, in order to allow resize of frames even in case of frameborder=0.
I didn't attach any test case. Can any one please review the patch and also suggest which type of test case is needed. 
So that I will submit another patch along with test case. Thank you in advance.

[Alexey Proskuryakov]

Comment on attachment 138944 [details]
Patch to allow resize of frames even in case of frameborder=0.

Please add an automated regression test for this, or explain why that's not possible.

[Swapna]

Created attachment 139434 [details]
Patch to allow resize of frames even in case of frameborder=0. - II

Patch along with automated regression test case. Can any one please review the patch. Thanks in advance.

[Kenneth Rohde Christiansen]

Comment on attachment 139434 [details]
Patch to allow resize of frames even in case of frameborder=0. - II

View in context: https://bugs.webkit.org/attachment.cgi?id=139434&action=review

> Source/WebCore/ChangeLog:9
> +     

Any link to what the spec says about this? I mean there must be a reason for the original code

> Source/WebCore/rendering/RenderFrameSet.cpp:671
> -    if (split == noSplit || !axis.m_allowBorder[split] || axis.m_preventResize[split]) {
> +    if (split == noSplit || axis.m_preventResize[split]) {

Please make sure that this is not breaking frame flattening in any way

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:8
> +            if (window.layoutTestController)
> +                layoutTestController.dumpAsText();
> +
> +            function log(frame, success, isWidth, size) {
> +              if (window.layoutTestController) {

inconsistent indentation

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:9
> +                alert(frame.name + ' resized correctly = ' + (success ? 'true' : 'false'));

We normally write PASSED or FAILED, but true, false. Please check how other tests are made

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:24
> +                if (!window.layoutTestController)

You have these tests all over. Why not rename init() to run() or so and add this test as the first thing.

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:30
> +                    //Move the One/* vertical resizer ten pixels west...

Please add a space after // and just one punctuation mark at the end

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:49
> +            function checkSuccess() {
> +                    log(One, One.frameElement.width == 100, true, 100)
> +                    log(Two, Two.frameElement.height == 80, false, 80)

very inconsistent indentation again

[Swapna]

Created attachment 140204 [details]
Patch to allow resize of frames even in case of frameborder=0. - III

This is the patch after incorporating review comments given for patch-II.

[Swapna]

(In reply to comment #5)
> (From update of attachment 139434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139434&action=review
> > Source/WebCore/ChangeLog:9
> > +     
> Any link to what the spec says about this? I mean there must be a reason for the original code
The specs say nothing about 'existence of frame border' Vs 'frame resizing'. Spec talks only about visibility of frameborder on having values like frameboder=0 and frameborder=1. And for reference FF and IE both allow resizing of the frames while frameborder=0. I suppose, it should be user friendly if we allow resize of frames even in case of frameborder=0.
interesting link:
http://stackoverflow.com/questions/7093298/allow-resizing-of-html-frames-while-adding-frameborder-0-attribute

> 
> > Source/WebCore/rendering/RenderFrameSet.cpp:671
> > -    if (split == noSplit || !axis.m_allowBorder[split] || axis.m_preventResize[split]) {
> > +    if (split == noSplit || axis.m_preventResize[split]) {
> 
> Please make sure that this is not breaking frame flattening in any way
It is not breaking frame flattening . For instance, My changes are in RenderFrameSet::startResizing , and it is getting called from RenderFrameSet::userResize.
In this RenderFrameSet::userResize function if frameFlatteningEnabled = true, it will return false and rest of the code won't be executed.


> > LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:8
> > +            if (window.layoutTestController)
> > +                layoutTestController.dumpAsText();
> > +
> > +            function log(frame, success, isWidth, size) {
> > +              if (window.layoutTestController) {
> 
> inconsistent indentation
> > LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:9
> > +                alert(frame.name + ' resized correctly = ' + (success ? 'true' : 'false'));
> 
> We normally write PASSED or FAILED, but true, false. Please check how other tests are made
> 
> > LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:24
> > +                if (!window.layoutTestController)
> 
> You have these tests all over. Why not rename init() to run() or so and add this test as the first thing.
> 
> > LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:30
> > +                    //Move the One/* vertical resizer ten pixels west...
> 
> Please add a space after // and just one punctuation mark at the end
> 
> > LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:49
> > +            function checkSuccess() {
> > +                    log(One, One.frameElement.width == 100, true, 100)
> > +                    log(Two, Two.frameElement.height == 80, false, 80)
> 
> very inconsistent indentation again

Did changes as per review comments in patch-III

[Swapna]

Can some one please review the patch.

[Eric Seidel (no email)]

Comment on attachment 140204 [details]
Patch to allow resize of frames even in case of frameborder=0. - III

This seems reasonable to me.  I wouldn't have used alerts for logging as they make the test painful to use manually.  You also seem to have a bunch of code in your test case which is not executed.  The code change is probably fine, but the test case needs work.

[Swapna]

Created attachment 141332 [details]
Patch to allow resize of frames even in case of frameborder=0. - IV

> This seems reasonable to me.  I wouldn't have used alerts for logging as they make the test painful to use manually. 
Removed usage of alerts for logging.

> You also seem to have a bunch of code in your test case which is not executed.
 In the test case the code mentioned under if(window.layoutTestController) is to be executed in layout test tool. And the rest code is to execute in browser. By this code, while we run the test case in browser it will give look and feel of resize of frames.

[Swapna]

Hi Eric,
Can you please review the patch. Thanks in advance.

[Eric Seidel (no email)]

Comment on attachment 141332 [details]
Patch to allow resize of frames even in case of frameborder=0. - IV

View in context: https://bugs.webkit.org/attachment.cgi?id=141332&action=review

Does this match other browsers?

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:1
> +<html>

Normally you would want an HTML5 doctype here.  <!DOCTYPE html>

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:43
> +                    document.getElementById("results").contentDocument.getElementById("console").appendChild(document.createTextNode((One.frameElement.width == 100 ? "PASS: "  : "FAIL: " ) + "frame'One' resized correctly\n"));
> +                    document.getElementById("results").contentDocument.getElementById("console").appendChild(document.createTextNode((Two.frameElement.height == 80 ? "PASS: "  : "FAIL: ") + "frame'Two' resized correctly"));

Don't you ahve a log function to do this?  I guess your log funciton isn't general purpose.

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:53
> +            <frame frameborder=0 name="One" style="border-right: 2px solid green;" />

self-closing tags are invalid html.

[Eric Seidel (no email)]

I am not really the right reviewer for this.  YOu need to convince us that you're making our behavior match other browsers.  The bug you mention says that Safari passes this test... but chrome fails?  If so, why is this WebKIt change needed?

[Swapna]

Created attachment 141712 [details]
Patch to allow resize of frames even in case of frameborder=0.

> LayoutTests/fast/frames/frames-with-frameborder-zero-can-be-resized.html:1
> +<html>
Normally you would want an HTML5 doctype here.  <!DOCTYPE html>

Added <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Frameset//EN"> as doctype.
Since <frame>&<frameset> tags are not supported in HTML5, we can't use HTML5 doctype.

>Don't you ahve a log function to do this?  I guess your log funciton isn't general purpose.
Generalized log function.

> +  <frame frameborder=0 name="One" style="border-right: 2px solid green;" />
self-closing tags are invalid html.
Fixed.

[Swapna]

(In reply to comment #13)
> I am not really the right reviewer for this.  YOu need to convince us that you're making our behavior match other browsers.  The bug you mention says that Safari passes this test... but chrome fails?  If so, why is this WebKIt change needed?

Thank you for your review comments.
I tested this on various browsers and the result is as follows:
1. IE-9        PASS
2. FF-12.0     PASS
3. safari-5.1  FAIL
4. chrome-18   FAIL

As I already mentioned in Comment #7, I suppose it should be user friendly if we support resize of frames even in case of frameborder=0 same as IE & FF. Let me know your opnion. Thank you.

[Eric Seidel (no email)]

Comment on attachment 141712 [details]
Patch to allow resize of frames even in case of frameborder=0.

OK.

[WebKit Review Bot]

Comment on attachment 141712 [details]
Patch to allow resize of frames even in case of frameborder=0.

Clearing flags on attachment: 141712

Committed r116978: <http://trac.webkit.org/changeset/116978>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.