Bug 14845 – Frame's noResize attribute can not be set by JavaScript

• Component: DOM
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

Setting frame.noResize=true has no effect in WebKit. The attribute works when specified in the source HTML.

Since this requires frames, I will attach several files for a testcase. Save them in the same directory and open dom_frame_noresize.html. Pressing the buttons should enable and disable resizing as it does in IE and Firefox.

[Brett Wilson (Google)]

Created attachment 15775 [details]
Main example file

[Brett Wilson (Google)]

Created attachment 15776 [details]
Subframe 1

[Brett Wilson (Google)]

Created attachment 15777 [details]
Subframe 2

[Dave Hyatt]

Sounds easy to fix.

[Eric Seidel (no email)]

Wow.  If you look at:
void HTMLFrameElementBase::parseMappedAttribute(MappedAttribute *attr)

You'll see 10 or so FIXMEs about how none of the frame properties are editable from JS (after the frame renderer is created).

[Eric Seidel (no email)]

Created attachment 18494 [details]
First attempt (no test cases!)

 WebCore/html/HTMLFrameElementBase.cpp |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

[Eric Seidel (no email)]

Ok, I've tested my first attempt with Marv's test cases.  This patch does not fix the bug, due to some strange behavior in setResize(bool), it either sets the attribute to 0 or to "", the "".  Clicking the "resize" button in marv's test case disables resize with the patch applied.

I'll come back and fix the broken js behavior.  However for this patch to move further, I need to create a bunch of test cases to test behavior of all these various attributes.  For example, should frame.noResize = "false"; really remove the "noResize" attribute from the element?  Does we'll need to test IE and FF to make sure.

[Dave Hyatt]

Comment on attachment 18494 [details]
First attempt (no test cases!)

I don't think setting margin width or height on the view has any effect after it has been created, but I could be wrong.

The scrolling code is wrong.  You're getting the page(), which is the top-level frame only.  So you're not really changing the scrollbar state on subframes.

[Eric Seidel (no email)]

The patch wasn't actually up for review, but I'm very glad to have the feedback! :)

[Bo Yang]

Created attachment 27718 [details]
First attempt to fix the frame resize bug

Add some new public method for HTMLFrameSetElement/RenderFrameSet, but I didn't import new dependence of the rendering system.

[Bo Yang]

Created attachment 27748 [details]
fix some indent problem

[Alexey Proskuryakov]

Comment on attachment 27748 [details]
fix some indent problem

Could you please add an automated test to the patch?

[Bo Yang]

Created attachment 27851 [details]
Patch (with automated testcase)

Add a automated testcase.

[Bo Yang]

Created attachment 27859 [details]
A better version

Detect whether the parentNode is a FrameSet...

[Eric Seidel (no email)]

Comment on attachment 27859 [details]
A better version

 130         if (attached() && parentNode()->renderer()->isFrameSet()) {
 131             HTMLFrameSetElement* fs = static_cast<HTMLFrameSetElement*>(parentNode());

seems like an unsafe check.  Just because the renderer() is a frameset, doesn't necessarily mean the Element is.  I think you should check hasTagName instead.

I've never seen this convention before:
m_noResize = !attr->isNull();

I didn't realize that parseMappedAttribute was called when an attribute was removed?

I also don't see how attr->isNull() will return true when noResize="false"?

The test case would be better if it printed PASS or FAIL instead of just printing the resulting metrics.

r- for the attr->isNull() issue.  I'm confused as to if that code actually works.

[Alexey Proskuryakov]

*** Bug 38781 has been marked as a duplicate of this bug. ***

[Daniel Bates]

> I've never seen this convention before:
> m_noResize = !attr->isNull();

This conventions comes up when handling HTML boolean attributes (e.g. disabled, checked). By section 2.5.2 of the HTML5 spec <http://www.w3.org/TR/html5/common-microsyntaxes.html#boolean-attributes> the "presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value."

> I didn't realize that parseMappedAttribute was called when an attribute was removed?

Yes, it is called when an attribute is removed. One such call chain is via Element.removeAttribute():

WebCore::jsElementPrototypeFunctionRemoveAttribute() => WebCore::Element::removeAttribute() => WebCore::NamedNodeMap::removeNamedItem(const String& name, ExceptionCode& ec) => NamedNodeMap::removeNamedItem(const QualifiedName& name, ExceptionCode& ec) => WebCore::NamedNodeMap::removeAttribute() =>  WebCore::StyledElement::attributeChanged() => WebCore::HTMLFrameElement::parseMappedAttribute()

Similar call chains exist when removing the attribute via the Web Inspector.

> 
> I also don't see how attr->isNull() will return true when noResize="false"?

It should not return true. We want to disallow frame resize for this case by section 2.5.2 of the HTML5 spec <http://www.w3.org/TR/html5/common-microsyntaxes.html#boolean-attributes>.

[Daniel Bates]

Created attachment 87056 [details]
Patch and test cases

[Darin Adler]

Comment on attachment 87056 [details]
Patch and test cases

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

OK as-is. Suggestions for further improvement.

> Source/WebCore/html/HTMLFrameElement.cpp:90
> +        m_noResize = !attr->isNull(); // This will evaluate to false when frame.noResize := false, which is what we want.

The reason to cache the existence of an attribute in a boolean local is performance; I do not think this code is performance critical and hence I don’t think m_noResize is a good idea.

I suggest we make the noResize() function non-inline and change it to call hasAttribute. This eliminates the need for an m_noResize data member.

I don’t understand the syntax "when frame.noResize := false" so I don’t know what your comment is saying.

> Source/WebCore/html/HTMLFrameElement.cpp:95
> +        if (HTMLFrameSetElement* frameSetElement = containingFrameSetElement(this)) {
> +            RenderObject* frameSetRenderer = frameSetElement->renderer();
> +            if (frameSetRenderer && frameSetRenderer->isFrameSet())
> +                toRenderFrameSet(frameSetRenderer)->computeEdgeInfo();
> +        }

The division of labor here seems wrong. The RenderFrame is the class that knows that edge info is based on noResize and hasFrameBorder, but now the HTML frame element is responsible for calling directly to the RenderFrameSet. I don’t think that RenderFrame should be half responsible for this. Instead it should be entirely responsible.

I think that here in HTMLFrameElement we should be calling updateFromRenderer on RenderFrame and RenderFrame in turn should be turning around and calling the RenderFrameSet to say that its edge info changed and then the RenderFrameSet, based on the fact that a frame called to say its edge info changed, will decide to recompute the edge info.

[Daniel Bates]

(In reply to comment #19)
> (From update of attachment 87056 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87056&action=review
> [...]
> I suggest we make the noResize() function non-inline and change it to call hasAttribute. This eliminates the need for an m_noResize data member.

Will remove instance variable m_noResize and make noResize() a non-inline  function.

> 
> I don’t understand the syntax "when frame.noResize := false" so I don’t know what your comment is saying.

The comment was trying to explain that the expression "!attr->isNull()" handles both the HTML attribute logic (when the HTML attribute is added and removed) as well as the JavaScript noResize property logic because the JavaScript-binding/DOM logic translates the noResize property setter to DOM attribute addition and removal (depending on the value being set).

> 
> > Source/WebCore/html/HTMLFrameElement.cpp:95
> > +        if (HTMLFrameSetElement* frameSetElement = containingFrameSetElement(this)) {
> > +            RenderObject* frameSetRenderer = frameSetElement->renderer();
> > +            if (frameSetRenderer && frameSetRenderer->isFrameSet())
> > +                toRenderFrameSet(frameSetRenderer)->computeEdgeInfo();
> > +        }
> [...]
> I think that here in HTMLFrameElement we should be calling updateFromRenderer on RenderFrame and RenderFrame in turn should be turning around and calling the RenderFrameSet to say that its edge info changed and then the RenderFrameSet, based on the fact that a frame called to say its edge info changed, will decide to recompute the edge info.

I like this approach. Will change.

[Daniel Bates]

Committed r82712: <http://trac.webkit.org/changeset/82712>

[Adam Barth]

Rolled out in http://trac.webkit.org/changeset/82747

[Daniel Bates]

Created attachment 89886 [details]
Patch and layout tests

Added check for needsLayout() in RenderFrameSet::notifyFrameEdgeInfoChanged().

We need to fix bug #58700 before we can land this patch.

[Daniel Bates]

Committed r86390: <http://trac.webkit.org/changeset/86390>