Bug 72319 – [Qt] Support customizing JS alert/confirm/prompt dialogs using QML

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

[Qt] Support customizing JS alert/confirm/prompt dialogs using QML

[Caio Marcelo de Oliveira Filho]

Created attachment 115045 [details]
Patch

[Simon Hausmann]

Great, this is _exactly_ what I had in mind.

Can we make it private first and use it for a while first, before we commit to it? (as a way to give us time to see if there are ways to make it even more convenient to use?)

But yeah, I don't have any to criticize about this patch. It's wonderful :). Let's let the others have a look at it.

[Simon Hausmann]

One thing I'm curious about: Are there any browsers that support "tab" modal prompts?

It seems that chrome for example brings up an application modal dialog, but does any browser support "per tab" modal prompts, too?

Is it worth exploring that use-case?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #3)
> One thing I'm curious about: Are there any browsers that support "tab" modal prompts?

Firefox (https://developer.mozilla.org/en/Using_tab-modal_prompts). Chrome supports too (you can navigate in other tabs) but the dialog still remains on top. Safari doesn't support it.

I think it's a good idea. The first step IMHO would be making the synchronous call for the dialogs between webprocess and UI process async and see whether we can "block" just a web page. I proposed this sync->async change previously in IRC (to avoid our nested mainloop) but the reason wasn't strong enough. Maybe tied with per tab dialog support it will be more compelling.

I will create a bug for this tomorrow :-)

[Alexis Menard (darktears)]

Comment on attachment 115045 [details]
Patch

I like it too

[Caio Marcelo de Oliveira Filho]

Created attachment 115360 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 115360 [details]
Patch

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

Overall I like the patch very much. Good job caio!

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:246
> +    confirmItem->setProperty("message", message);

We'll need to document that at some point at it is totally obscure for the user.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:159
> +};

Even if I know our C++ headers are private, does that need to be here? Let's try to keep it clean at least so that if people use webkit-private they don't end up with our implementation details defined/exposed (well you may say that our private is implementation details of QML items but you got my point).

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_javaScriptDialogs.qml:68
> +    }

That's cool to use, and let people with full customization capabilities.

[Caio Marcelo de Oliveira Filho]

Created attachment 115452 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 115360 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:246
>> +    confirmItem->setProperty("message", message);
> 
> We'll need to document that at some point at it is totally obscure for the user.

This is still a private feature and with the API still in-flux. I prefer we defer documentation until it stabilizes. At this point I think a good coverage in the QML tests help more.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:159
>> +};
> 
> Even if I know our C++ headers are private, does that need to be here? Let's try to keep it clean at least so that if people use webkit-private they don't end up with our implementation details defined/exposed (well you may say that our private is implementation details of QML items but you got my point).

Agreed, and fixed. Since DialogRunner has grown a bit, I moved it to its own file.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #8)
> Created an attachment (id=115452) [details]
> Patch

This new version reflects also the feedback from Tor-Arne in IRC today. He pointed out that ideally we should expose "models" to the dialogs, instead of setting properties.

I think the new patch fits more in this idea (by exposing things in the QDeclarativeContext used to create the dialog), and take it a step further: we expose slots to be called instead of expecting the dialog to define certain signals. This avoids a lot of "boilerplate" in the code for dialog.

[Caio Marcelo de Oliveira Filho]

Created attachment 115587 [details]
Patch

[WebKit Review Bot]

Attachment 115587 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #12)
> Attachment 115587 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> 
> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Ignore this style error. Created a patch for this https://bugs.webkit.org/show_bug.cgi?id=72620

[Caio Marcelo de Oliveira Filho]

Created attachment 115607 [details]
Patch

[WebKit Review Bot]

Attachment 115607 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 115607 [details]
Patch

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

Great start :) Let's get the experiment in.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:118
> +    QScopedPointer<QDeclarativeComponent> alertDialog;
> +    QScopedPointer<QDeclarativeComponent> confirmDialog;
> +    QScopedPointer<QDeclarativeComponent> promptDialog;

Are you sure about the use of QScopedPointer here? In QDeclarative I see that for example model delegates aren't stored with implied ownership transfer.

[Caio Marcelo de Oliveira Filho]

Committed r100767: <http://trac.webkit.org/changeset/100767>

[Caio Marcelo de Oliveira Filho]

(In reply to comment #16)
> Are you sure about the use of QScopedPointer here? In QDeclarative I see that for example model delegates aren't stored with implied ownership transfer.

Thanks for catching that. Landed without the scoped pointer, following same approach as other QML elements.