I'd describe this problem slightly differently.

Steps to reproduce:

1. Log in to a website for which epiphany can remember your username and password (e.g. the Adobe bug tracker at http://bugs.adobe.com/jira/login.jsp, using details from bugmenot.com)
2. Click "Store password" in the info bar
3. Log out of the website
4. Log in to the same website using a *different* username and password
5. Click "Store password" in the info bar
6. Log out of the website
7. Verify that seahorse (GNOME "Passwords and Keys" app) shows both sets of saved credentials
8. Navigate to the website's login page

What you expected to happen:

Both saved usenames are shown in a drop-down (or in the info bar); clicking either updates the pre-filled account details in the username / password boxes.

What actually happens:

Details for the first account stored are always pre-filled.

In addition, entering details of the second account (which must be done manually) results in another "Would you like to store the password for ..." infobar pop-up.

I'd suggest that saving multiple accounts when it's not actually possible to recall any beyond the first is a bug, not an enhancement. If it's not feasible to make a UI change to allow selecting from multiple accounts, I think preventing the user from saving more than one account (i.e. not offering "Would you like to store the password ...") for the same site/login page would be preferable to the current situation.

I'm working on a UI for this. The basic functionality for this is in bug 699606.

Created attachment 250824 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking.

There's a lot to be done for this patch still - need to figure out how to properly style the box (though I did enjoy having it use the site's style, I think it may break badly in some sites), and add keyboard handling/a11y stuff. I started by trying to use GtkMenu for the options, but the fact that it was being handled by the WebProcess seemed to trigger some funny interactions, and the mouse/keyboard grab was pretty sucky. Opinions on that?

Created attachment 251267 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking or using the
arrow keys.

I think this is pretty complete, now. This patch requires this to build and work: https://bugs.webkit.org/show_bug.cgi?id=119651

Review of attachment 251267 [details] [review]:

Looks good in general, but I have not had the chance to test it because the web extension is not working right now for me reliably.

::: embed/web-extension/ephy-web-extension.c
@@ +526,3 @@
+  webkit_dom_element_set_attribute (main_div, "id", "ephy-user-choices-container", NULL);
+
+                               &color);

This 2147483647 is worth explaining or a macro :)

@@ +650,3 @@
+    return TRUE;
+
+

Chrome doesn't show the list when you first focus in the node. I find this sensible because of user privacy, but I have no strong opinion.

@@ +685,3 @@
+  else if (!g_strcmp0 (webkit_dom_keyboard_event_get_key_identifier (keyboard_event), "Down"))
+    keyval = GDK_KEY_Down;
+  ephy_web_dom_utils_get_absolute_bottom_for_element (WEBKIT_DOM_ELEMENT (username_node), &x, &y);

What is U+001B? Add this as a comment.

@@ +723,3 @@
+
+    iter = webkit_dom_element_get_next_element_sibling (iter);
+

Would it be possible to do this by remembering the selected element instead of iterating to find it? Would that be more scalable and/or clean?

::: lib/ephy-web-dom-utils.c
@@ +425,3 @@
+ * to the origin of the page.
+ **/
+ * @x: return address for the x coordinate.

return type in a separate line.

@@ +461,3 @@
+ * to the origin of the page.
+ **/
+

ditto.

I tested it now. I get a lot of warnings with twitter, for example:

** (WebKitWebProcess:20197): CRITICAL **: gchar* webkit_dom_node_get_text_content(WebKitDOMNode*): assertion 'WEBKIT_DOM_IS_NODE(self)' failed

** (WebKitWebProcess:20197): CRITICAL **: void webkit_dom_html_input_element_set_value(WebKitDOMHTMLInputElement*, const gchar*): assertion 'value' failed

** (WebKitWebProcess:20197): CRITICAL **: gchar* webkit_dom_node_get_text_content(WebKitDOMNode*): assertion 'WEBKIT_DOM_IS_NODE(self)' failed

** (WebKitWebProcess:20197): CRITICAL **: void webkit_dom_html_input_element_set_value(WebKitDOMHTMLInputElement*, const gchar*): assertion 'value' failed

** (WebKitWebProcess:20197): CRITICAL **: void webkit_dom_element_set_attribute(WebKitDOMElement*, const gchar*, const gchar*, GError**): assertion 'WEBKIT_DOM_IS_ELEMENT(self)' failed

** (WebKitWebProcess:20197): CRITICAL **: void webkit_dom_element_set_attribute(WebKitDOMElement*, const gchar*, const gchar*, GError**): assertion 'WEBKIT_DOM_IS_ELEMENT(self)' failed

** (WebKitWebProcess:20197): CRITICAL **: WebKitDOMElement* webkit_dom_element_get_first_element_child(WebKitDOMElement*): assertion 'WEBKIT_DOM_IS_ELEMENT(self)' failed

** (WebKitWebProcess:20197): CRITICAL **: void webkit_dom_element_set_attribute(WebKitDOMElement*, const gchar*, const gchar*, GError**): assertion 'WEBKIT_DOM_IS_ELEMENT(self)' failed

Some other comments (all tested in twitter):

- When I enter twitter.com, the page puts the focus immediately in the username field. That pops up the list immediately. Perhaps this is also a good reason to wait until the second activation.
- Typing a username not in the list and then trying to click in the password entry is not possible. You need to click somewhere else or use tab. This is not good.
- Clicking in an entry different than the selected one doesn't always change the input field to update it.
- When typing, it would be great to reduce displayed list to match what I am entering. This would also avoid us from obscuring the password field.

(In reply to comment #7)
> Chrome doesn't show the list when you first focus in the node. I find this
> sensible because of user privacy, but I have no strong opinion.

hmm, this might be a good idea. Wonder if we could also differentiate focus caused by user action from auto-focus, show it for the former, not for the later
 
> @@ +723,3 @@
> +
> +    iter = webkit_dom_element_get_next_element_sibling (iter);
> +
> 
> Would it be possible to do this by remembering the selected element instead of
> iterating to find it? Would that be more scalable and/or clean?

I'll give that approach a try, should be easy to keep track of that state using GObject data.
 
> ::: lib/ephy-web-dom-utils.c
> @@ +425,3 @@
> + * to the origin of the page.
> + **/
> + * @x: return address for the x coordinate.
> 
> return type in a separate line.

I did not understand this, these are return (out) parameters, similar to the ones used in ephy_web_dom_utils_get_best_icon, do you want to have empty lines separating those, or did you confuse this with the function's return?

I'll work on addressing the other comments and the bugs you found with your testing! Will hopefully have a new patch up by tomorrow.

I only meant to say that the void should go in a different line:

  void
  foo ()

instead of

  void foo()

I now noticed the same applies to other methods. A nit anyway!

An update:

(In reply to comment #8)
> ** (WebKitWebProcess:20197): CRITICAL **: gchar*
> webkit_dom_node_get_text_content(WebKitDOMNode*): assertion
> 'WEBKIT_DOM_IS_NODE(self)' failed

Ooops, this was a last-minute change in which node I was passing to one of the helpers, fixed locally.

> - When I enter twitter.com, the page puts the focus immediately in the username
> field. That pops up the list immediately. Perhaps this is also a good reason to
> wait until the second activation.

So I think there's no sane way right now of using focus for this. I agree with you it's bad to activate when the page focuses the entry, but it looks like there's no good way of doing what I think would be the solution - distinguishing user-triggered from page-triggered focus. So my suggestion would be to just not use focus for this - we will show the list on the following ocasions:

* the entry is clicked
* the user hits down or up arrow
* the user starts typing (we will then show a filtered list)

> - Typing a username not in the list and then trying to click in the password
> entry is not possible. You need to click somewhere else or use tab. This is not
> good.

Yeah… this will be fixed by the filtering.

> - Clicking in an entry different than the selected one doesn't always change
> the input field to update it.

This is fixed locally as well, I did not have the <a> tag take up the whole of the <li>

> - When typing, it would be great to reduce displayed list to match what I am
> entering. This would also avoid us from obscuring the password field.

Only this left to do, can probably finish this tonight and upload the new patch =)

(In reply to comment #10)
> I only meant to say that the void should go in a different line:
[…]
> I now noticed the same applies to other methods. A nit anyway!

Oh, doh, the context in the comment misled me, will fix those =)

Created attachment 253079 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking or using the
arrow keys.

Created attachment 253080 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking or using the
arrow keys.

Created attachment 253081 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking or using the
arrow keys.

Err, sorry for the noise, had forgotten to put the return type of a bunch of methods in their own lines, and then I realized I had a few left-overs.

So this version works like this:

* When the input is focused nothing happens.
* Clicking the input shows the options
* While with the input field focused (by tabing to it or if the page focuses it like twitter does) the options will also be shown if the user presses the down or up arrows
* Editing the username field shows the options and filters them according to the string that is in the field
* Clicking elsewhere in the page or unfocusing the input hides the options

I think this should address all of the issues that were identified. I tested this version with twitter, amazon aws and rimuhosting, works nicely =)

Review of attachment 253081 [details] [review]:

It's looking much better! Thanks a lot. There are a few issues I still notice, though (always twitter):

- Type a username that is not already saved and press an arrow key, the webprocess crashes. Same happens when you fully enter a username already saved. Seems to be an issue when there is no popup element (blocker).
- Arrow-down, select an element, press enter. The password is not updated apparently because the login fails (major).
- Click on the entry, click on an unselected element, login. The login fails (I guess the password was not updated) (major).
- Pressing Tab in the username entry moves the focus to the password one. Initially, the full password is selected, but the passwords seems to be changed and the cursor moves after the password. Perhaps it is possible to intercept the focus-out handler, update the password field, and only then let the event propagate, dunno (minor).
- If you type an username not available, the latest pre-filled password is preserved. Maybe we should empty it (minor).

I am worried mostly about the three first items, if you think we can get them fixed today/tomorrow I guess we can still push them before I release with UI freeze in mind.

As for the code itself, didn't have time now to review it, so will probably do later.

Created attachment 253214 [details] [review]
Show a box with a list of available users for a login form

If the login input is clicked, or focused the list of available
users is shown, and one can be chosen by clicking or using the
arrow keys.

This version should fix all 4 items =)

Review of attachment 253214 [details] [review]:

Okay. I must confess I am not familiar with DOM hacking so my review might be a bit shallow and some comments might not make much sense, but in any case, I think this is looking and working good enough to go in. Please address the few things I comment and commit to master! Thanks a lot!

::: embed/web-extension/ephy-web-extension.c
@@ +387,3 @@
+  user_choices = webkit_dom_document_get_element_by_id (document, "ephy-user-choices-container");
+  if (user_choices) {
+  WebKitDOMElement *user_choices;

Why is that extra reference needed?

@@ +500,3 @@
+    g_string_append_printf (style_attribute, "background-color: %s;", color);
+    g_free (color);
+static char*

Please factor out the redundant code, leave only the function call (you can simply use the ? operator). Also, unsure that we need GString at all. Just fill this one earlier.

@@ +524,3 @@
+    g_string_append_printf (style_attribute, "color: %s;", color);
+    g_free (color);
+{

Same here.

@@ +562,3 @@
+  g_string_append (style_attribute, "border-top: 0;");
+  g_string_append (style_attribute, "border-radius: 8px;");
+get_user_choice_style (gboolean selected)

I know GString is handy and the code is more readable, but I don't think we need it here. A single g_strdup_printf() or similar suffices if you fetch the absolute bottom before.

@@ +601,3 @@
+     * input.
+     */
+

If I understand this right, you can skip the whole cycle if username_node_ever_edited is FALSE, no? Maybe it's enough with returning early in that case?

@@ +747,3 @@
+
+  /* Fetch the first li. */
+                                NULL);

I think you're not using this anymore.

@@ +755,3 @@
+    if (keyval == GDK_KEY_Up)
+      to_select = webkit_dom_element_get_previous_element_sibling (selected);
+                                    "padding: 0;",

But you already know selected is TRUE here, no?

@@ +764,3 @@
+    else if (keyval == GDK_KEY_Down)
+      to_select = webkit_dom_element_get_first_element_child (container);
+

This is really minor and I wouldn't worry about it now, but I think we shouldn't cycle the selection if we don't do it in other WKGTK+ popups, and I suspect this code does.

@@ +910,3 @@
+        webkit_dom_event_target_add_event_listener (WEBKIT_DOM_EVENT_TARGET (username_node), "blur",
+                                                    G_CALLBACK (username_node_changed_cb), FALSE,
+        g_object_set_data (G_OBJECT (username_node), "ephy-document", document);

Are the two handlers needed or would "blur" be suficient?

Review of attachment 253214 [details] [review]:

I noticed I haven't fixed the "password changes after blur and is thus left unselected" issue. I got tempted to fix it, but I'm afriad of doing any logic changes this late, so I will fix that in a follow up patch.

::: embed/web-extension/ephy-web-extension.c
@@ +387,3 @@
+  user_choices = webkit_dom_document_get_element_by_id (document, "ephy-user-choices-container");
+  if (user_choices) {
+    g_object_ref (user_choices);

I don't think it is - I removed it and nothing broke after quite a bit of testing… it's probably a left over from a very early debugging-by-shooting-in-every-direction attempt.

@@ +500,3 @@
+    g_string_append_printf (style_attribute, "background-color: %s;", color);
+    g_free (color);
+  }

Wow, yeah, this function sux. I blame being drunk or something.

@@ +562,3 @@
+  g_string_append (style_attribute, "border-top: 0;");
+  g_string_append (style_attribute, "border-radius: 8px;");
+  g_string_append (style_attribute, "-webkit-user-modify: read-only ! important;");

done! should have revisited the code after it was finished, GString was pretty handy for not having to care a lot about where stuff got fetched while developing but was indeed not required

@@ +601,3 @@
+     * input.
+     */
+    if (username_node_ever_edited && !g_str_has_prefix(data->username, username))

It's the opposite, actually: If username_node_ever_edited is false we want to add all options to the list - meaning we do not even want to filter. We only skip the loop iteration to implement the filtering.

@@ +747,3 @@
+
+  /* Fetch the first li. */
+  iter = webkit_dom_element_get_first_element_child (container);

indeed!

@@ +755,3 @@
+    if (keyval == GDK_KEY_Up)
+      to_select = webkit_dom_element_get_previous_element_sibling (selected);
+    else if (selected && keyval == GDK_KEY_Down)

Oops =) refactoring and not reviewing ;)

@@ +764,3 @@
+    else if (keyval == GDK_KEY_Down)
+      to_select = webkit_dom_element_get_first_element_child (container);
+  }

As discussed on IRC, both GTK+ popup menus and WebKitGTK+ <select>s cycle, so we should too.

@@ +910,3 @@
+        webkit_dom_event_target_add_event_listener (WEBKIT_DOM_EVENT_TARGET (username_node), "blur",
+                                                    G_CALLBACK (username_node_changed_cb), FALSE,
+                                                    web_page);

change will also catch things like the username being changed by a script, so I think we should keep both

Attachment 253214 [details] pushed as 1d8b0fe - Show a box with a list of available users for a login form