This bug report originated from issue QTBUG-2162 http://bugreports.qt.nokia.com/browse/QTBUG-2162 --- Description --- QWebView QWebPage - mainframe()-evaluateJavaScript("window.open('url')") doesn't open new browser window. When calling evaluateJavaScript("window.open('url')) on a mainFrame() or just load a webpage containing <script>window.open('http://www.trolltech.com/');</script> should open a new window containing the webpage. This is due to QWebView::createWindow() not creating a new window by default. We should consider adding a property to QWebView that enables the default implementation to create new QWebView instances, something along the lines of a 'bool autoCreateWindows' property. - How to reproduce: 1. Build and compile the code. 2. Start the application - Expected result: Two windows should appear. One containing the Trolltech webpage and one saying foo... ect. - Actual result: Only the last page containing: foo...before calling open foo...after calling open. A new window should have popped up showing the Trolltech webpage. Test case main.cpp for above ============================= class MainWidget : public QWidget { Q_OBJECT public: MainWidget(); private: QWebPage *page; QWebView *view; }; MainWidget::MainWidget(){ view = new QWebView(this); page = new QWebPage(this); QUrl url("test.html"); page->settings()->setAttribute(QWebSettings::JavascriptEnabled,true); // Make sure JavaScript is enabled page->mainFrame()->load(url); // Load the test case view->setPage(page); QVariant variant = page->mainFrame()->evaluateJavaScript("window.open('http://www.trolltech.com/');"); qDebug() << variant.toString(); // This doesn't return anything either. QVBoxLayout *layout = new QVBoxLayout; layout->addWidget(view); setLayout(layout); } int main(int argc, char *argv[]){ QApplication app(argc, argv); MainWidget mainWid; mainWid.show(); return app.exec(); } --- Comments --- Does this still occur when setting the property QWebSettings::JavascriptCanOpenWindows ?
I disagree that by default we should create new windows. I think this is part of the application integrate that needs to be taken care of by the application developer. Allowing new top-level windows coming up by default (that can be controlled via JavaScript) sounds like the wrong default to me from a security POV.
> --- Comments --- > > Does this still occur when setting the property > QWebSettings::JavascriptCanOpenWindows ? I just tested the sample application pasted here, and setting the property has no effect (and it ready shouldn't). so one who wants to make JavaScript to open new windows by default should set the 'JavascriptCanOpenWindows' to 'true' AND reimplement QWebPage's createWindow method ... it should get mentioned in DOCS in a clear way (if it is not yet there). (In reply to comment #1) > I disagree that by default we should create new windows. I think this is part > of the application integrate that needs to be taken care of by the application > developer. Allowing new top-level windows coming up by default (that can be > controlled via JavaScript) sounds like the wrong default to me from a security > POV. INVALID or WONTFIX then, simon ?
(In reply to comment #2) > > --- Comments --- > > > > Does this still occur when setting the property > > QWebSettings::JavascriptCanOpenWindows ? > > I just tested the sample application pasted here, and setting the property has > no effect (and it ready shouldn't). > > so one who wants to make JavaScript to open new windows by default should set > the 'JavascriptCanOpenWindows' to 'true' AND reimplement QWebPage's > createWindow method ... > > it should get mentioned in DOCS in a clear way (if it is not yet there). > > (In reply to comment #1) > > I disagree that by default we should create new windows. I think this is part > > of the application integrate that needs to be taken care of by the application > > developer. Allowing new top-level windows coming up by default (that can be > > controlled via JavaScript) sounds like the wrong default to me from a security > > POV. > > INVALID or WONTFIX then, simon ? I'd say WONTFIX, but ideally RESOLVED/FIXED with a patch to the documentation ;-)
(In reply to comment #1) > I disagree that by default we should create new windows. I think this is part > of the application integrate that needs to be taken care of by the application > developer. Allowing new top-level windows coming up by default (that can be > controlled via JavaScript) sounds like the wrong default to me from a security > POV. Creating new windows by default is not what the description suggests. It suggests adding a property that the default implementation (QWebView) can check. This property (e.g. autoCreateWindows) would be "false" by default, preserving the existing behavior. But regardless of whether this is implemented, the doc for QWebView::createWindow() should refer to QWebSettings::JavascriptCanOpenWindows (indeed, the testcase didn't enable this attribute, so createWindow() wasn't called regardless), and say what the default createWindow() implementation does (e.g. "always returns 0" or "depends on the autoCreateWindows property"). I'm in favor of just doing the doc update like Simon suggested.
> I'm in favor of just doing the doc update like Simon suggested. Right. I will try something shortly ...
Created attachment 65678 [details] patch v1 - fix the documentation If it is ok, it should be cherry-picked into both qtwebkit2.0 and qtwebkit2.1 branches. Kenneth, Simon, Kent, what are your takes on it?
Comment on attachment 65678 [details] patch v1 - fix the documentation I'm okay with that, please make this bug block the release bugs. However there's a typo in this patch: QWebSettings::setJavaScriptCanOpenWindowsAutomatically doesn't exist and should be setAttribute(...) :)
Created attachment 65701 [details] (commit r66217, r=hausmann)patch v2 - addressed simon's review
Comment on attachment 65701 [details] (commit r66217, r=hausmann)patch v2 - addressed simon's review WebKit/qt/Api/qwebview.cpp:960 + \sa QWebPage::createWindow(), QWebSettings::setJavaScriptCanOpenWindowsAutomatically() There's one left :) Please fix before landing, otherwise r=me :)
Comment on attachment 65701 [details] (commit r66217, r=hausmann)patch v2 - addressed simon's review Clearing flags on attachment 65701 [details] Committed r66217: <http://trac.webkit.org/changeset/66217>
(In reply to comment #9) > (From update of attachment 65701 [details]) > WebKit/qt/Api/qwebview.cpp:960 > + \sa QWebPage::createWindow(), QWebSettings::setJavaScriptCanOpenWindowsAutomatically() > There's one left :) > > Please fix before landing, otherwise r=me :) Done before landing... Thanks.
Revision r66217 cherry-picked into qtwebkit-2.1 with commit de555d5dc3ef9ad493ba5f8b874efacfc440b1dd
"of QWebSettings to true in order to get it called." This sounds a bit bad. "in order for it to get called", would be better
(In reply to comment #13) > "of QWebSettings to true in order to get it called." > > This sounds a bit bad. > > "in order for it to get called", would be better As per kenneth suggestion, changed in r66221. Committed r66221: <http://trac.webkit.org/changeset/66217>
Revision r66221 cherry-picked into qtwebkit-2.1 with commit c61bb8fd4eec94c5ffa7107bb48f0f360ae3c2d3