Bug 79500 – [GTK] Add support for window.showModalDialog in WebKit2GTK+

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

This would require implementing runModal feature in the WebKitUIClient, emitting a 'run-as-modal' signal from the WebKitWebView object and creating a new mainloop there (if the signal is handled) to block user interaction in the original window (the opener) when the modal dialog is showing. See bug 53600 for a similar case in WebKit1.

I'm setting this bug as depending on bug 79496 and bug 79499 because I'm assuming that we have the following issues fixed for writing the patch (which I'll be attaching right after filing this bug):

  - Support for GMainLoop and GMainContext in GRefPtr
  - Support for nested event loops in RunLoop [*]

[*] We need this for the webprocess not to die after closing a modal dialog (see bug 79499 for more details), but this is different from the other nested loop that we would be adding with the patch for this bug, since this one will be in the UI process and will take care of using user interaction with the opener window mainly. So yes, it seems like we need both, one in the Web Process (bug 79499) and the other one in the UI process (this bug).

[Mario Sanchez Prada]

Created attachment 128750 [details]
Patch proposal + Unit test

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 128750 [details]
Patch proposal + Unit test

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

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:141
> +    webkitWebViewRunAsModalPage(WEBKIT_WEB_VIEW(clientInfo));

Why 'Page'? We usually use webkitWebViewNameOfTheSignal() so this would be webkitWebViewRunAsModal().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:497
> +     * Emitted when JavaScript code calls
> +     * <function>window.showModalDialog</function>. If this signal is
> +     * handled (returns %TRUE), a new mainloop will be created and run
> +     * until the current webview is finalized.
> +     *
> +     * Returns: %TRUE to stop other handlers from being invoked for the event.
> +     *    %FALSE to propagate the event further.

We have a problem with this approach. The web process runs the nested loop unconditionally unless canRunModal is false. canRunModal is a page initialization parameter, so when this signal is emitted it's too late to decide, and the nested main loop is already running in the web process. Since this signal can only be emitted once per web view (the view runs as modal until it's destroyed) we could use a construct property to be able to enable/disable modal views on every web view. Avoiding a modal view is something that you typically want to do globally or at least per view, so it doesn't need to be decided when the signal is emitted. We can't use a WebKitSetting for this, because settings are supposed to be able to be used at any moment, but canRunModal can only be set when the page is initialized. So, we could add a construct-only property, so that when canRunModal is FALSE, this signal is not emitted at all. We would also need to add API to create a web view with initial properties. We could use something similar to webkit_settings_new_with_settings() or wrap WebPageCreationParameters and add webkit_web_view_new_with_creatiion_parameters() (or whatever name we choose).

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:486
> +    UIClientTest::add("WebKitWebView", "run-as-modal", testWebViewRunAsModal);

You might want to test the case where you return FALSE from the signal and the main loop is not run.

> Tools/MiniBrowser/gtk/main.c:48
> +    GtkWidget *mainWindow = browser_window_new(WEBKIT_WEB_VIEW(webView), 0);

You can use NULL in C code.

[Martin Robinson]

(In reply to comment #3)
 
> We can't use a WebKitSetting for this, because settings are supposed to be able to be used at any moment, but canRunModal can only be set when the page is initialized. 

canRunModal is called on the child WebView right? So perhaps a WebSetting that would work would be one on the parent like, "can-create-modal-child-webviews." canRunModal would simply check the property on the parent WebView.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > We can't use a WebKitSetting for this, because settings are supposed to be able to be used at any moment, but canRunModal can only be set when the page is initialized. 
> 
> canRunModal is called on the child WebView right? So perhaps a WebSetting that would work would be one on the parent like, "can-create-modal-child-webviews." canRunModal would simply check the property on the parent WebView.

That's a great idea.

[Mario Sanchez Prada]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > We can't use a WebKitSetting for this, because settings are supposed to be able to be used at any moment, but canRunModal can only be set when the page is initialized. 
> 
> canRunModal is called on the child WebView right? So perhaps a WebSetting that would work would be one on the parent like, "can-create-modal-child-webviews." canRunModal would simply check the property on the parent WebView.

The decision of creating a new modal dialog and the action doing that (creating the frame, running the loop, notifying the UIClient...) happens in DOMWindow.cpp, in WebCore, so I'm not sure how having a WebSetting in the UI process can influence this decision in any way.

Another way would be to make that WebSetting changing a new option in WebCore/page/Settings that we would check in DOMWindow.cpp (as we do with canShowModalDialog or allowPopups) to decide whether to go ahead or not, but that would be a little bit hackish since we already have the canRunModal property for the WebPage in there, and adding a new one would be too much like a workaround, I think.

Last, but not least, the new webView for the dialog is not create in WebKit but in the application (in the handler for the WebKitWebView::create signal) so we can't control how we do that at all, I think, and if we try to hook on the ::create signal it would be already too late since the WebPage would have been created already, and so the (creational) canRunModal parameter would have been already set.

Thus, I think that we have to think about other options here...

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
> Thus, I think that we have to think about other options here...

Talking to Carlos about this we realized of some other things that might be worth considering and that would probably affect the final solution:

  - The decision taken in the Web process to know whether WebKit can run modal dialogs or not,
    ultimately relies on WebUIClient::canRunModal(), which returns true if WebUIClient::runModal()
    is implemented for the specific port (in API/gtk/WebKitUIClient.cpp, in our case).

  - It seems to us that changing this property often does not make much sense after all. It is not like
     blocking popups (for what we already have a WebSetting), but something more at the level of how
     you would like windows to behave in your system (e.g. at the level of the Window Manager), since
     we're talking about having or not _modal_ dialogs, not about having dialogs at all. So, in this sense,
     it looks reasonable that it's a creational parameter for the WebPage.

  - Martin's suggestion doesn't seem to be feasible because new webviews are created in the application
    while handling the WebKitWebView::create signal, and so we don't have enough control to decide
    from there how to create the window.

So, all things considered, Carlos had what I think it was a really good idea: as this is a global feature and the decision is ultimately made based on having defined in the port the WebUIClieng::runModal() function, we could perhaps add a new setting to the WebKitWebContext that would be checked to decide whether to implement or not runModal in attachUIClientToView(), which is called whenever a WebView is constructed.

That would be global to all the webviews of course, but I think it's not a bad solution. I'd work on that line

[Mario Sanchez Prada]

Created attachment 129272 [details]
Patch proposal + Unit tests

(In reply to comment #7)
> [...]
> So, all things considered, Carlos had what I think it was a really good idea: as this is a global feature and the 
> decision is ultimately made based on having defined in the port the WebUIClieng::runModal() function, we 
> could perhaps add a new setting to the WebKitWebContext that would be checked to decide whether to 
> implement or not runModal in attachUIClientToView(), which is called whenever a WebView is constructed.

So, turned out that at the end I haven't follow that approach because I found something out I didn't know of before: it is actually possible to change the value of the canRunModal attribute in the WebPage from the UIProcess, as there's even a message already defined for that: SetCanRunCommand, which is already used in WebKit::WebPageProxy::initializeUIClient(). The only thing we'd need would be to add some public functions in WebPageProxy so we can query and set the canRunModal attribute in the WebPage from the WebKitWebView, so I went ahead and wrote this new patch, which precisely address the issue doing that.

Also, I found out another important thing too: canRunModal is used in the web process to call runModal in the UI client, _but also to create the new page_ (the modal dialog), which means that if this attribute is set to false no dialog will be created (so it's not jsut a matter of the dialog being modal or not, as I thought in principle). So, I took this into account as well when writing the unit tests and now we have two of them: one to check that the 'create', 'ready-to-show' and 'run-as-modal' are emitted when allowing modal dialogs (new API added for this in WebKitWebView too) and another one to check that no signal is emitted if not allowed (as no dialog will be ever created).

[Mario Sanchez Prada]

Created attachment 129276 [details]
Patch proposal + Unit tests

Now attaching the right patch, as I messed things up with the last one (didn't build, didn't use the proper wait() function un the unit tests to exit the main loop).

[Gustavo Noronha (kov)]

Comment on attachment 129276 [details]
Patch proposal + Unit tests

Attachment 129276 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11669166

[Carlos Garcia Campos]

Comment on attachment 129276 [details]
Patch proposal + Unit tests

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

I prefer to use a WebKitSetting for that. The view can connect to ::notify of the settings to call m_page->setCanRunModal()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:494
> +     * Emitted when JavaScript code calls
> +     * <function>window.showModalDialog</function>, right before
> +     * creating and running a new mainloop to block user interaction
> +     * in the parent webview until the current webview is finalized.

You should mention that this is called after ready signal in the newly created view and that the view will run in modal mode until it's closed. You should also mention that this signal is to allow the user to prepare the view to run in modal mode, typically setting the modal hint and transient for.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1409
> + * Enable the current web view to create and run modal dialogs from
> + * JavaScript through the <function>window.showModalDialog</function>.

You should clarify here that if this is set to FALSE, when window.showModalDialog(), no dialog will be created at all, so ::create signal won't be emitted. In other words, this setting prevents create signal to be emitted when window.showModalDialog() is used.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:129
> +    gboolean   (* run_as_modal)         (WebKitWebView             *web_view);

This is not boolean enymore

[Mario Sanchez Prada]

Created attachment 129722 [details]
Patch proposal + Unit tests

Attaching a new patch addressing the issues pointed out by Carlos, and rebased against latest changes in trunk.

Sorry I'm not providing a ChangeLog entry properly filled yet because I have to literally run out of the office now, will do in the next patch.

(In reply to comment #11)
> (From update of attachment 129276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129276&action=review
> 
> I prefer to use a WebKitSetting for that. The view can connect to ::notify of the settings to call m_page->setCanRunModal()

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:494
> > +     * Emitted when JavaScript code calls
> > +     * <function>window.showModalDialog</function>, right before
> > +     * creating and running a new mainloop to block user interaction
> > +     * in the parent webview until the current webview is finalized.
> 
> You should mention that this is called after ready signal in the newly created view and that the view will run in modal mode until it's closed. You should also mention that this signal is to allow the user to prepare the view to run in modal mode, typically setting the modal hint and transient for.

Fixed

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1409
> > + * Enable the current web view to create and run modal dialogs from
> > + * JavaScript through the <function>window.showModalDialog</function>.
> 
> You should clarify here that if this is set to FALSE, when window.showModalDialog(), no dialog will be created at all, so ::create signal won't be emitted. In other words, this setting prevents create signal to be emitted when window.showModalDialog() is used.

Fixed
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:129
> > +    gboolean   (* run_as_modal)         (WebKitWebView             *web_view);
> 
> This is not boolean enymore

Fixed

[Carlos Garcia Campos]

Comment on attachment 129722 [details]
Patch proposal + Unit tests

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:850
> +                                                         TRUE,

I think this should be FALSE by default. Otherwise an app not handling ::run-as-modal would end up with a main loop blocking a non modal dialog

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:176
> +static void allowModalDialogsChanged(GObject* object, GParamSpec*, WebKitWebView* webView)

You casted this in g_signal_connect, so you can use directly WebKitSettings *settings, so that you don't need neither a cast nor a local variable.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:538
> +     * Emitted after #WebKitWebView::create and
> +     * #WebKitWebView::ready-to-show on the newly created
> +     * #WebKitWebView when JavaScript code calls

This is probably a bit confusing, it might look like create is called in the newly created view, I would leave only Emitted after #WebKitWebView::ready-to-show on the newly created #WebKitWebView.

[Mario Sanchez Prada]

Created attachment 129916 [details]
Patch proposal + Unit tests

(In reply to comment #13)
> (From update of attachment 129722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129722&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:850
> > +                                                         TRUE,

Agreed. Fixed.

> I think this should be FALSE by default. Otherwise an app not handling ::run-as-modal would end up with a main loop blocking a non modal dialog
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:176
> > +static void allowModalDialogsChanged(GObject* object, GParamSpec*, WebKitWebView* webView)
> 
> You casted this in g_signal_connect, so you can use directly WebKitSettings *settings, so that you don't need neither a cast nor a local variable.

I'm a moron. Fixed (the extra cast, not me being a moron. You can file a bug for that if you want, though)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:538
> > +     * Emitted after #WebKitWebView::create and
> > +     * #WebKitWebView::ready-to-show on the newly created
> > +     * #WebKitWebView when JavaScript code calls
> 
> This is probably a bit confusing, it might look like create is called in the newly created view, I would leave only Emitted after #WebKitWebView::ready-to-show on the newly created #WebKitWebView.

It is definitely confusing. Fixed too.

Also, the new patch features a ChangeLog again. Reviewers won't have any excuse now! :-)

[Mario Sanchez Prada]

Created attachment 134057 [details]
Patch proposal + Unit tests

The previous patch got  a bit outdated, so now attaching a new one that applies cleanly against today's trunk.

[Gustavo Noronha (kov)]

Comment on attachment 134057 [details]
Patch proposal + Unit tests

Attachment 134057 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12142589

[Mario Sanchez Prada]

Created attachment 143786 [details]
Patch proposal + Unit test

The previous patch got  a bit outdated again, so now attaching a new one that applies cleanly against today's trunk.

I also made the most of this to re-implement the unit test in a different way, using a new subclass of UIClientTest (ModalDialogsTest) to avoid adding some "ifs" to check whether it was a "modal dialogs" test or not.

[Carlos Garcia Campos]

Comment on attachment 143786 [details]
Patch proposal + Unit test

Patch looks good to me in general, but I would like someone to take a look at the changes in WebPageProxy

[Mario Sanchez Prada]

(In reply to comment #18)
> (From update of attachment 143786 [details])
> Patch looks good to me in general, but I would like someone to take a look at the changes in WebPageProxy

Thanks for the review Carlos.

In order to give a bit more of context to the people reviewing those changes in WebPageProxy, let me c&p here an excerpt from the ChangeLog file, as I have written it when making this patch:

 "Allow setting and getting the value of WebPage's canRunModal
  attribute in the WebProcess from the UIProcess after the creation
  of a WebPage, to allow using it from WebKitWebView to allow the
  client application to decide whether to allow create modal
  dialogs, which would result in launching an additional nested
  event loop in the web process, after creating the dialog."

It has been a while since I wrote this patch so I probably don't recall the details with 100% accuracy, but the issue this tries to fix is that there is no way so far to enable or disable the ability of running modal dialogs from the UI Process.

[Mario Sanchez Prada]

Ping reviewers?

[Eric Seidel (no email)]

If there were not gtk api changes (or so much of a comment history on this bug), I'd be happy to rubber-stamp this.  At this point seems you just need to pin down one of the gtk reviewers to finish the job.

[Carlos Garcia Campos]

(In reply to comment #21)
> If there were not gtk api changes (or so much of a comment history on this bug), I'd be happy to rubber-stamp this.  At this point seems you just need to pin down one of the gtk reviewers to finish the job.

The API changes look good to me.

[Eric Seidel (no email)]

Comment on attachment 143786 [details]
Patch proposal + Unit test

I guess I should say that you also can't break the gtk-ews bubble. :)  Unless we believe it's lying to us?

[Mario Sanchez Prada]

Created attachment 148545 [details]
Patch proposal + Unit test

It seems I forgot to update webkit2gtk-sections.txt with the new accessors in WebKitSettings. This should fix the redness in GTK EWS

[Mario Sanchez Prada]

Committed r120908: <http://trac.webkit.org/changeset/120908>