Created attachment 83448 [details] [review]
the patch that works for me

I have some issues with this patch:

1) Why is it necessary to change the syntax of the command passed to the
   daemon?

+	/*printf ("%c%c%c%s\n", STX, BEL,
+	  GDM_INTERRUPT_SELECT_USER, login);*/
+	printf ("%c%s\n", STX, login);

2) This hack is ugly

+	/* HACK: determine whether selection changed because of key or
+	 * button press

   Why not fix the code so that we have a button press listener on the
   Face browser and make the code so that it does an actual select
   on the user only when the mouse button is pressed.  Then we could
   support keynav into the face browser in a cleaner way.  Setting the
   entry to "" when not button press is really ugly.

   You create button listeners but only to listen to when the last
   button was pressed.  Why not just make the button press select the
   user in the listview?

3) What is the purpose of this bit?

-       if ((event->keyval == GDK_Tab ||
-            event->keyval == GDK_KP_Tab) &&
-           (event->state & (GDK_CONTROL_MASK|GDK_MOD1_MASK|GDK_SHIFT_MASK)) == 0)
-          {
-		greeter_item_pam_login (GTK_ENTRY (entry), entry_info);
-		return TRUE;
-           }
-

4) How does this relate to a11y?

+			if (info->data.list.label_fgcolor != NULL) 
+				g_object_set (cell, "foreground",
+					      info->data.list.label_fgcolor,

5) Do we really want to do this?

+/* We *never* want to lose focus when we are in the process of
+ * authenticating the user */
+static gboolean 
+pam_focus_out_event (GtkWidget     *widget,
+		     GdkEventFocus *event,
+		     gpointer       user_data)
+{
+	if (!greeter_probably_login_prompt) {
+		gtk_widget_grab_focus (widget);
+	}
+	return FALSE;
+}
+
   Why can't the user tab into the face browser and pick a different
   user if they want, or decided that they selected the wrong username?
   Does it cause problems if the focus leaves the entry field when
   Password is being requested?

   Also, relying on greeter_probably_login_prompt isn't guaranteed to
   work with all PAM modules, so it isn't good to rely on, aside from
   trivial cosmetic things.

6) I notice you set the label for the entry field.  Does that mean you
   fixed bug #355005?  If so, can you mark that bug as a duplicate of
   this one?

7) You might querry Bill Haneman about making the labels work with a11y.
   I suspect that the best way to fix this would be to hack GDM to ditch
   using GREETER_TYPE_CANVAS_TEXT and instead use an actual GTK+ text
   label widgets in a canvas frame.

   I did similar work with GDM to fix the buttons so that they use 
   actual GTK+ button widgets rather than using CANVAS items, so if you 
   want to check that patch it might help you understand the sort of
   work involved with making this change.  I did that code change on
   04-26-2006, so you should be able to extract the patch from svn.

Note this is related t bug #355005.  I previously tried to get the atk label to work in greeter_item_pam_prompt, but couldn't figure out how to get it to work.
I was previously trying to set LABEL_FOR and LABELLED_BY, but this was causing
the greeter to core dump.  I think because gdmgreeter uses GNOME_CANVAS_TEXT,
which doesn't use GtkText widgets.  

I applied the bit of your patch that adds the label to the entry field.

However, I think the above issues need to be addressed if we want to support
a11y in the face browser.

Also, it seems that you only fixed the Face Browser in gdmgreeter, not gdmlogin.
It should be fixed in both places.

David, any update or interest in working further on this patch?

Created attachment 88622 [details] [review]
patch updated to 2.19.1

I think this patch needs a bit of love before it can go upstream.  From a usability perspective, is it really so valuable to avoid selecting the user when the user moves up and down the list via the keyboard?  

Also, does this patch actually work?  Note that the original patch has some modifications to daemon/verify-pam.c to turn off the feature that makes Tab act like Enter.  Without removing this, the user can't "Tab" to move focus into the face browser (or other widgets).  Note related bug #433495.

I know gdmgreeter has a lot of logic to force focus to be in the entry field all the time which would need to be looked at to make sure that the focus doesn't jump around when the user does specific actions.  I'm not sure that this has been really tested.

This patch would need to also work for gdmlogin before I'd accept it.  I really don't want Tab or focus handling to be so significantly different between the two programs.

If someone wants to look into this, I'd accept a patch. 

Changing the synopsis since the a11y features of this have been applied (making the entry field have a label).  Also now GDM does support using Tab to move focus to other widgets.  However, the face browser is not yet key navigable, so making the synopsis indicate that this is the remaining issue.

This is fixed in 2.21 also, so we may just want to close this bug out.

Yup.  Fixed in trunk.