Bug 189545

Created attachment 349543 [details] Patch

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.

(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.

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.

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.

(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.

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

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.

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

(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?

(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