Bug 53600

This requires a complete implementation of ChromeClientGtk::canRunModal and ChromeClientGtk::runModal functions. Patch incoming.

Created attachment 126265 [details] Patch Adds support for running modal dialogs

Comment on attachment 126265 [details] Patch This seems like a fine change. We need another reviewer to approve the API though. CCing some people.

CCing some people to approve this new API. This implements modal dialogs.

Comment on attachment 126265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126265&action=review Looks pretty good to me. I had an implementation of this locally for some demos but you beat me to it :) > Source/WebKit/gtk/webkit/webkitwebview.cpp:2758 > + 0, 0, Hrm, it seems more idiomatic to make this a TRUE = handled and use g_signal_accumulator_true_handled.

Created attachment 126324 [details] Patch Uses g_signal_accumulator_true_handled

Comment on attachment 126324 [details] Patch Clearing flags on attachment: 126324 Committed r107351: <http://trac.webkit.org/changeset/107351>

All reviewed patches have been landed. Closing bug.

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review I know I'm late, but I've found several issues in this patch. > Source/WebKit/gtk/webkit/webkitwebview.cpp:2746 > /* Double * missing here > Source/WebKit/gtk/webkit/webkitwebview.cpp:2747 > + * WebKitWebView::run-modal-dialog Trailing : missing here. also the name is a bit confusing, it seems this signal is to run a modal dialog, while it actually runs the toplevel window of the web view in modal mode. > Source/WebKit/gtk/webkit/webkitwebview.cpp:2757 > + * Since: 1.7.6 This version doesn't exist, it's 1.7.90 > Source/WebKit/gtk/webkit/webkitwebview.cpp:2759 > + webkit_web_view_signals[RESOURCE_LOAD_FAILED] = g_signal_new("run-modal-dialog", RESOURCE_LOAD_FAILED -> that should be RUN_MODAL_DIALOG > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962 > + GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view))); You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly. > Tools/GtkLauncher/main.c:134 > + GtkWidget *window = gtk_widget_get_toplevel(GTK_WIDGET(webView)); > + gtk_window_set_transient_for(GTK_WINDOW(window), GTK_WINDOW(openerWindow)); > + gtk_window_set_modal(GTK_WINDOW(window), TRUE); > + return TRUE; Instead of duplicating this in GtkLauncher and DRT, this should be default implementation when the signal is not handled in WebView

Reverted r107351 for reason: Several issues introduced in WebKitGTK+ API Committed r108459: <http://trac.webkit.org/changeset/108459>

Sorry for rolling this out, but we are close to a new release, and I think we have to discuss this a bit more. After reading he code, now I understand what this signal is for. This signal is only called by DOMWindow::showModalDialog(). This method creates a normal popup dialog, so that signals create-web-view are web-view-ready normally, and after the new frame is shown in a window, runModal() is called, to run the newly created view in modal mode, until close() is called (close-web-view signal is emitted). The current name of the signal it's similar to the DOMWindow method (showModalDialog()), but the method does a lot of more things than what the signal is expected to do. So, since the other signals involved refer to the view and not to a window (create-web-view, web-view-ready and close-web-view) this signal could be called run-modal-web-view (or something similar). The documentation should explain also that this signal is always emitted (if emitted) after web-view-ready signal to run the newly created view in modal mode, until close-web-view signal is emitted (and the view is destroyed). Does it sound good?

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:2748 > + * @webView: the object which received the signal This should be @web_view > Source/WebKit/gtk/webkit/webkitwebview.cpp:2751 > + * avoided if FALSE is returned in the signal handler. Otherwise, the %FALSE > Source/WebKit/gtk/webkit/webkitwebview.cpp:2752 > + * @webView (or its toplevel window) should be made transient for its parent, @web_view > Source/WebKit/gtk/webkit/webkitwebview.cpp:2753 > + * set as a modal window, and TRUE should be returned in the signal handler. %TRUE > Source/WebKit/gtk/webkit/webkitwebview.cpp:2754 > + * After that, a loop will be created and run until the @webView is closed @web_view > Source/WebKit/gtk/webkit/webkitwebview.cpp:2756 > + * (i.e. its chrome is destroyed). > + * Signal is true_handled, it should include information about the Return value: * Return value: %TRUE to stop handlers from being invoked for the event or * %FALSE to propagate the event furter

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review About the name in the next comment. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2746 >> /* > > Double * missing here Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2747 >> + * WebKitWebView::run-modal-dialog > > Trailing : missing here. also the name is a bit confusing, it seems this signal is to run a modal dialog, while it actually runs the toplevel window of the web view in modal mode. Noted about the ':', I'll write about the name below. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2748 >> + * @webView: the object which received the signal > > This should be @web_view Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2751 >> + * avoided if FALSE is returned in the signal handler. Otherwise, the > > %FALSE Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2752 >> + * @webView (or its toplevel window) should be made transient for its parent, > > @web_view Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2753 >> + * set as a modal window, and TRUE should be returned in the signal handler. > > %TRUE Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2754 >> + * After that, a loop will be created and run until the @webView is closed > > @web_view Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2756 >> + * > > Signal is true_handled, it should include information about the Return value: > > * Return value: %TRUE to stop handlers from being invoked for the event or > * %FALSE to propagate the event furter I'd like to be more verbose - %TRUE to stop handlers being invoked and run the modal dialog loop, %FALSE to state that the modal dialog should not be run and propagate the event further. (Not this exact wording, but something similar.) >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2757 >> + * Since: 1.7.6 > > This version doesn't exist, it's 1.7.90 Noted. >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2759 >> + webkit_web_view_signals[RESOURCE_LOAD_FAILED] = g_signal_new("run-modal-dialog", > > RESOURCE_LOAD_FAILED -> that should be RUN_MODAL_DIALOG Oops :) >> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962 >> + GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view))); > > You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly. I'll put a similar check in both DRT and GtkLauncher. >> Tools/GtkLauncher/main.c:134 >> + gtk_window_set_modal(GTK_WINDOW(window), TRUE); >> + return TRUE; > > Instead of duplicating this in GtkLauncher and DRT, this should be default implementation when the signal is not handled in WebView When signal is not handled (or FALSE is returned), the current implementation interprets this as a request not to run the modal dialog loop, cancelling any further operations in that direction. I think it wouldn't be appropriate to force a modal dialog to be shown if the signal is not handled.

(In reply to comment #13) > (From update of attachment 126324 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review > >> Source/WebKit/gtk/webkit/webkitwebview.cpp:2756 > >> + * > > > > Signal is true_handled, it should include information about the Return value: > > > > * Return value: %TRUE to stop handlers from being invoked for the event or > > * %FALSE to propagate the event furter > > I'd like to be more verbose - %TRUE to stop handlers being invoked and run the modal dialog loop, %FALSE to state that the modal dialog should not be run and propagate the event further. > > (Not this exact wording, but something similar.) Well, this is kind of stantard text we use in every signal that is true_handled. > >> Tools/GtkLauncher/main.c:134 > >> + gtk_window_set_modal(GTK_WINDOW(window), TRUE); > >> + return TRUE; > > > > Instead of duplicating this in GtkLauncher and DRT, this should be default implementation when the signal is not handled in WebView > > When signal is not handled (or FALSE is returned), the current implementation interprets this as a request not to run the modal dialog loop, cancelling any further operations in that direction. > > I think it wouldn't be appropriate to force a modal dialog to be shown if the signal is not handled. You are right, I agree. The same way we don't have a default impl for web-view-rady and close-web-view.

(In reply to comment #11) > Sorry for rolling this out, but we are close to a new release, and I think we have to discuss this a bit more. After reading he code, now I understand what this signal is for. This signal is only called by DOMWindow::showModalDialog(). This method creates a normal popup dialog, so that signals create-web-view are web-view-ready normally, and after the new frame is shown in a window, runModal() is called, to run the newly created view in modal mode, until close() is called (close-web-view signal is emitted). > No problem about the roll-out, but sorry for the mess. > The current name of the signal it's similar to the DOMWindow method (showModalDialog()), but the method does a lot of more things than what the signal is expected to do. So, since the other signals involved refer to the view and not to a window (create-web-view, web-view-ready and close-web-view) this signal could be called run-modal-web-view (or something similar). The documentation should explain also that this signal is always emitted (if emitted) after web-view-ready signal to run the newly created view in modal mode, until close-web-view signal is emitted (and the view is destroyed). > The create-web-view signal is triggered on the originatin web view, creating the new web view - the dialog web view. On this dialog web view are then triggered the web-view-ready and close-web-view signals. run-modal-web-view sound to me like some other web view will be run as a modal dialog (rather than the one on which the signal was triggered). I'd suggest something like 'run-as-modal-web-view' or even simpler, just 'run-as-modal'.

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review >>>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2756 >>>> + * >>> >>> Signal is true_handled, it should include information about the Return value: >>> >>> * Return value: %TRUE to stop handlers from being invoked for the event or >>> * %FALSE to propagate the event furter >> >> I'd like to be more verbose - %TRUE to stop handlers being invoked and run the modal dialog loop, %FALSE to state that the modal dialog should not be run and propagate the event further. >> >> (Not this exact wording, but something similar.) > > Well, this is kind of stantard text we use in every signal that is true_handled. OK then, the standard text is fine but the documentation should state clearly what will be going on based on the return value of the handler.

(In reply to comment #15) > > The current name of the signal it's similar to the DOMWindow method (showModalDialog()), but the method does a lot of more things than what the signal is expected to do. So, since the other signals involved refer to the view and not to a window (create-web-view, web-view-ready and close-web-view) this signal could be called run-modal-web-view (or something similar). The documentation should explain also that this signal is always emitted (if emitted) after web-view-ready signal to run the newly created view in modal mode, until close-web-view signal is emitted (and the view is destroyed). > > > > The create-web-view signal is triggered on the originatin web view, creating the new web view - the dialog web view. On this dialog web view are then triggered the web-view-ready and close-web-view signals. Exactly, I would move the signal creation and signal value in the enum between web-view-ready and close-web-view signals, to make it clear that they are related. web-view-ready doc says emitted in the newly created view (or something like that). That should be clarified in the doc for run-modal too. > run-modal-web-view sound to me like some other web view will be run as a modal dialog (rather than the one on which the signal was triggered). I'd suggest something like 'run-as-modal-web-view' or even simpler, just 'run-as-modal'. run-as-modal sounds good to me, but I guess it should be web-view-run-as-modal for consistency. I never liked the web-view prefix/suffix in those signals (and we don't use it in wk2), but I think it helps to understand that those signals are all related, and that a web view can't be switched to modal mode at any time, but after ready when it has been created by create.

(In reply to comment #16) > > Well, this is kind of stantard text we use in every signal that is true_handled. > > OK then, the standard text is fine but the documentation should state clearly what will be going on based on the return value of the handler. Exactly, thanks!

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review Hi, I started today working on a similar patch for WebKit2GTK+ and this patch has been extremely helpful so far, so thank you. Just have a doubt related to creating a new mainloop, see my comment below... > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:206 > + GMainContext* threadDefaultContext = g_main_context_ref_thread_default(); > + g_main_context_acquire(threadDefaultContext); > + > + m_modalLoop = g_main_loop_new(threadDefaultContext, FALSE); > + g_main_loop_run(m_modalLoop); > + g_main_loop_unref(m_modalLoop); > + m_modalLoop = 0; > + > + g_main_context_release(threadDefaultContext); > + g_main_context_unref(threadDefaultContext); Is this actually needed? I mean, as far as I can see, calling gtk_window_set_modal(window, TRUE) will already call gtk_grab_add(), which as per documentation does the following: "Makes widget the current grabbed widget. This means that interaction with other widgets in the same application is blocked and mouse as well as keyboard events are delivered to this widget." So, I wonder if it would be enough emitting the signal so the application can decide whether to call to set_transient_for and set_modal (which would do all the modal magic, I think), or if I'm missing something here, and perhaps creating and running a new main loop would be needed after all.

(In reply to comment #19) > [...] So, I wonder if it would be enough emitting the signal so the application can decide whether to call to > set_transient_for and set_modal (which would do all the modal magic, I think), or if I'm missing something > here, and perhaps creating and running a new main loop would be needed after all. Also, in case it was needed to create that new mainloop, I wonder whether it should be a decision that the client application should make, in the same way it's its responsibility to call to gtk_window_set_modal and gtk_window_set_transient_for (and so WKGTK should care only about emitting the signal) Sorry for the two-comments thing

(In reply to comment #20) > (In reply to comment #19) > > [...] So, I wonder if it would be enough emitting the signal so the application can decide whether to call to > > set_transient_for and set_modal (which would do all the modal magic, I think), or if I'm missing something > > here, and perhaps creating and running a new main loop would be needed after all. > > Also, in case it was needed to create that new mainloop, I wonder whether it should be a decision that the client application should make, in the same way it's its responsibility to call to gtk_window_set_modal and gtk_window_set_transient_for (and so WKGTK should care only about emitting the signal) > > Sorry for the two-comments thing The main loop is required as it serves as the event loop for the web view that is running as the modal dialog. This blocks user input events in the web view from which the dialog originated. Giving the job of spinning the event loop to the client application may work, but it complicates things for that application implementation-wise. Requesting others' input. Also please have a look at bug 17171 for modal dialogs in WebKit2Gtk+ - there are some early sketches of what an API for common JavaScript dialogs (alert, confirm, prompt) might look like, and I've proposed about perhaps including modal dialogs in there as well.

Comment on attachment 126324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review >>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962 >>> + GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view))); >> >> You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly. > > I'll put a similar check in both DRT and GtkLauncher. Unfortunately this check is not possible here, because the dialog web view is not a child of a GtkWindow since a newly-created web view is not packed into anything. Because the dialog web view doesn't have any parents, gtk_widget_get_toplevel returns a pointer to the dialog web view itself. The check would then fail because the pointer does not represent neither a toplevel nor a GtkWindow, causing test failures.

Created attachment 128211 [details] Patch

Comment on attachment 128211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128211&action=review One very important thing about this signal is that the client must be able to differentiate between modal dialogs and normal child WebViews. Imagine an application that makes a modal WebView an overlay over an existing one, instead of a new window. Is web-view-ready called for modal dialogs? If it is and especially if it's called before run-web-view-as-modal, it's going to be really tricky to pack the modal dialog WebView properly. There is no create-modal-web-view, like in the Mac port, to know to pack the widget differentily. r- for now, until these issues are worked out or the documentation explains it. Thank you for updating your patch. I think this can definitely go into stable with just a little polish. > Source/WebKit/gtk/webkit/webkitwebview.cpp:1690 > + * made transient for its parent, set as a modal window, and %TRUE should be > + * returned in the signal handler. After that, a loop will be created internally > + * and run until the @web_view is closed (i.e. its chrome is destroyed) and the > + * #WebKitWebView::close-web-view signal is emitted. > + * > + * Return value: %TRUE to stop handlers from being invoked for the event or > + * %FALSE to propagate the event furter > + * > + * Since: 1.7.90 > + */ The documentation should make it clear that this is the appropriate time to show the window and not during create-web-view. Another important piece of information is whether or not web-view-ready fires here. You should probably change the Since: line to say "1.8.0," because I'm not sure at what point this patch will be merged into the stable branch. > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1294 > "signal::create-web-view", webViewCreate, 0, > + "signal::web-view-run-as-modal", webViewRunModalDialog, 0, > "signal::close-web-view", webViewClose, 0, The signal name is not really consistent with create-web-view and close-web-view and also doesn't scan as an English phrase. I much prefer run-web-view-as-modal.

(In reply to comment #21) > [...] > The main loop is required as it serves as the event loop for the web view that is running as the modal dialog. This blocks user input events in the web view from which the dialog originated. Ok I understand, but I still have a concern (maybe an invalid one, though): If we block on the modal dialog this way, wouldn't we getting more than just an user block regarding to the original window (the opener)? For instance, imagine a new window is opened with showModalDialog() _while_ the original window (the opener) has not still finished rendering... wouldn't this cause the rendering to get blocked (so it never finishes, since the original mainloop won't be dispatching paint events) once the modal dialog is running? If that was the case, I'm not sure that's what we really want... perhaps it would be better to block user input only (as performed by gtk_grab_add()) instead of blocking the original main loop maybe too much. > Giving the job of spinning the event loop to the client application may work, but it complicates things for that application implementation-wise. Requesting others' input. Yeah, it's probably not a good idea to move that stuff to the application. Not 100% sure either, though. > Also please have a look at bug 17171 for modal dialogs in WebKit2Gtk+ - there are some early sketches of what an API for common JavaScript dialogs (alert, confirm, prompt) might look like, and I've proposed about perhaps including modal dialogs in there as well. I'll take a look, thanks for the pointer!

(In reply to comment #24) > Is web-view-ready called for modal dialogs? If it is and especially if it's called before run-web-view-as-modal, it's going to be really tricky to pack the modal dialog WebView properly. There is no create-modal-web-view, like in the Mac port, to know to pack the widget differentily. Another important usecase is blocking the modal dialog entirely.

(In reply to comment #24) > (From update of attachment 128211 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128211&action=review > > One very important thing about this signal is that the client must be able to differentiate between modal dialogs and normal child WebViews. Imagine an application that makes a modal WebView an overlay over an existing one, instead of a new window. > > Is web-view-ready called for modal dialogs? If it is and especially if it's called before run-web-view-as-modal, it's going to be really tricky to pack the modal dialog WebView properly. There is no create-modal-web-view, like in the Mac port, to know to pack the widget differentily. Yes, as I said in comment #11. ShowModaldialog is: 1.- create-web-view 2.- web-view.-ready (on the newly created web view) 3.- run-modal (on the newly created web view) 4.- close That's the only way to create a modal web view. > r- for now, until these issues are worked out or the documentation explains it. Thank you for updating your patch. I think this can definitely go into stable with just a little polish. I think this patch should include unit tests too before landing.

(In reply to comment #27) > That's the only way to create a modal web view. What do you mean by this statement?

(In reply to comment #28) > (In reply to comment #27) > > > That's the only way to create a modal web view. > > What do you mean by this statement? I mean there's no other way to create a modal web view. In other words, ChromeClient::runModal() is only called from DOMWindow::showModalDialog(). Please read comment #11.

(In reply to comment #29) > > What do you mean by this statement? > > I mean there's no other way to create a modal web view. In other words, ChromeClient::runModal() is only called from DOMWindow::showModalDialog(). Please read comment #11. "there's no other way to create a modal web view" still makes no sense to me. There's at least one other way, and that's to have an API like the Mac port where the creation of a modal WebView happens in a different delegate method than the create of other child-WebViews. You sent me this link before: https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/WebKit/Protocols/WebUIDelegate_Protocol/Reference/Reference.html#//apple_ref/doc/uid/TP40003838 Note the webView:createWebViewModalDialogWithRequest: method.

Comment on attachment 128211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128211&action=review > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:99 > + if (m_modalLoop) > + g_main_loop_quit(m_modalLoop); You should check the loop is actually running before calling quit with g_main_loop_is_running() > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:206 > + GMainContext* threadDefaultContext = g_main_context_ref_thread_default(); > + g_main_context_acquire(threadDefaultContext); > + > + m_modalLoop = g_main_loop_new(threadDefaultContext, FALSE); > + g_main_loop_run(m_modalLoop); > + g_main_loop_unref(m_modalLoop); > + m_modalLoop = 0; > + > + g_main_context_release(threadDefaultContext); > + g_main_context_unref(threadDefaultContext); You should call GDK_THREADS_LEAVE() before running the loop and GDK_THREADS_ENTER() when it finishes. Do we really need to get and ref the default context? isn't it enough to create the main loop with the default context? Something like m_modalLoop = g_main_loop_new(0, FALSE); GDK_THREADS_LEAVE(); g_main_loop_run(m_modalLoop); GDK_THREADS_ENTER(); g_main_loop_unref(m_modalLoop);

(In reply to comment #22) > (From update of attachment 126324 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review > > >>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962 > >>> + GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view))); > >> > >> You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly. > > > > I'll put a similar check in both DRT and GtkLauncher. > > Unfortunately this check is not possible here, because the dialog web view is not a child of a GtkWindow since a newly-created web view is not packed into anything. Because the dialog web view doesn't have any parents, gtk_widget_get_toplevel returns a pointer to the dialog web view itself. The check would then fail because the pointer does not represent neither a toplevel nor a GtkWindow, causing test failures. But you are using the toplevel as a GtkWindow without checking neither it's a toplevel nor a GtkWindow. gtk_window_set_transient_for(GTK_WINDOW(viewTopLevel), GTK_WINDOW(window)); gtk_window_set_modal(GTK_WINDOW(viewTopLevel), TRUE);

(In reply to comment #30) > (In reply to comment #29) > > > What do you mean by this statement? > > > > I mean there's no other way to create a modal web view. In other words, ChromeClient::runModal() is only called from DOMWindow::showModalDialog(). Please read comment #11. > > "there's no other way to create a modal web view" still makes no sense to me. There's at least one other way, and that's to have an API like the Mac port where the creation of a modal WebView happens in a different delegate method than the create of other child-WebViews. > > You sent me this link before: https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/WebKit/Protocols/WebUIDelegate_Protocol/Reference/Reference.html#//apple_ref/doc/uid/TP40003838 > > Note the webView:createWebViewModalDialogWithRequest: method. That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2.

(In reply to comment #33) > That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2. I don't believe it's the case that it's Mac-specific. It's listed here: https://developer.mozilla.org/en/DOM/window.open Note that runModalDialog and window.open seem to take the same set of features. Some of them are interpreted by WindowFeatures.cpp and it seems others should just be passed on.

(In reply to comment #34) > (In reply to comment #33) > > > That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2. > > I don't believe it's the case that it's Mac-specific. It's listed here: https://developer.mozilla.org/en/DOM/window.open Maybe it's not mac specific, but doesn't look standard either. > Note that runModalDialog and window.open seem to take the same set of features. Some of them are interpreted by WindowFeatures.cpp and it seems others should just be passed on. That's because runModalDialog has to crated the window first, so it passes the features to createWindow(). If we want to support 'dialog', I think we should emulate showModalDialog, so the result would the same: create-web-view, web-view-ready, run-as-modal, close

(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > > > That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2. > > > > I don't believe it's the case that it's Mac-specific. It's listed here: https://developer.mozilla.org/en/DOM/window.open > > Maybe it's not mac specific, but doesn't look standard either. The standard says this about the features argument: "The third argument, features, has no defined effect and is mentioned for historical reasons only. User agents may interpret this argument as instructions to set the size and position of the browsing context, but are encouraged to instead ignore the argument entirely." [1] This isn't entirely helpful. My feeling is that we should probably pass it along because it's a property on WindowFeatures and is also exposed in the Qt, Chromium, Mac, Windows, and WebKit2 C APIs. 1. http://www.w3.org/TR/html5/browsers.html#dom-open

(In reply to comment #24) > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1294 > > "signal::create-web-view", webViewCreate, 0, > > + "signal::web-view-run-as-modal", webViewRunModalDialog, 0, > > "signal::close-web-view", webViewClose, 0, > > The signal name is not really consistent with create-web-view and close-web-view and also doesn't scan as an English phrase. I much prefer run-web-view-as-modal. run-web-view-as-modal seems better indeed. (In reply to comment #25) > (In reply to comment #21) > > [...] > > The main loop is required as it serves as the event loop for the web view that is running as the modal dialog. This blocks user input events in the web view from which the dialog originated. > > Ok I understand, but I still have a concern (maybe an invalid one, though): > > If we block on the modal dialog this way, wouldn't we getting more than just an user block regarding to the original window (the opener)? For instance, imagine a new window is opened with showModalDialog() _while_ the original window (the opener) has not still finished rendering... wouldn't this cause the rendering to get blocked (so it never finishes, since the original mainloop won't be dispatching paint events) once the modal dialog is running? > I'm not sure how the standard rendering is handled, but requestAnimationFrame callbacks are not invoked during running modal dialogs[1]. This is definitely something to try and test out. Still, comparing the approach to GtkDialog implementation of gtk_dialog_run, it is pretty similar.[2] > If that was the case, I'm not sure that's what we really want... perhaps it would be better to block user input only (as performed by gtk_grab_add()) instead of blocking the original main loop maybe too much. > Blocking only user input is not sufficient, as this causes the showModalDialog function to return immediately an undefined value. An example/testcase of this can be found at [3]. > (From update of attachment 128211 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128211&action=review > > > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:99 > > + if (m_modalLoop) > > + g_main_loop_quit(m_modalLoop); > > You should check the loop is actually running before calling quit with g_main_loop_is_running() > > > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:206 > > + GMainContext* threadDefaultContext = g_main_context_ref_thread_default(); > > + g_main_context_acquire(threadDefaultContext); > > + > > + m_modalLoop = g_main_loop_new(threadDefaultContext, FALSE); > > + g_main_loop_run(m_modalLoop); > > + g_main_loop_unref(m_modalLoop); > > + m_modalLoop = 0; > > + > > + g_main_context_release(threadDefaultContext); > > + g_main_context_unref(threadDefaultContext); > > You should call GDK_THREADS_LEAVE() before running the loop and GDK_THREADS_ENTER() when it finishes. Do we really need to get and ref the default context? isn't it enough to create the main loop with the default context? Something like > > m_modalLoop = g_main_loop_new(0, FALSE); > GDK_THREADS_LEAVE(); > g_main_loop_run(m_modalLoop); > GDK_THREADS_ENTER(); > g_main_loop_unref(m_modalLoop); These are all valid points and will be taken into account. These are basically the same steps used in gtk_dialog_run[2], thanks! (In reply to comment #31(In reply to comment #32) > (In reply to comment #22) > > (From update of attachment 126324 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review > > > > >>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962 > > >>> + GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view))); > > >> > > >> You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly. > > > > > > I'll put a similar check in both DRT and GtkLauncher. > > > > Unfortunately this check is not possible here, because the dialog web view is not a child of a GtkWindow since a newly-created web view is not packed into anything. Because the dialog web view doesn't have any parents, gtk_widget_get_toplevel returns a pointer to the dialog web view itself. The check would then fail because the pointer does not represent neither a toplevel nor a GtkWindow, causing test failures. > > But you are using the toplevel as a GtkWindow without checking neither it's a toplevel nor a GtkWindow. > > gtk_window_set_transient_for(GTK_WINDOW(viewTopLevel), GTK_WINDOW(window)); > gtk_window_set_modal(GTK_WINDOW(viewTopLevel), TRUE); True, yet for some odd reason, it does the job. Without these two steps, test failures occur. There are also no compile-time or run-time complaints. I'm hoping that after these debates and with subsequent results it will be possible to pack a modal web view into a GtkWindow, so all these calls would make sense. Another way is to pack every newly-created web view in DRT in its GtkWindow. (In reply to comment #36) > (In reply to comment #35) > > (In reply to comment #34) > > > (In reply to comment #33) > > > > > > > That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2. > > > > > > I don't believe it's the case that it's Mac-specific. It's listed here: https://developer.mozilla.org/en/DOM/window.open > > > > Maybe it's not mac specific, but doesn't look standard either. > > The standard says this about the features argument: "The third argument, features, has no defined effect and is mentioned for historical reasons only. User agents may interpret this argument as instructions to set the size and position of the browsing context, but are encouraged to instead ignore the argument entirely." [1] > > This isn't entirely helpful. My feeling is that we should probably pass it along because it's a property on WindowFeatures and is also exposed in the Qt, Chromium, Mac, Windows, and WebKit2 C APIs. > > 1. http://www.w3.org/TR/html5/browsers.html#dom-open While not standardized, it would be extremely helpful to indicate a dialog web view is to be created. The only problem is the possibility of this being removed in the future, because of the not-standardized thing. Luckily we won't be the only port API affected, which is a bit of reassuring. [1] http://trac.webkit.org/browser/trunk/LayoutTests/fast/animation/request-animation-frame-during-modal.html [2] http://git.gnome.org/browse/gtk+/tree/gtk/gtkdialog.c#n1034 [3] https://developer.mozilla.org/en/DOM/window.showModalDialog

(In reply to comment #27) > I think this patch should include unit tests too before landing. It really should, yes.

(In reply to comment #36) > (In reply to comment #35) > > (In reply to comment #34) > > > (In reply to comment #33) > > > > > > > That's called from ChromeClient::createWindow() when the dialog feature is present in WindowFeatures. I'm not sure 'dialog' is a standard feature, I think it's mac specific and we don't support it neither in wk1 nor wk2. > > > > > > I don't believe it's the case that it's Mac-specific. It's listed here: https://developer.mozilla.org/en/DOM/window.open > > > > Maybe it's not mac specific, but doesn't look standard either. > > The standard says this about the features argument: "The third argument, features, has no defined effect and is mentioned for historical reasons only. User agents may interpret this argument as instructions to set the size and position of the browsing context, but are encouraged to instead ignore the argument entirely." [1] > > This isn't entirely helpful. My feeling is that we should probably pass it along because it's a property on WindowFeatures and is also exposed in the Qt, Chromium, Mac, Windows, and WebKit2 C APIs. Yes, but it should be handled by the user like the other features. The result will be the same in the end, user connect to create-web-view and creates the view adding into a toplevel, then web-view-ready is emitted, now is when the user is supposed to handle the features before showing the toplevel. If the window properties object contains 'dialog' (we should probably find a better name, though) the user should set the modal hint, transient for and run a nested main loop. But it's always up to the user whether to handle the window feature or not. This is it's clear the difference between window.showModalDialog() and window.open() with dialog fature. I don't know objective-c, but it seems to me that mac only handles 'dialog' if the normal create-web-view delegate is not hanlded (because dialog is passed in the features dictionary, so the user should handle it).

(In reply to comment #39) > If the window properties object contains 'dialog' (we should probably find a better name, though) the user should set the modal hint, transient for and run a nested main loop. It's called 'dialog' on every single port and it's also called 'dialog' in web content. If we choose a different name it should be obvious to people to what bit of the DOM API it corresponds, so I think we should be cautious there. > But it's always up to the user whether to handle the window feature or not. This is it's clear the difference between window.showModalDialog() and window.open() with dialog fature. From the MDN link I posted above, I believe it's just an instruction to how the window should look. "The dialog feature removes all icons (restore, minimize, maximize) from the window's titlebar, leaving only the close button. Mozilla 1.2+ and Netscape 7.1 will render the other menu system commands (in FF 1.0 and in NS 7.0x, the command system menu is not identified with the Firefox/NS 7.0x icon on the left end of the titlebar: that's probably a bug. You can access the command system menu with a right-click on the titlebar). Dialog windows are windows which have no minimize system command icon and no maximize/restore down system command icon on the titlebar nor in correspondent menu item in the command system menu. They are said to be dialog because their normal, usual purpose is to only notify info and to be dismissed, closed. On Mac systems, dialog windows have a different window border and they may get turned into a sheet." In that sense, I think it's somewhat orthogonal to showModalDialog, which just controls whether or not there's an inner main loop while the WebView is showing. showModalDialog has some very specific semantics in the HTML5 spec and the 'dialog' window feature isn't present at all there. > I don't know objective-c, but it seems to me that mac only handles 'dialog' if the normal create-web-view delegate is not hanlded (because dialog is passed in the features dictionary, so the user should handle it). The code in question is: } else if (features.dialog && [delegate respondsToSelector:@selector(webView:createWebViewModalDialogWithRequest:)]) { newWebView = CallUIDelegate(m_webView, @selector(webView:createWebViewModalDialogWithRequest:), nil); } else { newWebView = CallUIDelegate(m_webView, @selector(webView:createWebViewWithRequest:), nil); } If the window features has 'dialog' and the UIDelegate has a webView:createWebViewModalDialogWithRequest: selector (method), it will be used. Otherwise the webView:createWebViewWithRequest: selector will be used even if 'dialog' is present. For what it's worth, I don't think we need to add a new signal, just be sure to expose this somehow in the window features.

(In reply to comment #40) > (In reply to comment #39) > > > If the window properties object contains 'dialog' (we should probably find a better name, though) the user should set the modal hint, transient for and run a nested main loop. > > It's called 'dialog' on every single port and it's also called 'dialog' in web content. If we choose a different name it should be obvious to people to what bit of the DOM API it corresponds, so I think we should be cautious there. Agree, fair enough. > > But it's always up to the user whether to handle the window feature or not. This is it's clear the difference between window.showModalDialog() and window.open() with dialog fature. > > From the MDN link I posted above, I believe it's just an instruction to how the window should look. > > "The dialog feature removes all icons (restore, minimize, maximize) from the window's titlebar, leaving only the close button. Mozilla 1.2+ and Netscape 7.1 will render the other menu system commands (in FF 1.0 and in NS 7.0x, the command system menu is not identified with the Firefox/NS 7.0x icon on the left end of the titlebar: that's probably a bug. You can access the command system menu with a right-click on the titlebar). Dialog windows are windows which have no minimize system command icon and no maximize/restore down system command icon on the titlebar nor in correspondent menu item in the command system menu. They are said to be dialog because their normal, usual purpose is to only notify info and to be dismissed, closed. On Mac systems, dialog windows have a different window border and they may get turned into a sheet." > > In that sense, I think it's somewhat orthogonal to showModalDialog, which just controls whether or not there's an inner main loop while the WebView is showing. showModalDialog has some very specific semantics in the HTML5 spec and the 'dialog' window feature isn't present at all there. Exactly, so even more reasons not to emit run-modal in this case. > > I don't know objective-c, but it seems to me that mac only handles 'dialog' if the normal create-web-view delegate is not hanlded (because dialog is passed in the features dictionary, so the user should handle it). > > The code in question is: > > } else if (features.dialog && [delegate respondsToSelector:@selector(webView:createWebViewModalDialogWithRequest:)]) { > newWebView = CallUIDelegate(m_webView, @selector(webView:createWebViewModalDialogWithRequest:), nil); > } else { > newWebView = CallUIDelegate(m_webView, @selector(webView:createWebViewWithRequest:), nil); > } > > If the window features has 'dialog' and the UIDelegate has a webView:createWebViewModalDialogWithRequest: selector (method), it will be used. Otherwise the webView:createWebViewWithRequest: selector will be used even if 'dialog' is present. So I understood it correctly then. > For what it's worth, I don't think we need to add a new signal, just be sure to expose this somehow in the window features. Yes, we should just expose the dialog feature in the WindowProperties object and let the user handle it like all other features.

I've just filed: [GTK] Add dialog feature to WebKitWindowProperties in WebKit2 GTK+ API https://bugs.webkit.org/show_bug.cgi?id=79483

Exposing the dialog property in window features seems fine, but I don't think this makes it possible for the user to prevent creating a dialog web view. Is this even required/needed? Should we make displaying and running modal dialogs optional through some setting or just signal through ChromeClient::canRunModal that modal dialogs are always going to be run?

(In reply to comment #43) > Exposing the dialog property in window features seems fine, but I don't think this makes it possible for the user to prevent creating a dialog web view. Is this even required/needed? Should we make displaying and running modal dialogs optional through some setting or just signal through ChromeClient::canRunModal that modal dialogs are always going to be run? It seems extrememly useful for popup blocking and the like. Do other ports allow this?

(In reply to comment #44) > (In reply to comment #43) > > Exposing the dialog property in window features seems fine, but I don't think this makes it possible for the user to prevent creating a dialog web view. Is this even required/needed? Should we make displaying and running modal dialogs optional through some setting or just signal through ChromeClient::canRunModal that modal dialogs are always going to be run? > > It seems extrememly useful for popup blocking and the like. Do other ports allow this? The DOMWindow::showModalDialog function aborts if the DOMWindow does not allow popups, that is when 'javascript-can-open-windows-automatically' property on WebKitWebSettings is set to FALSE. That should be sufficient, no?

*** Bug 81500 has been marked as a duplicate of this bug. ***

Given the focus is now shifting towards WebKit2 APIs and WebKit2 in general, meaning new API additions to WebKit1 don't really make sense, I'm willing to close this bug as a WONTFIX. I'll do so after I get some additional thumbs-ups in agreement.

I think it makes sense to shift this work to WebKit2. :)

(In reply to comment #48) > I think it makes sense to shift this work to WebKit2. :) Work getting done on this feature by Mario, bug #79500. Closing as WONTFIX.

(In reply to comment #49) > (In reply to comment #48) > > I think it makes sense to shift this work to WebKit2. :) > > Work getting done on this feature by Mario, bug #79500. Yes. I have been quite busy with other matters lately, but will hopefully have more time to push it forward during the following weeks.

Comment on attachment 128211 [details] Patch Just clearing the flags on the patch and marking it as obsolete.