Summary: | Frame's noResize attribute can not be set by JavaScript | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brett Wilson (Google) <brettw> | ||||||||||||||||||||||
Component: | DOM | Assignee: | Daniel Bates <dbates> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, chrome, dbates, emacemac7, eric, techrazy.yang | ||||||||||||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Bug Depends on: | 57682, 58700 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Brett Wilson (Google)
2007-08-01 09:03:28 PDT
Created attachment 15775 [details]
Main example file
Created attachment 15776 [details]
Subframe 1
Created attachment 15777 [details]
Subframe 2
Sounds easy to fix. 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). Created attachment 18494 [details]
First attempt (no test cases!)
WebCore/html/HTMLFrameElementBase.cpp | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
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. 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.
The patch wasn't actually up for review, but I'm very glad to have the feedback! :) 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.
Created attachment 27748 [details]
fix some indent problem
Comment on attachment 27748 [details]
fix some indent problem
Could you please add an automated test to the patch?
Created attachment 27851 [details]
Patch (with automated testcase)
Add a automated testcase.
Created attachment 27859 [details]
A better version
Detect whether the parentNode is a FrameSet...
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.
*** Bug 38781 has been marked as a duplicate of this bug. *** > 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>. Created attachment 87056 [details]
Patch and test cases
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. (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. Committed r82712: <http://trac.webkit.org/changeset/82712> Rolled out in http://trac.webkit.org/changeset/82747 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. Committed r86390: <http://trac.webkit.org/changeset/86390> |