GNOME Bugzilla – Bug 750689
userList: Entry does not always have focus
Last modified: 2015-07-31 18:22:34 UTC
Created attachment 304928 [details] [review] userList: entry should grab focus when revealed if a userlist is opened after changing chatroom, the entry does not have focus. this patch calls gtk_widget_grab_focus on the GtkEntry to ensure the search field always have focus when the revealer is opened.
Review of attachment 304928 [details] [review]: The commit message is correct, but the code does not match it - it simply grabs the focus whenever the entry's visibility *may* change. That's not what we want: - if the entry is hidden (i.e. a room with <5 users), it does not make sense to focus it (if the entry hasn't been shown before, it even breaks keynav of the user list completely) - user actions like navigating inside the list and expanding an entry should *not* move the focus (Apart from those major issues, there's also a nit regarding the commit message: sentences start capitalized, and the subject line should use sentence capitalization)
Created attachment 305083 [details] [review] userList: Entry should grab focus when revealed thanks for the clarification. do you think it might be better to connect to notify::visible instead?
Review of attachment 305083 [details] [review]: Sorry, had this tab open since mid-June, but only got to the point how much I dislike this approach - finally got some time to look into this properly ... ::: src/userList.js @@ +29,3 @@ })); + this.widget.connect('notify::visible', Lang.bind(this, function() { + if (this.widget.visible && this._userList.numRows > MAX_USERS_SHOWN) Duplicating the condition for showing the entry is bad. @@ +30,3 @@ + this.widget.connect('notify::visible', Lang.bind(this, function() { + if (this.widget.visible && this._userList.numRows > MAX_USERS_SHOWN) + this._entry.grab_focus(); GTK+ already moves the focus into the popover when showing it. Under some conditions this focus is wrong, and we should figure out why and address it. Not papering over the issue by picking some signal that happens to work to explicitly set the correct focus.
Created attachment 308423 [details] [review] userList: Fix focusing the search entry after room changes GtkContainer's default focus chain is based on its children's allocated y position and height[0]. This is problematic when the user list has just been created and doesn't have a proper allocation yet, as it will wrongly sort before the search entry in that case. Work around this by adding the user list to a dedicated bin (that is kept around on room changes) instead of directly to the main box. [0] https://git.gnome.org/browse/gtk+/tree/gtk/gtkcontainer.c#n2737
Created attachment 308424 [details] [review] userList: Fix focusing the search entry after room changes GtkContainer's default focus chain is based on its children's allocated y position and height[0]. This is problematic when the user list has just been created and doesn't have a proper allocation yet, as it will wrongly sort before the search entry in that case. Work around this by overwriting the default focus chain with our own. [0] https://git.gnome.org/browse/gtk+/tree/gtk/gtkcontainer.c#n2737
Either of the patches above addresses the issue, but I haven't quite made up my mind which approach I like better.
We should eventually address this in GTK+, but it's an annoying issue, so push one of the fixes now. Attachment 308424 [details] pushed as 6425e8d - userList: Fix focusing the search entry after room changes