Bug 107705 – [Qt] window.open passes height and width parameters even if not defined in a page

• Component: New Bugs
• Status: RESOLVED FIXED

In latest qtwebkit, unlike in previous stable versions, there's issue with top links in Gmail, when using QupZilla web browser (which supports javascript popup windows). Those links are not opening in new tab, as they do with qtwebkit 2.2, but rather they open in a popup window with 100x100 px in dimension.

Web browsers (behaviour tested in Opera, for example) process window.popup request like this:

 - if the height and width parameters are defined a popup window with the defined dimensions is opened
 - if heoight/width aren't defined then a window is opened in a new tab

This is the exact same behaviour of qtwebkit 2.2, but with the version 2.3 it seems that if no width/height in window.open request is defined a default minimum of 100px is passed to a browser.

(QupZilla web browser workarounded this for qtwebkit-2.3 by checking if the popup window has width and height of 100px: https://github.com/QupZilla/qupzilla/commit/2135ab8d13d221fb45d53419fcc9019ddaf4a070)

[David Rosca]

The change is in QWebPage::geometryChangeRequested(QRect) signal.
Until QtWebKit 2.3, it fired with
 * invalid QRect if it was opening links with target="_blank" and
 * valid QRect if it was opening real popup window (window.open)

However, it has now changed to return QRect.size() == QSize(100, 100) if it is opening target="_blank" links.
So it makes impossible to distinguish window.open (with size 100x100) and target="_blank" links.

[Allan Sandfeld Jensen]

(In reply to comment #1)
> The change is in QWebPage::geometryChangeRequested(QRect) signal.
> Until QtWebKit 2.3, it fired with
>  * invalid QRect if it was opening links with target="_blank" and
>  * valid QRect if it was opening real popup window (window.open)
> 
> However, it has now changed to return QRect.size() == QSize(100, 100) if it is opening target="_blank" links.
> So it makes impossible to distinguish window.open (with size 100x100) and target="_blank" links.

Is that the same trick used by all the Qt browsers, to detect this?

[David Rosca]

I know only about kWebKitPart and QupZilla that are trying to detect whether to open popup window.
And yes, kWebKitPart is also using the exact same trick.

[Allan Sandfeld Jensen]

The issue appears to be with enforcing the minimumWindowSize from ChromeClient.h

[Allan Sandfeld Jensen]

Seems to be regresssion caused the patch for bug #102072

[Allan Sandfeld Jensen]

Created attachment 184465 [details]
Patch

Quick patch, needs test.

(In reply to comment #6)
> Created an attachment (id=184465) [details]
> Patch
> 
> Quick patch, needs test.

It works! Thank you very much.

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=184465) [details] [details]
> > Patch
> > 
> > Quick patch, needs test.
> 
> It works! Thank you very much.

Some more information:

window.open opens window in new tab if no width or height are defined and if only one of those is defined.

If both window and height are defined window.open opens a popup window with given dimensions.
If height/width is defined lower than 100 px opened popup window defaults to 100 px.

[David Rosca]

Confirm, the patch fixe the issue.

@daimonion This has nothing to do with QtWebKit, this decision is made inside client application (kWebKitPart or QupZilla)

[Allan Sandfeld Jensen]

Created attachment 184469 [details]
Patch

[WebKit Review Bot]

Attachment 184469 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:448:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3281:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3282:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 184469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184469&action=review

> Source/WebCore/page/DOMWindow.cpp:346
> +    // Let size 0 pass through, since that indicates default size, not minimum size.
> +    if (window.width())
> +        window.setWidth(min(max(minimumSize.width(), window.width()), screen.width()));
> +    if (window.height())
> +        window.setHeight(min(max(minimumSize.height(), window.height()), screen.height()));

Shouldnt this be documented in some API?

Can we make a WK2 C api unit test?

[Kenneth Rohde Christiansen]

LGTM, but please check if there are some places where documentation has to be updated (say EFL, GTK etc)

[Allan Sandfeld Jensen]

Created attachment 186857 [details]
Patch

Update EFL and GTK to handle the return to possibly empty rects in setWindowRect.

[WebKit Review Bot]

Attachment 186857 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:448:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3280:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3281:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 186857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review

r=me with comments

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131
> -    if (!ewk_view_setting_enable_auto_resize_window_get(m_view))
> +    if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty())

You should add a FIXME: An empty rect means that default size should be used. Handle this case.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146
> -        gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());
> +        if (!intrect.isEmpty())
> +            gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());

Same here

[Kenneth Rohde Christiansen]

Comment on attachment 186857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review

r=me with comments

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131
> -    if (!ewk_view_setting_enable_auto_resize_window_get(m_view))
> +    if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty())

You should add a FIXME: An empty rect means that default size should be used. Handle this case.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146
> -        gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());
> +        if (!intrect.isEmpty())
> +            gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());

Same here

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (From update of attachment 186857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review
> 
> r=me with comments
> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131
> > -    if (!ewk_view_setting_enable_auto_resize_window_get(m_view))
> > +    if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty())
> 
> You should add a FIXME: An empty rect means that default size should be used. Handle this case.
> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146
> > -        gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());
> > +        if (!intrect.isEmpty())
> > +            gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height());
> 
> Same here

I was assuming the default size would be used when the window was opened, which means only the resize is unnecessary.

[Kenneth Rohde Christiansen]

You are probably right... fine with mee

[Allan Sandfeld Jensen]

(In reply to comment #19)
> You are probably right... fine with mee

Thanks!

[WebKit Review Bot]

Comment on attachment 186857 [details]
Patch

Clearing flags on attachment: 186857

Committed r142755: <http://trac.webkit.org/changeset/142755>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Please don't use bracketed prefixes on cross-platform patches. This one had cross-platform changes, and actually caused a regression in Safari (<rdar://problem/14693930>).

[Allan Sandfeld Jensen]

(In reply to comment #23)
> Please don't use bracketed prefixes on cross-platform patches. This one had cross-platform changes, and actually caused a regression in Safari (<rdar://problem/14693930>).

Sorry, the brackets should not have been there. I was later told the patch solve the same problem on GTK and Chromium.

[Alexey Proskuryakov]

Filed bug 119633.