Created attachment 278220 [details] [review]
appDisplay: Move focus to popup only if necessary

Currently we move the focus to the popup every time we
open a popup.
However, the first item is selected always although the
user didn't press any key.
To avoid that, we move the focus only if we press a key.
To do that we have to steal the focus from the AllView
IconGrid, since the focus is trapped there.
To achieve that add a check on the key event handler in
AllView and move the focus to the current popup if it exists.

Review of attachment 278220 [details] [review]:

No. For a start, I think that the current behavior is correct when opening the popup via keynav, so we only want to change it otherwise (e.g. when clicking the folder button). But more importantly, this breaks stuff left and right when the focus is *not* on the popup:
 - hitting escape closes the app picker instead of the popup
 - hitting space/return triggers the make-space-for-popup animation again
 - hitting tab/arrows will move focus *both* inside the parent and
   into the popup

Created attachment 278270 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Not sure how much necessary is the long comment inside the code, but it's kinda difficult to understand why we are doing that without and explanation while reading the code.

Review of attachment 278270 [details] [review]:

Mostly nits, but the handling of up/down is wrong and needs fixing.

::: js/ui/appDisplay.js
@@ +1239,3 @@
+        // when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
+        // keys
+        this.actor.connect('key-press-event', Lang.bind(this,

The method is a bit long for an anonymous function imho (not to mention that it is accompanied by a comment of almost the same length) ...

@@ +1241,3 @@
+        this.actor.connect('key-press-event', Lang.bind(this,
+            function(actor, event) {
+                if (global.stage.get_key_focus() == actor) {

I'd prefer an early return here:

if (global.stage.get_key_focus() != actor)
    return Clutter.EVENT_PROPAGATE;

@@ +1250,3 @@
+                        case Clutter.Down:
+                        case Clutter.Right:
+                            direction = isLtr ? Gtk.DirectionType.TAB_FORWARD :

No, the handling of Down (and Up below) is wrong - mirroring only makes sense for horizontal directions. If you want to avoid code duplication, you could do something like:

  let needsMirroring = false;
  switch(...) {
    case Clutter.Right:
        needsMirroring = isRtl;
    case Clutter.Down:
    case Clutter.Tab:
        direction = Gtk.DirectionType.TAB_FORWARD;
        break;

    case Clutter.LEFT:
        ....
    }

    if (needsMirroring)
        direction = (direction == Gtk.DirectionType.TAB_FORWARD) ? Gtk.DirectionType.TAB_BACKWARD
                                                                 : Gtk.DirectionType.TAB_FORWARD;

But then case statements without break are not that great for readability ...

@@ +1325,3 @@
+        
+        // Grab focus on the AppFolderPopup actor withouth focusing
+        // a child until a key press is actually pressed by the user

Not sure the comment adds much value tbh

(In reply to comment #5)
> Review of attachment 278270 [details] [review]:
> 
> Mostly nits, but the handling of up/down is wrong and needs fixing.
> 
> ::: js/ui/appDisplay.js
> @@ +1239,3 @@
> +        // when that happens using TAB_FORWARD or TAB_BACKWARD instead of
> arrow
> +        // keys
> +        this.actor.connect('key-press-event', Lang.bind(this,
> 
> The method is a bit long for an anonymous function imho (not to mention that it
> is accompanied by a comment of almost the same length) ...
right
> 
> @@ +1241,3 @@
> +        this.actor.connect('key-press-event', Lang.bind(this,
> +            function(actor, event) {
> +                if (global.stage.get_key_focus() == actor) {
> 
> I'd prefer an early return here:
> 
> if (global.stage.get_key_focus() != actor)
>     return Clutter.EVENT_PROPAGATE;
> 
> @@ +1250,3 @@
> +                        case Clutter.Down:
> +                        case Clutter.Right:
> +                            direction = isLtr ? Gtk.DirectionType.TAB_FORWARD
> :
> 
> No, the handling of Down (and Up below) is wrong - mirroring only makes sense
> for horizontal directions. If you want to avoid code duplication, you could do
> something like:
> 
>   let needsMirroring = false;
>   switch(...) {
>     case Clutter.Right:
>         needsMirroring = isRtl;
>     case Clutter.Down:
>     case Clutter.Tab:
>         direction = Gtk.DirectionType.TAB_FORWARD;
>         break;
> 
>     case Clutter.LEFT:
>         ....
>     }
> 
>     if (needsMirroring)
>         direction = (direction == Gtk.DirectionType.TAB_FORWARD) ?
> Gtk.DirectionType.TAB_BACKWARD
>                                                                  :
> Gtk.DirectionType.TAB_FORWARD;
> 
> But then case statements without break are not that great for readability ...
> 
sigh right.
Yeah, that case statement confused me tbh. I would see what can I do.
> @@ +1325,3 @@
> +        
> +        // Grab focus on the AppFolderPopup actor withouth focusing
> +        // a child until a key press is actually pressed by the user
> 
> Not sure the comment adds much value tbh
Right

Created attachment 278275 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Created attachment 278518 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Review of attachment 278518 [details] [review]:

My keyboard has a couple more keys than left, right, up, down, tab and escape (try something like ctrl-alt-tab or alt-f2 after opening the popup and see it break) ...

Can't you just override the handling of the arrow keys and not interfere with the already correct behavior otherwise?

Created attachment 280975 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Review of attachment 280975 [details] [review]:

::: js/ui/appDisplay.js
@@ +1263,3 @@
+        if (event.get_key_symbol() == Clutter.KEY_Escape) {
+            this.popdown();
+            return Clutter.EVENT_STOP;

Why do you need this?

(In reply to comment #11)
> Review of attachment 280975 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +1263,3 @@
> +        if (event.get_key_symbol() == Clutter.KEY_Escape) {
> +            this.popdown();
> +            return Clutter.EVENT_STOP;
> 
> Why do you need this?

Oh sorry, your grabHelper change was after this patch, so rebasing was not "enough" and I didn't notice that change, so no longer needed.

Created attachment 281000 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Review of attachment 281000 [details] [review]:

::: js/ui/appDisplay.js
@@ +1259,3 @@
+    _onKeyPress: function(actor, event) {
+        if (!this._isOpen)
+            return Clutter.EVENT_PROPAGATE;

Mmmh, is there a case where this._isOpen is false but the popup actor has key focus?

@@ +1288,3 @@
+            case Clutter.Right:
+                direction = isLtr ? Gtk.DirectionType.TAB_FORWARD :
+                                    Gtk.DirectionType.TAB_BACKWARD;

Whoops, wrong direction for up/down under RTL locales ...

Created attachment 281021 [details] [review]
appDisplay: Move focus to popup only when necessary

Since we need to only grab focus on one item child when the user
actually press a key we don't use navigate_focus when opening
the popup.
Instead of that, grab the focus on the AppFolderPopup actor
and actually moves the focus to a child only when the user
actually press a key.
It should work with just grab_key_focus on the AppFolderPopup
actor, but since the arrow keys are not wrapping_around the focus
is not grabbed by a child when the widget that has the current focus
is the same that is requesting focus, so to make it works with arrow
keys we need to connect to the key-press-event and navigate_focus
when that happens using TAB_FORWARD or TAB_BACKWARD instead of arrow
keys

Review of attachment 281021 [details] [review]:

The commit message could use some updates (talking about "grab_key_focus", though that's no longer called directly; also: "the user [...] press" instead of "the user [...] presses" a couple of times). How about:

Unlike for the main app view, where we only move the key focus once the users starts navigating, 
the key focus is moved immediately when opening a folder popup. This is unexpected, so make app 
folders consistent with the main view - as arrow keys will not work while the container itself
has key focus, we handle those explicitly by translating them to TAB_FORWARD and TAB_BACKWARD
respectively.

::: js/ui/appDisplay.js
@@ +1297,3 @@
+            default:
+                return Clutter.EVENT_PROPAGATE;
+                break;

The break is never reached, so you could kill that before pushing

Attachment 281021 [details] pushed as 554001c - appDisplay: Move focus to popup only when necessary