Bug 21638

Summary: WebCore/page/FrameTree.cpp:find() dispatches form submissions to incorrect frame
Product: WebKit Reporter: John Holdsworth <webkit>
Component: FramesAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED    
Severity: Normal CC: ap, cmarcelo, ddkilzer, webkit
Priority: P2 Keywords: EasyFix, GoodFirstBug
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dbeclipse.org/webkit/top.html
Attachments:
Description Flags
Fix for FrameTree.cpp for ambiguous frames.
sam: review-
Test case for frame bug.
none
Revised Fix for FrameTree.cpp for ambiguous frames. ddkilzer: review-

John Holdsworth
Reported 2008-10-16 01:59:47 PDT
Hi, When two frames in a web application have the same name, on form submission the frame first defined on the page is chosen as the target to load rather than the frame "closest" to the submitting form in the frame tree. The other major browsers: Firefox, IE etc use the closest frame. (See the URL http://dbeclipse.org/webkit/top.html as an example - each form should submit into its own enclosing frame) A minor four line fix to FrameTree.cpp shown below is already available and is about to be submitted under this bug report. To replicate create the following files: top.html: <html> <frameset cols='50%,50%'> <frame src='frame.html'> <frame src='frame.html'> </frameset> frame.html: <html> <frameset rows='100,*'> <frame src='form.html'> <frame src='javascript: ""' name='out'> </frameset> form.html: <html><body> <form target=out action='http://www.google.com/search'> Search for: <input name='q' value='webkit'> <input type=submit value='Click me'></form> On opening top.html both forms submit into the left hand side frame. After applying the fix each form submits to the correct frame beneath the form. This results in no regression according to the webkit-tests. This fix would be useful in Web 2.0 applications, for example, those which implement their own tabbed interface and have more than one frame with the same name in different frames. Patch is below: --- WebCore/page/FrameTree.cpp (revision 37618) +++ WebCore/page/FrameTree.cpp (working copy) @@ -183,11 +183,13 @@ Frame* FrameTree::find(const AtomicStrin if (name == "_blank") return 0; - // Search subtree starting with this frame first. - for (Frame* frame = m_thisFrame; frame; frame = frame->tree()->traverseNext(m_thisFrame)) - if (frame->tree()->name() == name) - return frame; - + // Search up tree starting with this frame first. + for (Frame* parent = m_thisFrame; parent; parent = parent->tree()->parent()) + for (Frame* frame = parent; frame; frame = frame->tree()->traverseNext(parent)) + if (frame->tree()->name() == name) + return frame; + + // page search may no longer be required... // Search the entire tree for this page next. Page* page = m_thisFrame->page(); Thanks for all your work. Best Regards, John Holdsworth
Attachments
Fix for FrameTree.cpp for ambiguous frames. (1.57 KB, patch)
2008-10-16 02:07 PDT, John Holdsworth 		sam: review-
DetailsFormatted DiffDiff
Test case for frame bug. (563 bytes, application/octet-stream)
2008-10-16 02:10 PDT, John Holdsworth 		no flags
Details
Revised Fix for FrameTree.cpp for ambiguous frames. (3.76 KB, patch)
2008-10-26 04:13 PDT, John Holdsworth 		ddkilzer: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
John Holdsworth
Comment 1 2008-10-16 02:07:50 PDT
Created attachment 24391 [details] Fix for FrameTree.cpp for ambiguous frames.
John Holdsworth
Comment 2 2008-10-16 02:10:46 PDT
Created attachment 24392 [details] Test case for frame bug.
Sam Weinig
Comment 3 2008-10-24 12:12:10 PDT
Comment on attachment 24391 [details] Fix for FrameTree.cpp for ambiguous frames. > + Frames now searched for by name starting at the innermost frame > + moving up the tree to the outermost containing parent level by level. Please remove tabs. > + // Search up tree starting with this frame first. > + for (Frame* parent = m_thisFrame; parent; parent = parent->tree()->parent()) > + for (Frame* frame = parent; frame; frame = frame->tree()->traverseNext(parent)) > + if (frame->tree()->name() == name) > + return frame; The two for-loops need braces. I realize the old code didn't have them, but they are required for all new code per our style guidelines. r- due to the style issues. Please also include a test with a LayoutTests changelog in the same patch.
John Holdsworth
Comment 4 2008-10-26 04:13:51 PDT
Created attachment 24681 [details] Revised Fix for FrameTree.cpp for ambiguous frames.
John Holdsworth
Comment 5 2008-10-26 04:16:06 PDT
Comment on attachment 24681 [details] Revised Fix for FrameTree.cpp for ambiguous frames. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 37894) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,17 @@ > +2008-10-26 John Holdsworth <webkit@johnholdsworth.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Tests: fast/frames/frameTree-top.html > + fast/frames/frameTree-frame.html > + fast/frames/frameTree-form.html > + > + Frames now searched for by name starting at the innermost frame > + moving up the tree to the outermost containing parent level by level. > + > + * page/FrameTree.cpp: > + (WebCore::FrameTree::find): > + > 2008-10-25 Geoffrey Garen <ggaren@apple.com> > > Not reviewed. > Index: WebCore/page/FrameTree.cpp > =================================================================== > --- WebCore/page/FrameTree.cpp (revision 37894) > +++ WebCore/page/FrameTree.cpp (working copy) > @@ -183,10 +183,15 @@ Frame* FrameTree::find(const AtomicStrin > if (name == "_blank") > return 0; > > - // Search subtree starting with this frame first. > - for (Frame* frame = m_thisFrame; frame; frame = frame->tree()->traverseNext(m_thisFrame)) > - if (frame->tree()->name() == name) > - return frame; > + // Search up tree starting with this frame first. > + for (Frame* parent = m_thisFrame; parent; parent = parent->tree()->parent()) { > + for (Frame* frame = parent; frame; frame = frame->tree()->traverseNext(parent)) { > + if (frame->tree()->name() == name) > + return frame; > + } > + } > + > + // page search may no longer be required... > > // Search the entire tree for this page next. > Page* page = m_thisFrame->page(); > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 37894) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2008-10-26 John Holdsworth <webkit@johnholdsworth.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Frames now searched for by name starting at the innermost frame > + moving up the tree to the outermost containing parent level by level. > + > + * fast/frames/frameTree-form.html: Added. > + * fast/frames/frameTree-frame.html: Added. > + * fast/frames/frameTree-top.html: Added. > + > 2008-10-24 Eric Seidel <eric@webkit.org> > > Reviewed by Sam Weinig. > Index: LayoutTests/fast/frames/frameTree-form.html > =================================================================== > --- LayoutTests/fast/frames/frameTree-form.html (revision 0) > +++ LayoutTests/fast/frames/frameTree-form.html (revision 0) > @@ -0,0 +1,6 @@ > +<!-- see/open frameTree-top.html --> > +<html><body> > +<form target=out action='http://www.google.com/search'> > +Search for: <input name='q' value='webkit'> > +<input type=submit value='Click me'></form> > +Output from form submission should appear below. > Index: LayoutTests/fast/frames/frameTree-frame.html > =================================================================== > --- LayoutTests/fast/frames/frameTree-frame.html (revision 0) > +++ LayoutTests/fast/frames/frameTree-frame.html (revision 0) > @@ -0,0 +1,6 @@ > +<!-- see/open frameTree-top.html --> > +<html> > +<frameset rows='100,*'> > + <frame src='frameTree-form.html'> > + <frame src='javascript: ""' name='out'> > +</frameset> > Index: LayoutTests/fast/frames/frameTree-top.html > =================================================================== > --- LayoutTests/fast/frames/frameTree-top.html (revision 0) > +++ LayoutTests/fast/frames/frameTree-top.html (revision 0) > @@ -0,0 +1,10 @@ > +<-- > + > +Open to test fix for bugzilla Bug# 21638: WebCore/page/FrameTree.cpp:find() dispatches form submissions to incorrect frame > + > +--> > +<html> > +<frameset cols='50%,50%'> > +<frame src='frameTree-frame.html'> > +<frame src='frameTree-frame.html'> > +</frameset>
David Kilzer (:ddkilzer)
Comment 6 2008-11-29 21:40:28 PST
Comment on attachment 24681 [details] Revised Fix for FrameTree.cpp for ambiguous frames. >+2008-10-26 John Holdsworth <webkit@johnholdsworth.com> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Frames now searched for by name starting at the innermost frame >+ moving up the tree to the outermost containing parent level by level. >+ >+ * fast/frames/frameTree-form.html: Added. >+ * fast/frames/frameTree-frame.html: Added. >+ * fast/frames/frameTree-top.html: Added. This is a good first step toward a layout test, John! However, please note that: - Layout tests are run in an automated fashion. - The patch should include the expected results of the tests (see WebKitTools/Scripts/run-webkit-tests --help). - Layout tests should not depend on external resources (like www.google.com). Please see this page for more details: http://webkit.org/quality/testwriting.html The easiest thing to do is to look at a similar test when writing your own. Note that since this is a form submission issue, you may need to write an "http" test (see LayoutTest/http/tests/) which run under an Apache web server. r- to fix layout test issues.
Note You need to log in before you can comment on or make changes to this bug.