Bug 107705 - [Qt] window.open passes height and width parameters even if not defined in a page
Summary: [Qt] window.open passes height and width parameters even if not defined in a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: QtWebkit23
  Show dependency treegraph
 
Reported: 2013-01-23 10:44 PST by jingdow
Modified: 2013-08-09 11:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2013-01-24 04:43 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2013-01-24 05:29 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (8.91 KB, patch)
2013-02-06 08:08 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jingdow 2013-01-23 10:44:44 PST
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)
Comment 1 David Rosca 2013-01-23 10:51:32 PST
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.
Comment 2 Allan Sandfeld Jensen 2013-01-24 03:12:37 PST
(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?
Comment 3 David Rosca 2013-01-24 03:17:17 PST
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.
Comment 4 Allan Sandfeld Jensen 2013-01-24 04:06:16 PST
The issue appears to be with enforcing the minimumWindowSize from ChromeClient.h
Comment 5 Allan Sandfeld Jensen 2013-01-24 04:27:32 PST
Seems to be regresssion caused the patch for bug #102072
Comment 6 Allan Sandfeld Jensen 2013-01-24 04:43:22 PST
Created attachment 184465 [details]
Patch

Quick patch, needs test.
Comment 7 jingdow 2013-01-24 04:49:31 PST
(In reply to comment #6)
> Created an attachment (id=184465) [details]
> Patch
> 
> Quick patch, needs test.

It works! Thank you very much.
Comment 8 jingdow 2013-01-24 04:55:18 PST
(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.
Comment 9 David Rosca 2013-01-24 05:14:55 PST
Confirm, the patch fixe the issue.

@daimonion This has nothing to do with QtWebKit, this decision is made inside client application (kWebKitPart or QupZilla)
Comment 10 Allan Sandfeld Jensen 2013-01-24 05:29:29 PST
Created attachment 184469 [details]
Patch
Comment 11 WebKit Review Bot 2013-01-24 05:33:13 PST
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.
Comment 12 Kenneth Rohde Christiansen 2013-01-24 05:39:21 PST
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?
Comment 13 Kenneth Rohde Christiansen 2013-01-24 05:47:04 PST
LGTM, but please check if there are some places where documentation has to be updated (say EFL, GTK etc)
Comment 14 Allan Sandfeld Jensen 2013-02-06 08:08:27 PST
Created attachment 186857 [details]
Patch

Update EFL and GTK to handle the return to possibly empty rects in setWindowRect.
Comment 15 WebKit Review Bot 2013-02-06 08:10:43 PST
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.
Comment 16 Kenneth Rohde Christiansen 2013-02-13 08:19:05 PST
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
Comment 17 Kenneth Rohde Christiansen 2013-02-13 08:19:20 PST
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
Comment 18 Allan Sandfeld Jensen 2013-02-13 08:35:01 PST
(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.
Comment 19 Kenneth Rohde Christiansen 2013-02-13 08:41:09 PST
You are probably right... fine with mee
Comment 20 Allan Sandfeld Jensen 2013-02-13 09:07:53 PST
(In reply to comment #19)
> You are probably right... fine with mee

Thanks!
Comment 21 WebKit Review Bot 2013-02-13 09:23:21 PST
Comment on attachment 186857 [details]
Patch

Clearing flags on attachment: 186857

Committed r142755: <http://trac.webkit.org/changeset/142755>
Comment 22 WebKit Review Bot 2013-02-13 09:23:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Alexey Proskuryakov 2013-08-09 08:29:49 PDT
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>).
Comment 24 Allan Sandfeld Jensen 2013-08-09 08:58:56 PDT
(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.
Comment 25 Alexey Proskuryakov 2013-08-09 11:44:32 PDT
Filed bug 119633.