Bug 190117

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

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.

Created attachment 351315 [details] Patch

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

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

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.

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

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.

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

Created attachment 351365 [details] Patch

Comment on attachment 351365 [details] Patch Clearing flags on attachment: 351365 Committed r236731: <https://trac.webkit.org/changeset/236731>

All reviewed patches have been landed. Closing bug.

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

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

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