Bug 189545 – [GTK] Make script dialogs modal to the current web view only

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

They are currently modal to the application.

[Carlos Garcia Campos]

Created attachment 349543 [details]
Patch

[Michael Catanzaro]

Comment on attachment 349543 [details]
Patch

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

Could you show before/after screenshots for both dialogs?

And does this make the text selectable?

> Source/WebCore/platform/gtk/po/POTFILES.in:32
> -../../../WebKit/UIProcess/API/gtk/WebKitScriptDialogGtk.cpp
> +../../../WebKit/UIProcess/API/gtk/WebKitScriptDialogImpl.cpp

Wow, you remembered... you're a translation hero, now.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 349543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349543&action=review
> 
> Could you show before/after screenshots for both dialogs?

I'll take screenshots, I tried to make the dialogs as close as possible to the old ones, but GTK+ theming is so hard...

> And does this make the text selectable?

No, I tried, but it affected the focus for some reason. Going to a different tab and back to the one with the alert made the label focused (with the text selected) instead of the default button.

> > Source/WebCore/platform/gtk/po/POTFILES.in:32
> > -../../../WebKit/UIProcess/API/gtk/WebKitScriptDialogGtk.cpp
> > +../../../WebKit/UIProcess/API/gtk/WebKitScriptDialogImpl.cpp
> 
> Wow, you remembered... you're a translation hero, now.

[Carlos Garcia Campos]

Created attachment 349639 [details]
Screenshot

That's what I could get without going crazy with the GTK+ theming. If we really want to make them more similar to the GTK ones, like making the buttons bigger, we could try to improve them in follow up patches. Taking the screenshots I've also realized that beforeunload (or any dialog with more text) doesn't look good with patch attached to bug #184875 applied. Maybe we should use a minimum content with for the scrolling window, based on web view size. I'll investigate.

[Michael Catanzaro]

Based on the screenshots, I think we should try harder with the theming here. I know it's hard....

To get the dialogs to look like those created by GTK, we need to set a widget path, like we do in createStyleContext() in RenderThemeGtk.cpp.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Based on the screenshots, I think we should try harder with the theming
> here. I know it's hard....

I don't think they are that different.

> To get the dialogs to look like those created by GTK, we need to set a
> widget path, like we do in createStyleContext() in RenderThemeGtk.cpp.

We already do that, but this case is even more complicated, because we are using real widgets, that create their own style context. We use a custom style context to render the background, but real widget style contexts don't inherit from it. 

Feel free to fight with the theming yourself if it's important for you. For me it's more important to *finally* have non-modal js dialogs.

[Carlos Garcia Campos]

Committed r236004: <https://trac.webkit.org/changeset/236004>

[Carlos Garcia Campos]

In the end I managed to make the dialogs look better by using gtk_widget_class_set_css_name() when compiling with GTK+ >= 3.20.

[Michael Catanzaro]

Committed r236009: <https://trac.webkit.org/changeset/236009>

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> Committed r236009: <https://trac.webkit.org/changeset/236009>

Could you explain this change? why do we need to forward declare WebKitWebView in script dialog header when it isn't used there at all?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Michael Catanzaro from comment #9)
> > Committed r236009: <https://trac.webkit.org/changeset/236009>
> 
> Could you explain this change? why do we need to forward declare
> WebKitWebView in script dialog header when it isn't used there at all?

Fixed in https://trac.webkit.org/changeset/236051/webkit