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
Created attachment 237760 [details] Screenshot of proposed changes
Created attachment 237762 [details] Patch
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
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.
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?
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....
Created attachment 237810 [details] Patch
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.
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.
See https://bugs.webkit.org/show_bug.cgi?id=136700
(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. :/
(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
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.
There's some alignment bug that didn't exist in my last patch....
Created attachment 237966 [details] Patch Alignment fixes
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.
Created attachment 238026 [details] Patch
Comment on attachment 238026 [details] Patch Clearing flags on attachment: 238026 Committed r173560: <http://trac.webkit.org/changeset/173560>
All reviewed patches have been landed. Closing bug.