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).
Created attachment 128750 [details] Patch proposal + Unit test
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
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.
(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.
(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.
(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...
(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
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).
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).
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
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
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
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.
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! :-)
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.
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
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.
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
(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.
Ping reviewers?
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.
(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.
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?
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
Committed r120908: <http://trac.webkit.org/changeset/120908>