Bug 190117 – [GTK] Theming of authentication dialog breaks with themes other than Adwaita

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

[Adrian Perez]

Using a theme different than Adwaita (the GTK+ default), and in particular
themes that have dark elements, can make the authentication dialog impossible
to read. This happens after bug #189545 (r236051). The Arc-Dark theme is
particularly good at triggering issues with the custom rendering/theming
of GTK+ elements done in WebKitGTK+, as seen in the attached screenshot.

[Adrian Perez]

I keep getting internal errors from Bugzilla when trying to upload
the screenshot. For the time being I have posted it here:

    http://fugu.perezdecastro.org/auth-dialog-arc-darker.png

[Adrian Perez]

Some findings after discussing the issue with Benjamin Otte (thanks!):

 - The dialog is missing the “background” style class.
 - The dialog has valign/halign = GTK_ALIGN_FILL, when it should be
   GTK_ALIGN_CENTER.

After setting those in the GTK+ inspector, the dialog looks fine, but the
Web view content is not seen behind the dialog (which might be due to
using the inspector, that needs checking). Also, there are some other
bits we may want to look into add for the styling; for example it would
be interesting to look into “gtkwindow.c” to check what is needed to have
GTK+ paint a shadow below the dialog.

[Adrian Perez]

Created attachment 351315 [details]
Patch

[Adrian Perez]

(In reply to Adrian Perez from comment #2)
> Some findings after discussing the issue with Benjamin Otte (thanks!):
> 
>  - The dialog is missing the “background” style class.

This was partly an issue. The save/restore was needed to avoid the base
class implementation of the draw method to paint an opaque background,
so in the patch instead we just mark the widget as app-paintable to
avoid that and do all the rendering in the WebKitWebViewDialog draw
implementation.

This also allows to remove the save/restore of the style context in
the widget draw implementation, and have the “background” style class
added just once at widget construction time.

>  - The dialog has valign/halign = GTK_ALIGN_FILL, when it should be
>    GTK_ALIGN_CENTER.

This was not a problem in the end.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 351315 [details]
Patch

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

So in the end we did not need to switch from "messagedialog" to "dialog"?

> Source/WebKit/ChangeLog:17
> +        is uneeded because the style classes are set at widget construction

unneeded

> Source/WebKit/ChangeLog:23
> +        (webkitWebViewDialogConstructed): Mark the widget as app-paintable to
> +        avoid the base widget paint operation to fill the whole background,
> +        because the dialog widget paints the translucent overlay itself; and
> +        also add the missing GTK_STYLE_CLASS_BACKGROUND class at construction
> +        when building against GTK+ 3.20 or newer.

I actually don't like the translucent overlay, though. It looks broken if you visit some random website, then a different website that requires HTTP auth, and the original website displays underneath in the background. There's no good reason for this, and it could really confuse the user into thinking there is a security problem.

But I guess we are talking about different overlays. I don't want the web content to be visible. But of course I want the gray background to be visible. And I'm sure that's what you're fixing here.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 351315 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351315&action=review
> 
> So in the end we did not need to switch from "messagedialog" to "dialog"?
> 
> > Source/WebKit/ChangeLog:17
> > +        is uneeded because the style classes are set at widget construction
> 
> unneeded
> 
> > Source/WebKit/ChangeLog:23
> > +        (webkitWebViewDialogConstructed): Mark the widget as app-paintable to
> > +        avoid the base widget paint operation to fill the whole background,
> > +        because the dialog widget paints the translucent overlay itself; and
> > +        also add the missing GTK_STYLE_CLASS_BACKGROUND class at construction
> > +        when building against GTK+ 3.20 or newer.
> 
> I actually don't like the translucent overlay, though. It looks broken if
> you visit some random website, then a different website that requires HTTP
> auth, and the original website displays underneath in the background.
> There's no good reason for this, and it could really confuse the user into
> thinking there is a security problem.
> 
> But I guess we are talking about different overlays. I don't want the web
> content to be visible. But of course I want the gray background to be
> visible. And I'm sure that's what you're fixing here.

We are talking about the same grey grey overlay that dims the rendered Web
content while a dialog is visible. This is making that work again, the way
it used to before the patch for bug #189545; nothing is changed, just fixed.

Personally I like it the overlay, and painting a solid theme-color background
would be ugly, veeery ugly. If we want to get rid of this translucent grey 
a much nicer looking option would be to make the WebKitWebViewDialog look
*more* like an actual window and make it have a shadow and so on, because
otherwise it's going to blend with the rendered Web content and be not very
obvious that there's a dialog “floating” on top of it. Yesterday Benjamin
and me toyed with the idea of adding the shadow as well, but it would require
further changes to the dialog implementation instead of just a small fix

(For future reference: the idea for the dialog shadow would be to add the
current dialog controls inside some box-like container, and add style classes
to the box so GTK+ will paint the same shadow it usually does for top-level
windows: an additional CSS node is needed to mock the top-level frame+shadow.)

[Adrian Perez]

My suggestion would be to land this (I'll post an updated patch
with the typo fixed) to get the dialog styling fix without doing
any new different styling. If we want a new/different styling we
should do that as a new bug. This way at least we won't regress
if we end up not having time to further rework the dialogs.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 351315 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351315&action=review
> 
> So in the end we did not need to switch from "messagedialog" to "dialog"?

JFTR, this was not needed in the end, it was a red herring.

[Adrian Perez]

Created attachment 351365 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351365 [details]
Patch

Clearing flags on attachment: 351365

Committed r236731: <https://trac.webkit.org/changeset/236731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #9)
> (In reply to Michael Catanzaro from comment #6)
> > Comment on attachment 351315 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=351315&action=review
> > 
> > So in the end we did not need to switch from "messagedialog" to "dialog"?
> 
> JFTR, this was not needed in the end, it was a red herring.

Not needed for the alerts either?

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Adrian Perez from comment #9)
> > (In reply to Michael Catanzaro from comment #6)
> > > Comment on attachment 351315 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=351315&action=review
> > > 
> > > So in the end we did not need to switch from "messagedialog" to "dialog"?
> > 
> > JFTR, this was not needed in the end, it was a red herring.
> 
> Not needed for the alerts either?

Alert message dialogs also show fine with the current “trunk”, I'll
attach a screenshot.

[Adrian Perez]

Created attachment 353904 [details]
Screenshot of alert window with current WebKitGTK+ trunk