GNOME Bugzilla – Bug 793100
Search popover and keyboard navigation: lack of scroll, movement by two items at a time.
Last modified: 2018-02-05 20:56:24 UTC
Created attachment 367764 [details] [review] Fix key navigation in search popover. When focus is on the search entry and the search results are shown in popover below it, it is possible to move around with keys up / down, etc., while entry remains focused. Unfortunatelly this suffers from two issues: * Moving to the next / previous item does not scroll to it, thus selected item may remain completely invisible. * When moving in upwards direction, movement is performed two items at a time.
Review of attachment 367764 [details] [review]: ::: src/searchPopover.js @@ +50,2 @@ _propagateKeys: function(entry, event) { + const keyval = event.get_keyval()[1]; Use "let" here to stay consistent with the rest of the code, see https://wiki.gnome.org/Projects/Gjs/StyleGuide @@ +62,3 @@ + // If we get an 'enter' keypress and we have a selected + // row, we do not want to propagate the event. + const row = this._list.get_selected_row(); We have used "let" to assign local-scope variables, so we should stick with that. Also, I think I prefer a blank line before the if-statement here for clarity. @@ +78,1 @@ + const length = this._list.get_children().length; Like above, "let" here… @@ +82,3 @@ + const direction = (keyval === Gdk.KEY_KP_Up || keyval === Gdk.KEY_Up) ? -1 : 1; + const row = this._list.get_selected_row(); "let" here @@ +89,3 @@ + idx = row.get_index() + direction; + } + const in_bounds = 0 <= idx && idx < length; We have consistently used camelCase for variables and functions/methods, so: let inBounds = ... @@ +105,3 @@ + _selectRow: function(row) { + this._list.select_row(row); + const adjustment = this._list.get_adjustment(); "let" here, or just use the expression directly in the if-statement. @@ +107,3 @@ + const adjustment = this._list.get_adjustment(); + if (adjustment) { + const allocation = row.get_allocation(); "let" here
Thanks for the patch! I might not have time to look at it into more detail before beginning of next week, as I will travel to Brussels tomorrow (for FOSDEM).
Created attachment 367796 [details] [review] Fix key navigation in search popover. You can find updated patch attached.
Review of attachment 367796 [details] [review]: LGTM (we generally try to follow a style of "module: Description of patch" for the headline of a commit, but I'll amend the patch before pushing it)
Attachment 367796 [details] pushed as 5e7af43 - Fix key navigation in search popover.
Seems to work nicely, thanks!