Remove nonstandard noresize attribute from HTML FrameSet Element

[Daniel Bates]

Neither the HTML 4.01 nor HTML5 specs define a noresize attribute for the HTML FrameSet element. From briefly searching Google and MSDN, I can't find any documentation that references this non-standard attribute.

HTML 4.01: <http://www.w3.org/TR/html401/present/frames.html#h-16.2.1>
HTML5: <http://www.w3.org/TR/html5/obsolete.html#frameset>
MSDN "NORESIZE Attribute/noResize Property": <http://msdn.microsoft.com/en-us/library/ms534194(VS.85).aspx> (Notice, the MSDN documentation doesn't apply to the HTML FrameSet element)

[Darin Adler]

Did you test it? Does it work in IE?

[Daniel Bates]

(In reply to comment #1)
> Did you test it? Does it work in IE?

Yes, I've tested in IE 6 and IE 5 (*), and IE 7 and it doesn't modify frame resize behavior.

(*) I tested IE 6 and IE 5 using the application IETester (http://www.my-debugbar.com/wiki/IETester/HomePage). When I have a chance, I will look to confirm this using an instance of Windows XP with IE 6.

[Daniel Bates]

Created attachment 87872 [details]
Test case

[Darin Adler]

OK, well removing this should be easy, and it seems unlikely there are people with frameset content relying on this WebKit-only behavior, even in the usual places we find them, like Dashboard and Mac OS X applications.

[Karthik Sarap]

Created attachment 97573 [details]
Propose Patch

#1>	Currently we have made the HTMLFrameSetElement::m_noresize = false so that it ignores the "noresize" attributes value being mentioned in the html page & always allow the frames to be resized.

There are two other ways it could have been resolved as described below:

#2>	We can remove the HTMLFrameSetElement::m_noresize & HTMLFrameSetElement::noResize(). But this will give problem for FrameEdgeInfo which is being used for both RenderFrame & RenderFrameSet, therefore we can't remove the m_preventResize from FrameEdgeInfo.[noresize is a standard attribute for frame element which needs to be respected]

We can continue to use FrameEdgeInfo for RenderFrame & introduce one more struct FrameSetEdgeInfo {}, which will only contain m_allowBorder & NOT m_preventResize.

#3>	If we don't want to introduce any struct as discussed above, we can still maintain same FrameEdgeInfo. But we have to remove RenderFrameSet::GridAxis::m_preventResize & modify RenderFrameSet::fillFromEdgeInfo() accordingly so that we don't set the m_preventResize for RenderFrameSet::m_rows & RenderFrameSet::m_cols.

Considering the fact that the code changes for #2 Or #3 discussed above would be little more, we have implemented #1. Any suggestions/comments are always welcome.

[WebKit Review Bot]

Comment on attachment 97573 [details]
Propose Patch

Attachment 97573 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8874521

New failing tests:
fast/frames/frame-inherit-noresize-from-frameset.html

[WebKit Review Bot]

Created attachment 97575 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Karthik Sarap]

Created attachment 97577 [details]
Fixed test case issue reported by Chromium EWS bot

Re-organized the test cases being added to fix the issue reported by chromium ews bot

[WebKit Review Bot]

Comment on attachment 97577 [details]
Fixed test case issue reported by Chromium EWS bot

Attachment 97577 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8877548

New failing tests:
fast/frames/frameset_ignore_noresize.html
fast/frames/frame-inherit-noresize-from-frameset.html

[WebKit Review Bot]

Created attachment 97579 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Karthik Sarap]

Comment on attachment 97577 [details]
Fixed test case issue reported by Chromium EWS bot

cancelling the review as the chromium fails & we upload the new patch soon.

[Karthik Sarap]

Comment on attachment 97577 [details]
Fixed test case issue reported by Chromium EWS bot

cancelling the review as the chromium fails & we upload the new patch soon.

[Mustafizur Rahaman (rahaman)]

Created attachment 97955 [details]
Patch after fixing test case issue

Hopefully everything is correct this time.

[WebKit Review Bot]

Comment on attachment 97955 [details]
Patch after fixing test case issue

Attachment 97955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8910907

New failing tests:
fast/frames/frameset_ignore_noresize.html

[WebKit Review Bot]

Created attachment 97961 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mustafizur Rahaman (rahaman)]

Created attachment 97966 [details]
Patch

I really want that feature in bugzilla where I can update my patch without sending email across. Sorry people for spamming your email box with repeated faulty patches :-(

[WebKit Review Bot]

Comment on attachment 97966 [details]
Patch

Attachment 97966 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8913900

New failing tests:
fast/frames/frameset_ignore_noresize.html

[WebKit Review Bot]

Created attachment 97972 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mustafizur Rahaman (rahaman)]

Created attachment 97976 [details]
Patch v2

Still fixing the chromium tests issue. In the last patch there was a diff of one trailing line mismatch, fixed it in this patch.

[Darin Adler]

Comment on attachment 97976 [details]
Patch v2

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

> Source/WebCore/html/HTMLFrameSetElement.cpp:100
>      } else if (attr->name() == noresizeAttr) {
> -        m_noresize = true;
> +        m_noresize = false;

This seems like the wrong way to do this. It’s silly to leave code in to parse the noresize attribute just to set this internal boolean to false, since it’s already false and will always be false. You could just as easily have removed this clause from parseMappedAttribute entirely.

[Darin Adler]

(In reply to comment #5)
> #2>    We can remove the HTMLFrameSetElement::m_noresize & HTMLFrameSetElement::noResize().

You can and should do it this way.

> But this will give problem for FrameEdgeInfo which is being used for both RenderFrame & RenderFrameSet, therefore we can't remove the m_preventResize from FrameEdgeInfo.

That’s no big deal. Just replace the four occurrences frameSet()->noResize() in RenderFrameSet call with false, and you’re done.

[Darin Adler]

Comment on attachment 97976 [details]
Patch v2

Please remove the dead code that deals with HTMLFrameSetElement::m_noresize in your patch.

But further, have we verified that there is no code depending on this non-standard WebKit feature? Did you do any research to try to determine that?

[Mustafizur Rahaman (rahaman)]

(In reply to comment #22)
> (From update of attachment 97976 [details])
> Please remove the dead code that deals with HTMLFrameSetElement::m_noresize in your patch.
> 
> But further, have we verified that there is no code depending on this non-standard WebKit feature? Did you do any research to try to determine that?

Have taken care of Comment #20, #21 & also removed the dead code in the new patch to be updated soon.

Regarding impact of these changes on other code, I have verified that this "noresize" attribute is only used for RenderFrameSet/RenderFrame (and that's why I did not modify anything in RenderFrame). Also, I have verified with a more complex test case of nested Frameset & Frames with various combination of "noresize" and found the behavior to be same as FF behavior.

[Mustafizur Rahaman (rahaman)]

Created attachment 98141 [details]
Patch after incorporating Darin's comments

[Darin Adler]

(In reply to comment #23)
> Regarding impact of these changes on other code, I have verified that this "noresize" attribute is only used for RenderFrameSet/RenderFrame (and that's why I did not modify anything in RenderFrame). Also, I have verified with a more complex test case of nested Frameset & Frames with various combination of "noresize" and found the behavior to be same as FF behavior.

I said code, but I was unclear. I did not mean code inside WebKit. I meant things like Mac OS X applications, iOS applications, and Dashboard widgets depending on WebKit behavior. These are the categories I know about because of my work at Apple. There may be others as well.

I understand that this new behavior matches Firefox.

[Darin Adler]

Comment on attachment 98141 [details]
Patch after incorporating Darin's comments

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

> Source/WebCore/rendering/RenderFrameSet.cpp:170
> -    if (inside && frameSet()->noResize()
> -            && !request.readOnly() && !result.innerNode()) {
> +    if (inside && !request.readOnly() && !result.innerNode()) {
>          result.setInnerNode(node());
>          result.setInnerNonSharedNode(node());
>      }

Since noResize is now always false, you should have removed this entire block, not removed the noResize check.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #25)
> (In reply to comment #23)
> > Regarding impact of these changes on other code, I have verified that this "noresize" attribute is only used for RenderFrameSet/RenderFrame (and that's why I did not modify anything in RenderFrame). Also, I have verified with a more complex test case of nested Frameset & Frames with various combination of "noresize" and found the behavior to be same as FF behavior.
> 
> I said code, but I was unclear. I did not mean code inside WebKit. I meant things like Mac OS X applications, iOS applications, and Dashboard widgets depending on WebKit behavior. These are the categories I know about because of my work at Apple. There may be others as well.
> 

Hi Darin,

I did some research on this, I have come across this link http://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariHTMLRef/Articles/Attributes.html which says "noresize" is used for frame tag. So, I was wondering whether any Apple implementation would use "noresize" for frameset.

Other than this, I did not come across any content on the net which uses "noresize" for frameset.

Please let me know whether based on this info, we can land this patch (then I will update another patch with incorporating your review comments). Else, we can mark this issue as invalid (with appropriate explanation).

On a side note, I have a question (because I am relatively newer in WebKit). In any such kind of fixes, if a legacy app depends on wrong WebKit behavior, then the app would break if we try to fix. Then what is the general philosophy/guideline we normally follow in such issues?

[Darin Adler]

(In reply to comment #27)
> I did some research on this, I have come across this link http://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariHTMLRef/Articles/Attributes.html which says "noresize" is used for frame tag. So, I was wondering whether any Apple implementation would use "noresize" for frameset.

Generally speaking, I don’t think the documentation tells us one way or another what the effect will be.

> Please let me know whether based on this info, we can land this patch (then I will update another patch with incorporating your review comments). Else, we can mark this issue as invalid (with appropriate explanation).
> 
> On a side note, I have a question (because I am relatively newer in WebKit). In any such kind of fixes, if a legacy app depends on wrong WebKit behavior, then the app would break if we try to fix. Then what is the general philosophy/guideline we normally follow in such issues?

There’s no simple answer. There has to be a risk/benefit consideration.

In some cases we take a risk and change behavior. The Apple Safari and WebKit team then has to deal with fallout later, diagnosing problems, tracking them down and figuring out which behavior change led to the symptom. Doing this diagnosis can be quite costly and frustrating, but risking it can be worth it if there is significant cost to keeping the legacy behavior.

Sometimes we end up adding a quirk to keep an old version of the application or website working while giving a path forward for others and newer versions.

In other cases, we keep some a legacy feature, such as a WebKit variant of some standardized feature, in the software long term. While this might frustrate some working on the project who get no benefit from keeping the legacy version of the feature, the cost is often low and bearable.

There may be others working on the project who have more perspective on this.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #28)
> (In reply to comment #27)
> > I did some research on this, I have come across this link http://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariHTMLRef/Articles/Attributes.html which says "noresize" is used for frame tag. So, I was wondering whether any Apple implementation would use "noresize" for frameset.
> 
> Generally speaking, I don’t think the documentation tells us one way or another what the effect will be.
> 
> > Please let me know whether based on this info, we can land this patch (then I will update another patch with incorporating your review comments). Else, we can mark this issue as invalid (with appropriate explanation).
> > 
> > On a side note, I have a question (because I am relatively newer in WebKit). In any such kind of fixes, if a legacy app depends on wrong WebKit behavior, then the app would break if we try to fix. Then what is the general philosophy/guideline we normally follow in such issues?
> 
> There’s no simple answer. There has to be a risk/benefit consideration.
> 
> In some cases we take a risk and change behavior. The Apple Safari and WebKit team then has to deal with fallout later, diagnosing problems, tracking them down and figuring out which behavior change led to the symptom. Doing this diagnosis can be quite costly and frustrating, but risking it can be worth it if there is significant cost to keeping the legacy behavior.
> 
> Sometimes we end up adding a quirk to keep an old version of the application or website working while giving a path forward for others and newer versions.
> 
> In other cases, we keep some a legacy feature, such as a WebKit variant of some standardized feature, in the software long term. While this might frustrate some working on the project who get no benefit from keeping the legacy version of the feature, the cost is often low and bearable.
> 
> There may be others working on the project who have more perspective on this.

Thanks Darin for the perspective, it really helped me understand the challenge/tradeoff in maintaining legacy better.

Based on the discussion, can I request you to please make the bug as INVALID/WONT SOLVE

[Mustafizur Rahaman (rahaman)]

As I don't have privilege to change the status of this bug, can someone please make it WONTFIX/RESOLVED based on the discussion above?