Created attachment 271171 [details] [review]
Search result popover becomes keyboard accessible

I want to propose a patch for this. This patch becomes the search result popover focusable by keyboard and force focus on it when the search results are ready (so, when the treeview is populated). Then, the treeview rows are activated using the 'row-activated' signal instead of using the tricky 'button-press-event' way. User can also activate a row with a mouse click as before thanks to the 'activate-on-single-click' property.

Any review will be highly appreciated!

Review of attachment 271171 [details] [review]:

::: src/mainWindow.js
@@ +246,3 @@
         this._searchPopup.updateResult(places, this._searchEntry.get_text());
         this._searchPopup.showResult();
+        this._searchPopup._treeView.grab_focus();

Hmmm... Maybe this breaks encapsulation, SearchPopup should have a method to grab the focus on the proper widget.

Review of attachment 271171 [details] [review]:

Thanks, the patch looks great!

You could look a bit at: https://wiki.gnome.org/Git/CommitMessages for tips on the commit message.

::: src/mainWindow.js
@@ +246,3 @@
         this._searchPopup.updateResult(places, this._searchEntry.get_text());
         this._searchPopup.showResult();
+        this._searchPopup._treeView.grab_focus();

Maybe you could override searchPopups grab_focus method with a vfunc_grab_focus? So that this becomes this._searchPopup.grab_focus().

Review of attachment 271171 [details] [review]:

::: src/mainWindow.js
@@ +246,3 @@
         this._searchPopup.updateResult(places, this._searchEntry.get_text());
         this._searchPopup.showResult();
+        this._searchPopup._treeView.grab_focus();

Or maybe even better to add a this._treeView.grab_focus() in the showResult method of searchPopup. So that we do not need to explicitly grab focus for the popup each time we want to showResult.

Created attachment 271255 [details] [review]
Search result popover becomes keyboard accessible - v2

Thanks for the review,

So now the focus grabbing is responsability of the SearchPopup.

Something I don't like yet is that when you press "Escape" having focus on the popover, the focus doesn't return to the search field.

I was about to fix that by using "closed" signal, assuming that this signal is emmited when the popover disappear, but that maybe will cause undesired focus behaviour (if I press/touch another point on the window to close the popover, there is no need to grab focus on the search entry.)

Review of attachment 271255 [details] [review]:

Thanks, great work!

The patch looks fine, and I am going to commit it. I will however change the commit message. In Maps have been using the style of using full link to the bug, partly because many of the developers use the git bz script that adds this by default.

I'm not that concerned with the focus not going back to search on Escape. I'm not sure it should. We can re-visit it if we find out that it's needed.