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
Created attachment 24391 [details] Fix for FrameTree.cpp for ambiguous frames.
Created attachment 24392 [details] Test case for frame bug.
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.
Created attachment 24681 [details] Revised Fix for FrameTree.cpp for ambiguous frames.
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>
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.