After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 726759 - app picker keynav: keep focus visible
app picker keynav: keep focus visible
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: overview
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-03-20 02:59 UTC by Matthias Clasen
Modified: 2014-03-26 10:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
search: Ensure that the default result is visible in the scroll view (1.63 KB, patch)
2014-03-20 13:58 UTC, Rui Matos
committed Details | Review
iconGrid: Add a key-focus-in signal (1.61 KB, patch)
2014-03-20 13:58 UTC, Rui Matos
committed Details | Review
appDisplay: Ensure the currently focused icon is viewable (1.89 KB, patch)
2014-03-20 13:58 UTC, Rui Matos
needs-work Details | Review
appDisplay: Ensure the currently focused icon is viewable (1.80 KB, patch)
2014-03-24 14:07 UTC, Rui Matos
committed Details | Review

Description Matthias Clasen 2014-03-20 02:59:12 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.
Comment 1 Rui Matos 2014-03-20 13:58:15 UTC
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...
Comment 2 Rui Matos 2014-03-20 13:58:23 UTC
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.
Comment 3 Rui Matos 2014-03-20 13:58:32 UTC
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.
Comment 4 Florian Müllner 2014-03-20 17:46:10 UTC
Review of attachment 272485 [details] [review]:

OK.
Comment 5 Florian Müllner 2014-03-20 17:46:18 UTC
Review of attachment 272486 [details] [review]:

If we do want to go through IconGrid, this looks good.
Comment 6 Florian Müllner 2014-03-20 17:46:24 UTC
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)
Comment 7 Rui Matos 2014-03-24 14:07:29 UTC
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.
Comment 8 Florian Müllner 2014-03-24 15:04:10 UTC
Review of attachment 272781 [details] [review]:

OK
Comment 9 Rui Matos 2014-03-26 10:58:55 UTC
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