Bug 25585 – [Qt] Webkit in Qt does not have window.showModalDialog

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Giacomo]

Webkit in Qt _does not have_ window.showModalDialog pseudo-JS function. To confirm this, window.alert(window.showModalDialog) pop ups "undefined".

But in the docs, see http://doc.trolltech.com/4.5/qwebpage.html#WebWindowType-enum, it seems modal browser dialogs can be generated. How, without window.showModalDialog?

[Tor Arne Vestbø]

This is not implemented in the Qt chrome client right now.

bool ChromeClientQt::canRunModal()
{
    notImplemented();
    return false;
}

Will investigate, thanks.

[Yael]

Created attachment 48260 [details]
Patch v1

WebKit already supports window.showModalDialog as defined in http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#dialogs-implemented-using-separate-documents.
This patch adds the necessary bits for QtWebKit to support that.

[Kenneth Rohde Christiansen]

Comment on attachment 48260 [details]
Patch v1


> +    bool canRun = false;
> +    QMetaObject::invokeMethod(m_webPage, "canRunModal", Qt::DirectConnection, Q_RETURN_ARG(bool, canRun));
> +    return canRun;
>  }

Is the invokeMethod supposed to modify the canRun? I suppose the Q_RETURN_ARG is a macro modifying it, but if that is the case, please add a comment. 
 
>  void ChromeClientQt::runModal()
>  {
> -    notImplemented();
> +    QMetaObject::invokeMethod(m_webPage, "runModal", Qt::DirectConnection);
>  }
>  

> @@ -72,7 +72,8 @@
>      Q_PROPERTY(LinkDelegationPolicy linkDelegationPolicy READ linkDelegationPolicy WRITE setLinkDelegationPolicy)
>      Q_PROPERTY(QPalette palette READ palette WRITE setPalette)
>      Q_PROPERTY(bool contentEditable READ isContentEditable WRITE setContentEditable)
> -    Q_ENUMS(LinkDelegationPolicy NavigationType WebAction)
> +    Q_PROPERTY(WebWindowType windowType READ webWindowType)
> +    Q_ENUMS(LinkDelegationPolicy NavigationType WebAction WebWindowType)

This patch introduces new API, please add it to the API tracker bug.

>  public:
>      enum NavigationType {
>          NavigationTypeLinkClicked,
> @@ -247,6 +248,8 @@
>      void setContentEditable(bool editable);
>      bool isContentEditable() const;
>  
> +    WebWindowType webWindowType() const;
> +
>  #ifndef QT_NO_CONTEXTMENU
>      bool swallowContextMenuEvent(QContextMenuEvent *event);
>  #endif
> @@ -301,6 +304,8 @@
>  
>  public Q_SLOTS:
>      bool shouldInterruptJavaScript();
> +    void runModal();
> +    bool canRunModal();


Are these supposed to be virtual?

[Yael]

Kenneth, When I introduced QWebPage::shouldInterruptJavaScript() I was told that we cannot add new virtual methods because that would break BC.
I am using here the same (ugly) technique to add a new "semi-virtual" method without breaking BC. 
I will add a comment about Q_RETURN_ARG modifying canRun.

[Yael]

Created attachment 48332 [details]
Patch v2

Added comment to ChromeClientQt as per comment #c3

[Yael]

Created attachment 48412 [details]
Patch v3

Make runModalDialog work almost "out of the box", with the exception that the application must set the modality flag on the correct window. WebKit does not know which window is the correct one for setting the modality flag.

[Kenneth Rohde Christiansen]

Comment on attachment 48412 [details]
Patch v3

should we change "should" to "must" ?

[Yael]

Landed as:  http://trac.webkit.org/changeset/54550

[Yael]

Comment on attachment 48412 [details]
Patch v3

> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(revision 54545)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(working copy)
> @@ -44,6 +44,7 @@
>  #include "SecurityOrigin.h"
>  
>  #include <qdebug.h>
> +#include <qeventloop.h>
>  #include <qtextdocument.h>
>  #include <qtooltip.h>
>  
> @@ -62,13 +63,15 @@
>  
>  ChromeClientQt::ChromeClientQt(QWebPage* webPage)
>      : m_webPage(webPage)
> +    , m_eventLoop(0)
>  {
>      toolBarsVisible = statusBarVisible = menuBarVisible = true;
>  }
>  
>  ChromeClientQt::~ChromeClientQt()
>  {
> -
> +    if (m_eventLoop)
> +        m_eventLoop->exit();
>  }
>  
>  void ChromeClientQt::setWindowRect(const FloatRect& rect)
> @@ -173,14 +176,16 @@
>  
>  bool ChromeClientQt::canRunModal()
>  {
> -    notImplemented();
> -    return false;
> +    return true;
>  }
>  
>  
>  void ChromeClientQt::runModal()
>  {
> -    notImplemented();
> +    m_eventLoop = new QEventLoop();
> +    QEventLoop* eventLoop = m_eventLoop;
> +    m_eventLoop->exec();
> +    delete eventLoop;
>  }
>  
>  
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.h
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.h	(revision 54545)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.h	(working copy)
> @@ -34,6 +34,7 @@
>  #include "KURL.h"
>  #include "PlatformString.h"
>  
> +class QEventLoop;
>  class QWebPage;
>  
>  namespace WebCore {
> @@ -158,6 +159,7 @@
>          bool toolBarsVisible;
>          bool statusBarVisible;
>          bool menuBarVisible;
> +        QEventLoop* m_eventLoop;
>      };
>  }
>  
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 54547)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,22 @@
> [phone]  Yael Aharon  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Webkit in Qt does not have window.showModalDialog
> +        https://bugs.webkit.org/show_bug.cgi?id=25585
> +
> +        Create a new eventloop when runModal() is called.
> +        Added comemnt in QWebPage::createWindow that the application is responsible
> +        for setting the modality of the appropriate window.
> +
> +        * Api/qwebpage.cpp:
> +        * WebCoreSupport/ChromeClientQt.cpp:
> +        (WebCore::ChromeClientQt::ChromeClientQt):
> +        (WebCore::ChromeClientQt::~ChromeClientQt):
> +        (WebCore::ChromeClientQt::canRunModal):
> +        (WebCore::ChromeClientQt::runModal):
> +        * WebCoreSupport/ChromeClientQt.h:
> +
>  2010-01-19  Kenneth Rohde Christiansen  <[e-mail]>
>  
>          Reviewed by Dave Hyatt.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 54545)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -1955,6 +1955,8 @@
>      If the view associated with the web page is a QWebView object, then the default implementation forwards
>      the request to QWebView's createWindow() function; otherwise it returns a null pointer.
>  
> +    If \a type is WebModalDialog, the application should call setWindowModality(Qt::ApplicationModal) on the new window.
> +
>      \sa acceptNavigationRequest()
>  */
>  QWebPage *QWebPage::createWindow(WebWindowType type)
> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 54547)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Yael Aharon  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Webkit in Qt does not have window.showModalDialog
> +        https://bugs.webkit.org/show_bug.cgi?id=25585
> +
> +        Set the modality flag when createWindow is called with window type WebWindowDialog.
> +
> +        * QtLauncher/main.cpp:
> +        (WebPage::createWindow):
> +
>  2010-02-09  Chang Shu  <[e-mail]>
>  
>          Reviewed by Laszlo Gombos.
> Index: WebKitTools/QtLauncher/main.cpp
> ===================================================================
> --- WebKitTools/QtLauncher/main.cpp	(revision 54545)
> +++ WebKitTools/QtLauncher/main.cpp	(working copy)
> @@ -520,9 +520,11 @@
>  #endif
>  }
>  
> -QWebPage* WebPage::createWindow(QWebPage::WebWindowType)
> +QWebPage* WebPage::createWindow(QWebPage::WebWindowType type)
>  {
>      LauncherWindow* mw = new LauncherWindow;
> +    if (type == WebModalDialog)
> +        mw->setWindowModality(Qt::ApplicationModal);
>      mw->show();
>      return mw->page();
>  }
Clearing flags.

[Csaba Osztrogonác]

Qt expected files updated by http://trac.webkit.org/changeset/54568

But I'm not sure if platform/qt/http/tests/security/cross-frame-access-call-expected.txt is correct:
-PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result of expression 'window.showModalDialog' [undefined] is not an object.' and is.
+*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError: Result of expression 'window.showModalDialog' [undefined] is not an object.' but instead is undefined.

What do you think about it?

[Simon Hausmann]

(In reply to comment #10)
> Qt expected files updated by http://trac.webkit.org/changeset/54568
> 
> But I'm not sure if
> platform/qt/http/tests/security/cross-frame-access-call-expected.txt is
> correct:
> -PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result
> of expression 'window.showModalDialog' [undefined] is not an object.' and is.
> +*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError:
> Result of expression 'window.showModalDialog' [undefined] is not an object.'
> but instead is undefined.
> 
> What do you think about it?

That's most likely caused due to the lack of modal window support in the Qt DRT.

[Yael]

(In reply to comment #10)
> Qt expected files updated by http://trac.webkit.org/changeset/54568
> 
> But I'm not sure if
> platform/qt/http/tests/security/cross-frame-access-call-expected.txt is
> correct:
> -PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result
> of expression 'window.showModalDialog' [undefined] is not an object.' and is.
> +*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError:
> Result of expression 'window.showModalDialog' [undefined] is not an object.'
> but instead is undefined.
> 
> What do you think about it?

I actually think that this is the expected result. Mac port does not support canRunModal in DRT, and their expected results reflect that. Since we do support it, we have different results.
It all comes down to the fact that the implementation of canRunModal in WebChromeClient.mm returns false when running in DRT, and ChromeClientQt.cpp returns true. 
(this is being called from JSDOMWindow::getOwnPropertySlot() before we check cross frames access rights)

[Tor Arne Vestbø]

As far as I can tell this is fixed?

[Giacomo]

Is this fixed in Qt 4.6.2?

[Balazs Kelemen]

I have just looked around in ChromeClientQt.cpp and I think there is a bug in this code: 

void ChromeClientQt::runModal()
{
    m_eventLoop = new QEventLoop();
    QEventLoop* eventLoop = m_eventLoop;
    m_eventLoop->exec();
    delete eventLoop;
}

It deletes the new object. I guess it should delete the old one. BTW I think we should not have a non-null loop here anyway.

[Yael]

(In reply to comment #15)
> I have just looked around in ChromeClientQt.cpp and I think there is a bug in this code: 
> 
> void ChromeClientQt::runModal()
> {
>     m_eventLoop = new QEventLoop();
>     QEventLoop* eventLoop = m_eventLoop;
>     m_eventLoop->exec();
>     delete eventLoop;
> }
> 
> It deletes the new object. I guess it should delete the old one. BTW I think we should not have a non-null loop here anyway.

The point is that the line after "m_eventLoop->exec()" is executed AFTER the dialog is closed. There is no "old" or "new".