Bug 29847 – [Qt] Add support for automatically creating new windows in QWebView

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

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 ?

[Simon Hausmann]

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.

[Antonio Gomes]

> --- 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 ?

[Simon Hausmann]

(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 ;-)

[Kent Hansen]

(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.

[Antonio Gomes]

> I'm in favor of just doing the doc update like Simon suggested.

Right. I will try something shortly ...

[Antonio Gomes]

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?

[Simon Hausmann]

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(...) :)

[Antonio Gomes]

Created attachment 65701 [details]
(commit r66217, r=hausmann)patch v2 - addressed simon's review

[Simon Hausmann]

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 :)

[Antonio Gomes]

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>

[Antonio Gomes]

(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.

[Ademar Reis]

Revision r66217 cherry-picked into qtwebkit-2.1 with commit de555d5dc3ef9ad493ba5f8b874efacfc440b1dd

[Kenneth Rohde Christiansen]

"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

[Antonio Gomes]

(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>

[Ademar Reis]

Revision r66221 cherry-picked into qtwebkit-2.1 with commit c61bb8fd4eec94c5ffa7107bb48f0f360ae3c2d3