Bug 136615 – [GTK] Use a nicer HTTP authentication dialog

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

[Michael Catanzaro]

Created attachment 237759 [details]
Screenshot of current authentication dialog

The default HTTP authentication dialog does not look very good. Instead of working around this in Epiphany, let's make the default dialog nicer.

Suggested improvements from [1] are:

 * The dialog is far too wide, with too much space between the labels and the entry fields.
 * The entry fields are too wide.
 * The "OK" button should have a more meaningful label, like "Authenticate" or "Unlock".
 * It doesn't have a heading.
 * The first line reads awkwardly: it is in the wrong tense. Perhaps "Username and password required" would be better.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=727149

[Michael Catanzaro]

Created attachment 237760 [details]
Screenshot of proposed changes

[Michael Catanzaro]

Created attachment 237762 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Attachment 237762 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

I couldn't find any tests that cover this dialog. Did I miss one, or do I need to add one, or can I leave it be?

[Michael Catanzaro]

Created attachment 237809 [details]
Updated screenshot

The dimmed labels only appear dimmed after the window loses focus once. I'm not sure what's up with that....

[Michael Catanzaro]

Created attachment 237810 [details]
Patch

[WebKit Commit Bot]

Attachment 237810 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 237810 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should remove this line.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:-151
> -    GtkWidget* icon = gtk_image_new_from_stock(GTK_STOCK_DIALOG_AUTHENTICATION, GTK_ICON_SIZE_DIALOG);
> -    gtk_misc_set_alignment(GTK_MISC(icon), 0.5, 0.0);
> -    gtk_box_pack_start(GTK_BOX(authWidget), icon, FALSE, FALSE, 0);
> -    gtk_widget_show(icon);

Why removing the dialog icon?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:140
> +        /* Prompt on HTTP authentication dialog */

Use C++ comment, and finish it with period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:141
> +        _("Authentication required by %s:"),

Why removing the port?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:144
> +    gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);

Can we take advantage to not use GtkMisc?

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:158
> +    /* Check button on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:159
> +    priv->rememberCheckButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));

Why removing the ':' ? Were these strings translated? Since the file was not listed in the POTFILES.in. If we already have translations of these, maybe we can try to improve the dialog without changing strings, since it's late in the release cycle.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:162
> +    /* Entry on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:163
> +    GtkWidget* username_label = gtk_label_new_with_mnemonic(_("_Username"));

username_label -> usernameLabel

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:164
> +    GtkWidget* username_entry = createEntry(&priv->loginEntry);

username_entry -> usernameEntry

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:167
> +    /* Entry on the HTTP authentication dialog */

C++ comments finished with a period.

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:168
> +    GtkWidget* password_label = gtk_label_new_with_mnemonic(_("_Password"));

password_label -> passwordLabel

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:169
> +    GtkWidget* password_entry = createEntry(&priv->passwordEntry);

password_entry -> passwordEntry

> Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:175
> +#if GTK_CHECK_VERSION(3, 0, 0)
> +    gtk_style_context_add_class(gtk_widget_get_style_context(username_label), "dim-label");
> +    gtk_style_context_add_class(gtk_widget_get_style_context(password_label), "dim-label");
> +#endif

What is this dim-label class for? We can remove the GTK+2 support here, maybe we can do it that first, and then this patch would be simpler.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:71
> +    /* Title of the HTTP authentication dialog */

C++ comments finished with a period.

[Carlos Garcia Campos]

See https://bugs.webkit.org/show_bug.cgi?id=136700

[Michael Catanzaro]

(In reply to comment #9)
> > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:-151
> > -    GtkWidget* icon = gtk_image_new_from_stock(GTK_STOCK_DIALOG_AUTHENTICATION, GTK_ICON_SIZE_DIALOG);
> > -    gtk_misc_set_alignment(GTK_MISC(icon), 0.5, 0.0);
> > -    gtk_box_pack_start(GTK_BOX(authWidget), icon, FALSE, FALSE, 0);
> > -    gtk_widget_show(icon);
> 
> Why removing the dialog icon?

It looks bad!

Also, I'm working from a mockup in https://bugzilla.gnome.org/show_bug.cgi?id=727149

> > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:141
> > +        _("Authentication required by %s:"),
> 
> Why removing the port?

It's a confusing technical detail. Internet Explorer and Firefox don't show the port, so I think we can omit it too. Safari and Chrome do (at least in the screenshots I checked).

> > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:144
> > +    gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);
> 
> Can we take advantage to not use GtkMisc?

After bug #136700 is fixed, we can use gtk_widget_set_halign instead. Here it had to be GtkMisc (or an ifdef) for compatibility with Gtk+ 2.

> > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:159
> > +    priv->rememberCheckButton = gtk_check_button_new_with_mnemonic(_("_Remember password"));
> 
> Why removing the ':' ? Were these strings translated? Since the file was not listed in the POTFILES.in. If we already have translations of these, maybe we can try to improve the dialog without changing strings, since it's late in the release cycle.

Yes, it was translated, and it actually is listed in POTFILES.in. How about I write an initial version of this patch that uses the old strings, then a separate patch to update the strings after we have branched?

> > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:175
> > +#if GTK_CHECK_VERSION(3, 0, 0)
> > +    gtk_style_context_add_class(gtk_widget_get_style_context(username_label), "dim-label");
> > +    gtk_style_context_add_class(gtk_widget_get_style_context(password_label), "dim-label");
> > +#endif
> 
> What is this dim-label class for? We can remove the GTK+2 support here, maybe we can do it that first, and then this patch would be simpler.

It's supposed to make the text gray instead of black, like in the mockup, but there is some bug that causes this to happen only after the window loses focus. :/

[Michael Catanzaro]

(In reply to comment #11)
> > > Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:144
> > > +    gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);
> > 
> > Can we take advantage to not use GtkMisc?
> 
> After bug #136700 is fixed, we can use gtk_widget_set_halign instead. Here it had to be GtkMisc (or an ifdef) for compatibility with Gtk+ 2.

Unfortunately we are stuck with GtkMisc for now: https://bugzilla.gnome.org/show_bug.cgi?id=736489

[Michael Catanzaro]

Created attachment 237962 [details]
Patch

The strings are not good or final; they're designed to use only strings that were already marked for translation in the original dialog.

[Michael Catanzaro]

There's some alignment bug that didn't exist in my last patch....

[Michael Catanzaro]

Created attachment 237966 [details]
Patch

Alignment fixes

[Carlos Garcia Campos]

Comment on attachment 237966 [details]
Patch

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

Thanks, I would only change a couple of things before landing.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:122
> +        label = gtk_label_new(message.get());
> +        gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);
> +        gtk_label_set_line_wrap(GTK_LABEL(label), TRUE);
> +        gtk_label_set_max_width_chars(GTK_LABEL(label), 40);
> +        gtk_widget_show(label);
> +        gtk_box_pack_start(GTK_BOX(authBox), label, FALSE, FALSE, 0);

This is pretty much the same block than the previous one. We could move it to a helper function. Something like the createLabel you are removing, but without the padding parameter.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:132
> +    gtk_style_context_add_class(gtk_widget_get_style_context(loginLabel), "dim-label");

Use GTK_STYLE_CLASS_DIM_LABEL instead of "dim-label"

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:143
> +    gtk_style_context_add_class(gtk_widget_get_style_context(passwordLabel), "dim-label");

Ditto.

[Michael Catanzaro]

Created attachment 238026 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238026 [details]
Patch

Clearing flags on attachment: 238026

Committed r173560: <http://trac.webkit.org/changeset/173560>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.