Bug 189545 - [GTK] Make script dialogs modal to the current web view only
Summary: [GTK] Make script dialogs modal to the current web view only
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 189544
Blocks: 184875
  Show dependency treegraph
 
Reported: 2018-09-12 06:17 PDT by Carlos Garcia Campos
Modified: 2018-09-17 05:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (53.54 KB, patch)
2018-09-12 06:28 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Screenshot (164.47 KB, image/png)
2018-09-13 00:31 PDT, Carlos Garcia Campos
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-09-12 06:17:45 PDT
They are currently modal to the application.
Comment 1 Carlos Garcia Campos 2018-09-12 06:28:56 PDT
Created attachment 349543 [details]
Patch
Comment 2 Michael Catanzaro 2018-09-12 21:27:52 PDT
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.
Comment 3 Carlos Garcia Campos 2018-09-12 23:52:13 PDT
(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.
Comment 4 Carlos Garcia Campos 2018-09-13 00:31:19 PDT
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.
Comment 5 Michael Catanzaro 2018-09-13 08:04:50 PDT
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.
Comment 6 Carlos Garcia Campos 2018-09-13 23:48:51 PDT
(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.
Comment 7 Carlos Garcia Campos 2018-09-14 04:01:15 PDT
Committed r236004: <https://trac.webkit.org/changeset/236004>
Comment 8 Carlos Garcia Campos 2018-09-14 04:03:04 PDT
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.
Comment 9 Michael Catanzaro 2018-09-14 09:50:21 PDT
Committed r236009: <https://trac.webkit.org/changeset/236009>
Comment 10 Carlos Garcia Campos 2018-09-17 00:30:07 PDT
(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?
Comment 11 Michael Catanzaro 2018-09-17 05:39:46 PDT
(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