GNOME Bugzilla – Bug 726759
app picker keynav: keep focus visible
Last modified: 2014-03-26 10:59:05 UTC
When using arrow keys to move the focus in the app picker, I can arrow down into the second page. Turning on orca reveals that I can navigate the focus around on the invisible page just fine. Unfortunately, the shell doesn't ensure that the page containing the focus is visible, which is the behaviour I would expect.
Created attachment 272485 [details] [review] search: Ensure that the default result is visible in the scroll view The default result is set to selected when key focus enters the search entry. We must also scroll the view if needed. -- This one wasn't reported here but since I was looking into this...
Created attachment 272486 [details] [review] iconGrid: Add a key-focus-in signal This fires whenever a grid's child emits its own key-focus-in signal.
Created attachment 272487 [details] [review] appDisplay: Ensure the currently focused icon is viewable We either scroll or paginate to the correct place when an icon gets key focus.
Review of attachment 272485 [details] [review]: OK.
Review of attachment 272486 [details] [review]: If we do want to go through IconGrid, this looks good.
Review of attachment 272487 [details] [review]: needs-work due to the wrong handling in FrequentView, plus one general comment: ::: js/ui/appDisplay.js @@ -559,3 @@ }, - _ensureIconVisible: function(icon) { So this was broken in commit 9df09db5fe7c4de which removed the functions that set up the key-focus-in signal without adding it to the replacement; maybe the easiest fix is to just overwrite addItem() in AllView to also set up the signal connection? @@ +672,3 @@ + _keyFocusIn: function(icon) { + let itemPage = this._grid.getItemPage(icon); + this.goToPage(itemPage); FrequentView is not paginated, so this won't work (it won't ever be scrolled either, so we don't need any special dealing with visibility here anyway)
Created attachment 272781 [details] [review] appDisplay: Ensure the currently focused icon is viewable -- (In reply to comment #6) > - _ensureIconVisible: function(icon) { > So this was broken in commit 9df09db5fe7c4de which removed the > functions that set up the key-focus-in signal without adding it to > the replacement; maybe the easiest fix is to just overwrite > addItem() in AllView to also set up the signal connection? I'd prefer to go through IconGrid since there's code outside of BaseAppView doing this._grid.addItem() which probably should be cleaned up but it's a separate issue. > FrequentView is not paginated, so this won't work (it won't ever be > scrolled either, so we don't need any special dealing with > visibility here anyway) Ok, removed this and added an empty default vfunc to BaseAppView instead.
Review of attachment 272781 [details] [review]: OK
Attachment 272485 [details] pushed as e339e26 - search: Ensure that the default result is visible in the scroll view Attachment 272486 [details] pushed as 8d8c75d - iconGrid: Add a key-focus-in signal Attachment 272781 [details] pushed as d0f69a7 - appDisplay: Ensure the currently focused icon is viewable