When pop-up blocking is turned on in Safari, the browser blocks execution of the window.open function even if the target window is already open. In other words, if you try to use the window.open function to change the URL location of an already open window, pop-up blocking will prevent this from working (during onload or onunload events). There is no reason that pop-up blocking should interfere with this capability since it has nothing to do with popping up windows. Many web applications rely on this functionality and are broken by Safari's pop-up blocking. Mozilla's pop-up blocking does the right thing in this case and doesn't interfere with using window.open to change the URL location of already open windows. See http://www.angelblade.com/bugs/popup/index.html for an example.
Increasing priority since this bug is breaking some dutch banking sites.
<rdar://problem/3853181> is assigned to vicki with P2
Here's the line of code that needs to change. In the window.open implementation, there's this: if (!allowPopUp(exec, window) && !(frame->findFrame(frameName))) return jsUndefined(); It says "if there's not already a frame by this name, and we won't allow a pop-up, then do nothing". The bug is that findFrame only looks for frames inside the current frame tree, not in other windows. We just need to fix the code to use a frame-searching function that implements the whole frame naming algorithm, like -[WebCoreFrameBridge findFrameNamed:].
I have the Radar for this, so I'm going to grab the Bugzilla too!
Created attachment 6898 [details] no test case yet, but would like a first pass of review anyway
Created attachment 6976 [details] patch with detailed change log and a layout test
Comment on attachment 6976 [details] patch with detailed change log and a layout test Looks great. A few comments. Is there a reason for the layout test not to dumpAsText()? + Frame* childFrame = parentFrame->tree()->child(m_name); + if (childFrame) + childFrame->openURL(getDocument()->completeURL(relativeURL.qstring())); I would move the assignment here inside the if statement, to match other code in your patch. I would call "nameForNewChild" "uniqueNameForNewChild" or just "uniqueNameForChild". "New" is misleading since you call this function on old frames inside FrameTree::setName. Maybe you left out "unique" in order to hide within the Frame class the details of how new children are named, but doing so made it difficult for me to understand why you would have to call a method like this at all. + char suffix[40]; // 16 + maximum width of an unsigned integer + sprintf(suffix, "/<!--frame%u-->-->", childCount()); This looks like it works, but it's shady. I would *strongly* prefer it if we handled sprintf with thick rubber gloves. Also, "maximum" on what system? 32 bit? 64? 128? (By my calculations, it's 20 characters on a 64 bit system: 18,446,744,074,000,000,000 sans commas.) And what about the null terminator? I suggest something like: ASSERT(sizeof(unsigned) <= 64); char suffix[sizeof("/<!--frame-->-->") + 20]; // 20 is the number of characters required to print the largest unsigned integer on a 64 bit system snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount()); DumpRenderTree needs a ChangeLog. r=me, but given the sprintf business, I'm kinda rounding up :).
(In reply to comment #7) > Is there a reason for the layout test not to dumpAsText()? I tried it, and it just dumped as the empty string. I don't think frame contents are included in dumpAsText. > + Frame* childFrame = parentFrame->tree()->child(m_name); > + if (childFrame) > + > childFrame->openURL(getDocument()->completeURL(relativeURL.qstring())); > > I would move the assignment here inside the if statement, to match other code > in your patch. OK. > I would call "nameForNewChild" "uniqueNameForNewChild" or just > "uniqueNameForChild". "New" is misleading since you call this function on old > frames inside FrameTree::setName. Maybe you left out "unique" in order to hide > within the Frame class the details of how new children are named, but doing so > made it difficult for me to understand why you would have to call a method like > this at all. I'd like to find a better name. I think uniqueNameForChild is probably good. > > + char suffix[40]; // 16 + maximum width of an unsigned integer > + sprintf(suffix, "/<!--frame%u-->-->", childCount()); > > This looks like it works, but it's shady. I would *strongly* prefer it if we > handled sprintf with thick rubber gloves. Also, "maximum" on what system? 32 > bit? 64? 128? (By my calculations, it's 20 characters on a 64 bit system: > 18,446,744,074,000,000,000 sans commas.) And what about the null terminator? I > suggest something like: > > ASSERT(sizeof(unsigned) <= 64); > char suffix[sizeof("/<!--frame-->-->") + 20]; // 20 is the number of characters > required to print the largest unsigned integer on a 64 bit system > snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount()); OK. I'll do something more like what you suggest. > DumpRenderTree needs a ChangeLog. Will do.