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 621671 - add keyboard focus navigation support to StWidget/StContainer
add keyboard focus navigation support to StWidget/StContainer
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Owen Taylor
gnome-shell-maint
Depends on:
Blocks: 626660
 
 
Reported: 2010-06-15 16:56 UTC by Dan Winship
Modified: 2010-10-29 12:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[St] add keyboard focus navigation support (22.07 KB, patch)
2010-06-15 16:56 UTC, Dan Winship
none Details | Review
[dash] use the new StWidget keynav support for navigating search results (19.54 KB, patch)
2010-06-15 16:56 UTC, Dan Winship
needs-work Details | Review
[St] add keyboard focus navigation support (22.11 KB, patch)
2010-06-21 15:53 UTC, Dan Winship
reviewed Details | Review
[dash] Redo keynav, and make it work outside of search as well (21.11 KB, patch)
2010-06-21 15:53 UTC, Dan Winship
reviewed Details | Review
St: add keyboard focus navigation support (27.38 KB, patch)
2010-10-14 18:29 UTC, Dan Winship
reviewed Details | Review
St: add StFocusManager, to handle keyboard navigation (12.22 KB, patch)
2010-10-14 18:29 UTC, Dan Winship
accepted-commit_now Details | Review
PopupSwitchMenuItem: don't misuse this.active (1.09 KB, patch)
2010-10-14 18:29 UTC, Dan Winship
committed Details | Review
PopupMenu: redo keynav using St.FocusManager (18.59 KB, patch)
2010-10-14 18:29 UTC, Dan Winship
reviewed Details | Review
St: add keyboard focus navigation support (29.13 KB, patch)
2010-10-26 15:53 UTC, Dan Winship
reviewed Details | Review
St: add StFocusManager, to handle keyboard navigation (12.22 KB, patch)
2010-10-26 15:54 UTC, Dan Winship
committed Details | Review
PopupMenu: redo keynav using St.FocusManager (18.90 KB, patch)
2010-10-26 15:55 UTC, Dan Winship
committed Details | Review
St: add keyboard focus navigation support (29.05 KB, patch)
2010-10-27 21:26 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2010-06-15 16:56:52 UTC
This adds support for keyboard focus navigation to St, based somewhat
on Gtk's support, though not entirely.

StWidget gets a "can-focus" property, which actually means "can receive
the focus via keynav", but "can-focus-via-keyboard" or whatever is too
long.

st_widget_navigate_focus() and st_widget_navigate_focus_for_event()
are basically like gtk_widget_child_focus(); they let you move to the
first/next focusable widget within a container in a given direction.

st_container_get_focus_chain() is sort of like
gtk_container_get_focus_chain(), except that there's currently no _set_
method, and it's basically there to be overridden by subclasses like
StOverflowBox and ShellGenericContainer that don't always paint all of
their children.


The second patch is a work-in-progress of porting dash search result
navigation to use the new code. It has several regressions (notably that
you lose focus in the entry after typing the first character, and you
can't use up/down to navigate through the app search results, since 
they're not arranged vertically). Some of that I'm still fixing and some
of it will need redesign, but you can at least use it to test the other
code.

I don't expect to commit the widget code until I have the dash code
fixed, but I wanted to put it up now to let people see it.
Comment 1 Dan Winship 2010-06-15 16:56:54 UTC
Created attachment 163697 [details] [review]
[St] add keyboard focus navigation support

Add StWidget:can-focus, st_widget_navigate_focus{,for_event}(), and
st_container_get_focus_chain(), and implement as needed to allow
keyboard navigation of widgets.
Comment 2 Dan Winship 2010-06-15 16:56:56 UTC
Created attachment 163698 [details] [review]
[dash] use the new StWidget keynav support for navigating search results
Comment 3 Owen Taylor 2010-06-15 19:28:32 UTC
(Taking for review since I wrote or rewrote most of the GTK+ focus code at one point)
Comment 4 Dan Winship 2010-06-21 15:53:21 UTC
Created attachment 164225 [details] [review]
[St] add keyboard focus navigation support

fix a few bugs
Comment 5 Dan Winship 2010-06-21 15:53:37 UTC
Created attachment 164226 [details] [review]
[dash] Redo keynav, and make it work outside of search as well

This makes keyboard navigation work more like it does in gtk; you
can't use Up and Down to navigate among apps in the search results any
more, because they're not stacked vertically. Also, Tab/Shift-Tab wrap
around, but arrow keys do not.
Comment 6 Dan Winship 2010-06-28 13:08:57 UTC
Owen pointed out MxFocusable/MxFocusManager, with a possible eye towards not diverging too gratuitously.


The distinction between adding focus-related methods to StWidget (and overriding them in some subclasses) vs having a separate focusable interface (and reimplementing it in some subclasses) is trivial. Mx also has "MxStylable" rather than having CSS stuff directly in MxWidget, so having it be in an interface is "more Mx-like" I guess, while having it directly in StWidget is "more St-like".


MxFocusable has a pair of virtual methods: mx_focusable_accept_focus(), and mx_focusable_move_focus(). St uses only a single method, st_widget_navigate_focus(), which combines the functionality of both MxFocusable methods. (If you call st_widget_navigate_focus(widget, direction) when the current focus is outside widget, it means "have widget (or one of its descendents) accept the focus, coming from direction". If you call it when the current focus is inside widget, it means "move the focus to the next focusable actor within widget in direction".)

The bigger difference between the two methods (and probably part of the reason why Mx needs 2 methods and St doesn't) is that mx_focusable_move_focus() recurses upward through the hierarchy if there are no more focusable actors inside widget, whereas st_widget_navigate_focus just fails in that case. This means that every focusable widget in the entire stage is always part of the focus chain in Mx, whereas in St, there's always an explicit... uh... "focus group". In Mx, you do "mx_focusable_move_focus(currently_focused_widget, direction)", and the focus can end up anywhere, while in St you do "st_widget_navigate_focus(focus_container, direction)", and the focus can only end up inside focus_container.

The not-recursing-upwards behavior is important for us because we don't the user menu to get the focus if you Shift-Tab from the dash search entry, etc.


My patch doesn't currently have anything like MxFocusManager, but that's just because I didn't need it yet. We may eventually want that. Perhaps that means that st_widget_navigate_focus_for_event() shouldn't really be an StWidget method? (The corresponding switch statement is in MxFocusManager in Mx.)


Mx doesn't have a can-focus property like StWidget. Rather, it seems to rely more on having some types of widget always be focusable (entries, buttons), and other types never focusable. Mx also doesn't have the automatic "focus" pseudo-class handling. Both of those could be added.


Mx doesn't have an equivalent of StContainer, so as a result there is a lot more duplicated code; each container type has to implement focus navigation itself, whereas in St we just have a single implementation in StContainer, plus st_container_get_focus_chain() to allow subclasses to override it a little bit without needing to reimplement the complicated bits.


Overall: St can't use Mx's system unless it was changed to allow for "focus group"s. Maybe there could be multiple MxFocusManagers, and a manager would only allow focus to be moved within its container; having a single toplevel MxFocusManager would simulate the current Mx behavior.

Going the other way, Mx could mostly use St's system, because "st_widget_navigate_focus(stage, direction)" would behave like Mx's keynav. Except for the fact that the stage isn't an StWidget. But there could be a static "st_widget_naviagate_focus_in_stage(direction)" or something.
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-06-28 15:41:44 UTC
Sorry, I have the intention to review this in my TODO list, but I forget that until today. After a quick review of the patch and your comments. Some comments, questions:

(In reply to comment #0)
> This adds support for keyboard focus navigation to St, based somewhat
> on Gtk's support, though not entirely.
> 
> StWidget gets a "can-focus" property, which actually means "can receive
> the focus via keynav", but "can-focus-via-keyboard" or whatever is too
> long.

Cool, this would improve setting by default any cally-actor the state ATK_STATE_FOCUSABLE (I need to change that)

And now the questions. As far as I see you have added that property because clutter lacks of that, but apart of that, you have used the different key focus functions (clutter_actor_grab_key_focus, clutter_stage_get_key_stage, etc) and signals (ClutterActor::key-focus-in, ClutterActor::key-focus-out). I'm wrong with that supposition ?

The current focus management/detection in cally is based on these signals, so it wouldn't required too many changes if it is the case (anyway, there are already virtual funcs defined on cally-actor if it is required to redefine it).

> st_container_get_focus_chain() is sort of like
> gtk_container_get_focus_chain(), except that there's currently no _set_
> method, and it's basically there to be overridden by subclasses like
> StOverflowBox and ShellGenericContainer that don't always paint all of
> their children.

So the focus chain is computed anytime is required?

Although it is not defined yet, there are any plans to add a _set_focus_chain in the future?

(In reply to comment #6)
> Owen pointed out MxFocusable/MxFocusManager, with a possible eye towards not
> diverging too gratuitously.

I have also reviewed this interfaces some months ago, but not really deeply, as it wouldn't require extra cally support, and were not introduced in St. In general my impressions were (I was wise enough to write down a summary):

    * The manager is created on the mx-window, but you can call it by hand, IMHO, it would be better to have one per-stage (in fact, it was solved [1])
    * It only manages the mx objects implementing mx-focusable, being mx-focusable a way to filter not significant widgets.
    * It doesn't support specify a custom focus chain
       * get-focus-chain
       * set-focus-chain
       * unset-focus-chain 
    * It just manage the TAB key, one direction
    * It require to include the arrows, or a way to configure the keys.
    * Works on basic examples (made by me) but not really fine on the current mx examples.

(Note that this were some months ago, probably some things has changed since then).

> The bigger difference between the two methods (and probably part of the reason
> why Mx needs 2 methods and St doesn't) is that mx_focusable_move_focus()
> recurses upward through the hierarchy if there are no more focusable actors
> inside widget, whereas st_widget_navigate_focus just fails in that case. This
> means that every focusable widget in the entire stage is always part of the
> focus chain in Mx, whereas in St, there's always an explicit... uh... "focus
> group". In Mx, you do "mx_focusable_move_focus(currently_focused_widget,

when you mean "focus group", what its the difference with any focus chain? (As in gtk when you don't use _set_focus_chain.)

> Mx doesn't have a can-focus property like StWidget. Rather, it seems to rely
> more on having some types of widget always be focusable (entries, buttons), and
> other types never focusable. Mx also doesn't have the automatic "focus"
> pseudo-class handling. Both of those could be added.

As far as I see, in practice, any widget implement MxFocusable is intended to be able to receive focus (so if you implement MxFocusable, implicitly means that can-focus=true)

> Going the other way, Mx could mostly use St's system, because
> "st_widget_navigate_focus(stage, direction)" would behave like Mx's keynav.
> Except for the fact that the stage isn't an StWidget. But there could be a
> static "st_widget_naviagate_focus_in_stage(direction)" or something.

This "stage isn't an StWidget" worries me as this means that it would have a different focus management. In my first impressions it seemed that the stage received the key focus, at least in the "initial gnome shell view". This would mean that at that moment is the ClutterStage the one with the key focus.

Anyway, recently this seemed to change. After digging a little on this bug [2] it seems that in that view no clutter object has the key focus, or at least it has been deactivated (any suggestion is welcome)

[1] http://bugzilla.moblin.org/show_bug.cgi?id=9639
[2] http://bugzilla.clutter-project.org/show_bug.cgi?id=2147
Comment 8 Dan Winship 2010-06-28 16:00:53 UTC
(In reply to comment #7)
> And now the questions. As far as I see you have added that property because
> clutter lacks of that, but apart of that, you have used the different key focus
> functions (clutter_actor_grab_key_focus, clutter_stage_get_key_stage, etc) and
> signals (ClutterActor::key-focus-in, ClutterActor::key-focus-out). I'm wrong
> with that supposition ?

All focus grabbing/querying/notification is done via existing clutter methods. The new stuff is all just for deciding which widget should receive focus next when doing keyboard navigation (and automatically adjusting CSS as the focus moves).

> Although it is not defined yet, there are any plans to add a _set_focus_chain
> in the future?

If we need it somewhere, yes.

> when you mean "focus group", what its the difference with any focus chain? (As
> in gtk when you don't use _set_focus_chain.)

"focus chain" generally just refers to the *order* that widgets are focused in.

The problem I was talking about with "focus groups" is that in Mx, it is always possible to get from one focusable widget to *any* other focusable widget on the stage by pressing Tab some number of times. This works fine as long as the entire stage is thought of as one "window", but it doesn't work for the shell UI; you don't want to be able to tab from the search results into the panel menu (just like you don't want to be able to tab from your web browser into the panel).

> As far as I see, in practice, any widget implement MxFocusable is intended to
> be able to receive focus (so if you implement MxFocusable, implicitly means
> that can-focus=true)

Right, but that means that every MxEntry is focusable. In St's system, I can have one StEntry be focusable, and another one not be. (Maybe there's not a lot of reason you'd want to do that... Gtk has the same feature though, and I'm pretty sure it gets used there.)

> This "stage isn't an StWidget" worries me as this means that it would have a
> different focus management.

It's entirely an implementation detail.
Comment 9 Owen Taylor 2010-09-23 21:27:47 UTC
Review of attachment 164225 [details] [review]:

General thoughts:

 * This feels like half an API - focus motion but you have to write the glue code  yourself. It appeals to me to have:

    st_focus_manager_get_for_stage ();      // default hook up for tostage
    st_focus_manager_new (toplevel_widget); // what we use for the multiple toplevels, feed it events manually

 * Directional focus motion with arrow keys never turned out to be as useful in GTK+ as originally envisioned. The problem is trapping - arrow into a combobox, next arrow pops down the combobox. But that being said, directional arrow key navigation was repurposed/hacked in GTK+ to implement things like arrow keys to move through radio buttons, and the end result seems to be good ... key navigation has worked well in GTK+ since the work was done on it in the GTK+ 2.0 era. (It's possible that the default implementation for arrow keys should be different from tab keys and not allow going off the end of a container and to the outer enclosing container. That's something that could be played with.)

::: src/st/st-container.c
@@ +355,3 @@
 
+/* filter @children to contain only only actors that overlap @ref
+ * in @direction

Not sure "overlap in UP" makes sense "overlap when moving in @direction"

@@ +371,3 @@
+  for (l = children, ret = NULL; l; l = l->next)
+    {
+      clutter_actor_get_position (l->data, &cx, &cy);

Ny general rule is to add a temporary variable if I use l->data more than once, to give the compiler more of a chance to catch my mistakes and to improve legibility.

@@ +379,3 @@
+        case GTK_DIR_DOWN:
+          if ((cx >= rx && cx < rx + rwidth) ||
+              (cx + cwidth > rx && cx + cwidth <= rx + rwidth))

This check is wrong.

 cx                             cx + cwidth
        rx     rx + width

 rx + rwidth > cx && rx < cx + cwidth

I think is right off the top of my head. (might want to check a few cases by drawing them out.)

@@ +410,3 @@
+
+  clutter_actor_get_position (actor_a, &ax, &ay);
+  clutter_actor_get_position (actor_b, &bx, &by);

GTK+ sorts the center. Not sure how much it matters in practice - would matter for something like hitting Up for:

          
 +------+ +------+
 |      | |      |
 |  A   | |  B   |
 |      | |      |
 +------+ |      |
          +------+
    +---------+
    |    C    |
    +---------+

with the focus on C

@@ +481,3 @@
+  if (focus_child)
+    {
+      /* Skip past focus_child */

You don't want to skip past focus_child, you want to skip to focus child and give it the chance to move focus internally and return TRUE, right?

::: src/st/st-overflow-box.c
@@ +315,3 @@
 
+static ClutterActor *
+visible_children_iter (StOverflowBox *box, GList **iter, int *n)

parameter line breaks; I don't care too much about internal iterator APIs, but this of the caller having to initialize seems messy. Wouldn't be more than a few more lines more to have:

 visible_child_iter_init (box, *&iter);
 while ((child = visible_child_iter_next (&iter)) 
   {
   }

or something like that.

::: src/st/st-widget.c
@@ +1704,3 @@
+                               GtkDirectionType  direction)
+{
+  if (widget->priv->can_focus)

Doesn't this need to check if the widget is already focused and return FALSE?

@@ +1725,3 @@
+ * can-focus property set. If the current focus actor is outside of
+ * @widget, this attempts to focus either @widget itself, or its first
+ * descendant in the order implied by @direction.

This is the canonical idea of gtk_widget_focus(), but look at gtk_radio_button_focus() for something where it is doing something much looser - gtk_radio_button_focus(radio_button, ...) can result in a widget that is *not* a child of the radio button getting focused.

Might make sense to plan ahead and make the contract "returns TRUE if the widget handles the focus motion and results in a new widget being focused, otherwise returns FALSE"

@@ +1776,3 @@
+      break;
+    case CLUTTER_Tab:
+      if (event->key.modifier_state & CLUTTER_SHIFT_MASK)

Unless something has changed in Clutter recently you probably need to handle both CLUTTER_KEY_Tab + SHIFT_MASK and CLUTTER_KEY_ISO_Left_Tab. (GTK+ canonicalizes to ISO_Left_Tab)
Comment 10 Owen Taylor 2010-09-23 21:39:27 UTC
Review of attachment 164226 [details] [review]:

Looks promising - don't have a lot of specific comments. Marking reviewed while waiting for a new revision based on comments on St changes, and other work Dan has outstanding.
Comment 11 Dan Winship 2010-10-14 18:24:45 UTC
(In reply to comment #9)
>  * This feels like half an API - focus motion but you have to write the glue
> code  yourself. It appeals to me to have:
> 
>     st_focus_manager_get_for_stage ();      // default hook up for tostage
>     st_focus_manager_new (toplevel_widget); // what we use for the multiple
> toplevels, feed it events manually

OK, the new patches have an explicit StFocusManager; you have a single focusmanager for the stage, and add "groups" to it to indicate focusable zones. A separate abstraction at the JS level lets you create focus groups corresponding to Ctrl-Alt-Tab-able zones (bug 618885).

> +        case GTK_DIR_DOWN:
> +          if ((cx >= rx && cx < rx + rwidth) ||
> +              (cx + cwidth > rx && cx + cwidth <= rx + rwidth))
> 
> This check is wrong.

The filtering/sorting should be identical to gtkcontainer now.

> GTK+ sorts the center. Not sure how much it matters in practice

well, in the current dash, you have

  [ Search          ]

  AppA AppB AppC AppD
  AppE AppF AppG AppH

and now moving down from the Search entry moves to AppB instead of AppA...

but of course, that's all going away anyway...

> +      /* Skip past focus_child */
> 
> You don't want to skip past focus_child, you want to skip to focus child and
> give it the chance to move focus internally and return TRUE, right?

No, we already gave focus_child a chance to move the focus internally before constructing the focus chain, and it returned FALSE.
Comment 12 Dan Winship 2010-10-14 18:28:16 UTC
Comment on attachment 164226 [details] [review]
[dash] Redo keynav, and make it work outside of search as well

I didn't bother updating this since the dash is going away anyway
Comment 13 Dan Winship 2010-10-14 18:29:17 UTC
Created attachment 172375 [details] [review]
St: add keyboard focus navigation support

Add StWidget:can-focus, st_widget_navigate_focus(), and
st_container_get_focus_chain(), and implement as needed to allow
keyboard navigation of widgets.
Comment 14 Dan Winship 2010-10-14 18:29:27 UTC
Created attachment 172376 [details] [review]
St: add StFocusManager, to handle keyboard navigation

StFocusManager allows setting up "focus groups" in which it will
automatically handle navigation between can_focus widgets.
Comment 15 Dan Winship 2010-10-14 18:29:34 UTC
Created attachment 172377 [details] [review]
PopupSwitchMenuItem: don't misuse this.active

The constructor was setting this.active to reflect the switch state,
which is wrong; this.active indicates whether or not the item is
highlighted.
Comment 16 Dan Winship 2010-10-14 18:29:47 UTC
Created attachment 172378 [details] [review]
PopupMenu: redo keynav using St.FocusManager

Each menu is a focus manager group, but there is also some explicit
focus handling between non-hierarchically-related widgets. Eg, to move
between menus, or from a menubutton into its menu.
Comment 17 Owen Taylor 2010-10-19 20:27:52 UTC
Review of attachment 172375 [details] [review]:

Looks good to me except for some comments about missing comments. And one bit of the logic I don't understand.

::: src/st/st-bin.c
@@ +234,3 @@
+  StBinPrivate *priv = ST_BIN (widget)->priv;
+
+  if (from == CLUTTER_ACTOR (widget) || from == priv->child)

Don't understand the priv->child check - how is focus on the child different from focus on a different descendant? 

Also consider something like GtkNotebook or a GtkTextView with embedded widgets that can have focus chain postions on the widget itself *and* on children. Don't you have to always delegate to the child's navigate_focus unless from is the bin itself?

::: src/st/st-container.c
@@ +203,3 @@
+ *
+ * Gets a list of the focusable children of @container, in "Tab"
+ * order. By default, this returns all mapped children of @container.

hmm, actually it returns all visible children of @container whether or not they are mapped. (Might be better to say "all children set to be visible" to avoid confusion about whether they actually *are* visible to the user)

@@ +618,3 @@
+      if (from)
+        {
+          if (from == focus_child)

Maybe an /* optimize the simple case */ comment

@@ +621,3 @@
+            clutter_actor_get_allocation_box (focus_child, &sort_data.box);
+          else
+            {

Maybe a comment that you are assuming no rotations or scales?

::: src/st/st-entry.c
@@ +228,3 @@
+
+  if (from == priv->entry)
+    return FALSE;

This needs a comment, it's not clear to me what it is doing and why the override was needed.

::: src/st/st-overflow-box.c
@@ +317,3 @@
+visible_children_iter_init (StOverflowBox  *box,
+                            GList         **iter,
+                            int            *n)

I meant with a struct, but sure :-), as I said, internal iters don't need to be stylish

@@ +326,3 @@
+visible_children_iter (StOverflowBox  *box,
+                       GList         **iter,
+                       int            *n)

Again, style points in a place where style doesn't matter, but you use i for the iteration variable externally and n here.
Comment 18 Owen Taylor 2010-10-19 21:14:47 UTC
Review of attachment 172376 [details] [review]:

Looks good. One tiny consistency issue with existing usage of setting-global-object-on-stage-user-data

::: src/st/st-focus-manager.c
@@ +146,3 @@
+  StFocusManager *manager;
+
+  manager = g_object_get_data (G_OBJECT (stage), "StFocusManager");

src/st/st-theme-context.c:  g_object_set_data (G_OBJECT (stage), "st-theme-context", context);
Comment 19 Owen Taylor 2010-10-19 21:17:34 UTC
Review of attachment 172377 [details] [review]:

Looks fine, though there seems to be some overusage of 'active' to mean different things. (I guess it's gtk_toggle_button_get_active() vs. CSS's :active pseudoclass, so not really *our* inconsistency.)
Comment 20 Owen Taylor 2010-10-20 14:50:11 UTC
Review of attachment 172378 [details] [review]:

Noticed in testing:

 - Menus popped up with right click from the app well are key navigable once you hover over an item, but not before
 - Unlike GTK+, menus don't wrap from top to bottom
 - Unlike GTK+, if you have a toggle menu item, and hit space on it, the menu doesn't stay up (GTK+ - space toggle, return toggle and close menu)

A few comments looking at the code. I won't claim to have understood every in and out, but it  looks reasonable and a not more complex than what was there before. Menus are just messy - there's no way around it.

::: js/ui/popupMenu.js
@@ +491,3 @@
+            this._isSubMenu = true;
+            this.actor.reactive = true;
+            this.actor.connect('key-press-event', Lang.bind(this, this._onKeyPressEvent));

You need a comment on the handler that it's only connected for submenus, or it's hard to understand and a booby trap for future modification

@@ +701,3 @@
+        if (this.isOpen &&
+            this._activeMenuItem &&
+            event.get_key_symbol() == Clutter.KEY_Left) {

Could use a comment. I have no real idea what this is for reading the code - my best guess is leaving a submenu and going to the parent menu item?

@@ +827,2 @@
         this._eventCaptureId = 0;
+        this._eventId = 0;

Was this meant to be keyPressEventId?
Comment 21 Dan Winship 2010-10-20 18:34:54 UTC
(In reply to comment #17)
> +  if (from == CLUTTER_ACTOR (widget) || from == priv->child)
> 
> Don't understand the priv->child check - how is focus on the child different
> from focus on a different descendant? 
> 
> Also consider something like GtkNotebook or a GtkTextView with embedded widgets
> that can have focus chain postions on the widget itself *and* on children.
> Don't you have to always delegate to the child's navigate_focus unless from is
> the bin itself?

It's implicit in the current code that an actor can either be focusable, or have focusable children, but not both. (And so if priv->child is focused, then calling navigate_focus on it must fail.) I guess that needs to be more explicit.

> + * order. By default, this returns all mapped children of @container.
> 
> hmm, actually it returns all visible children of @container whether or not they
> are mapped.

That's a bug. It shouldn't return unmapped children, since we don't want the focus landing on something invisible.

> +            clutter_actor_get_allocation_box (focus_child, &sort_data.box);
> 
> Maybe a comment that you are assuming no rotations or scales?

Hm... I can't keep track of allocation works under transformations... what is the exact effect here, if the container is rotated (but its children have no rotation relative to the container), or if a child is rotated? I'm guessing in the former case, the arrow keys would just behave as though the container was not rotated. In the latter case... the allocation box would not correspond to the container's coordinate system, right? So we'd lose horribly?

I was using get_allocation_box() here to force a layout if necessary, but I guess we could call get_allocation_box() and then use get_allocation_vertices() or something if that would give more sane results?

> +  if (from == priv->entry)
> +    return FALSE;
> 
> This needs a comment, it's not clear to me what it is doing and why the
> override was needed.

It's because the StEntry is marked can_focus, but it's the internal ClutterText that actually has the clutter keyboard focus. This confuses the default navigate_focus() implementation, and when you try to Tab out of the focused ClutterText, it will end up deciding that the correct thing to do is to focus the StEntry. (And then st_entry_key_focus_in() just immediately reassigns focus to the ClutterText.) So it's impossible to keynav out.

(In reply to comment #20)
>  - Menus popped up with right click from the app well are key navigable once
> you hover over an item, but not before

oh, I didn't test in the overview at all with this round of patches

>  - Unlike GTK+, menus don't wrap from top to bottom

Yeah. Is gtk's behavior a feature? (Other than for getting from "Afghanistan" to "United States" faster?) Normal window behavior seems to be that Tab/Shift-Tab wrap around, but arrow keys do not. This is easy enough to change though since we override a bunch of other menu keynav behavior.

>  - Unlike GTK+, if you have a toggle menu item, and hit space on it, the menu
> doesn't stay up (GTK+ - space toggle, return toggle and close menu)

OK... so we want that?
Comment 22 Owen Taylor 2010-10-20 20:21:41 UTC
(In reply to comment #21)
> (In reply to comment #17)
> > +  if (from == CLUTTER_ACTOR (widget) || from == priv->child)
> > 
> > Don't understand the priv->child check - how is focus on the child different
> > from focus on a different descendant? 
> > 
> > Also consider something like GtkNotebook or a GtkTextView with embedded widgets
> > that can have focus chain postions on the widget itself *and* on children.
> > Don't you have to always delegate to the child's navigate_focus unless from is
> > the bin itself?
> 
> It's implicit in the current code that an actor can either be focusable, or
> have focusable children, but not both. 

I don't think this is a correct assumption for toolkits in general, but taking it as such...

> (And so if priv->child is focused, then calling navigate_focus on it must fail.) 

So the GtkBin code is just an optimization? Is it a worthwhile optimization?

> > +            clutter_actor_get_allocation_box (focus_child, &sort_data.box);
> > 
> > Maybe a comment that you are assuming no rotations or scales?
> 
> Hm... I can't keep track of allocation works under transformations... what is
> the exact effect here, if the container is rotated (but its children have no
> rotation relative to the container), or if a child is rotated? I'm guessing in
> the former case, the arrow keys would just behave as though the container was
> not rotated. In the latter case... the allocation box would not correspond to
> the container's coordinate system, right? So we'd lose horribly?

My understanding of the exact effect is that we first translate the origin to the origin of the allocation, and then perform the transformation, so if I have an actor that has an an actor with allocation +10+10x10x10 and x_scale and y_scale are 2, then it occupies the box+10+10x20x20 in parent coordinates.

So the particular issues are that the bounding box of the child is different from its allocation if its transformed, and that the logic that you have to compute the position of the focus child descendant using clutter_actor_get_transformed_position() doesn't work if any widgets in the heirarchy (including parent widgets!) have transforms or scales.

It's certainly possible to compute the allocation vertices of the current focus descendant and of the children in terms of the coordinate system of the container (or in hte coordinate system of the toplevel if you want the arrow keys to be more "meaningful" for rotated actors), but I don't have any problem with just ignoring the problem ... just a request to document what we are ignoring.

> I was using get_allocation_box() here to force a layout if necessary, but I
> guess we could call get_allocation_box() and then use get_allocation_vertices() or something if that would give more sane results?
> 
> > +  if (from == priv->entry)
> > +    return FALSE;
> > 
> > This needs a comment, it's not clear to me what it is doing and why the
> > override was needed.
> 
> It's because the StEntry is marked can_focus, but it's the internal ClutterText
> that actually has the clutter keyboard focus. This confuses the default
> navigate_focus() implementation, and when you try to Tab out of the focused
> ClutterText, it will end up deciding that the correct thing to do is to focus
> the StEntry. (And then st_entry_key_focus_in() just immediately reassigns focus
> to the ClutterText.) So it's impossible to keynav out.

Sounds like good comment material. Though maybe it would be cleaner to just implement the navigate_focus method from scratch without chaining up to a confused parent class implementation?
 
> >  - Unlike GTK+, menus don't wrap from top to bottom
> 
> Yeah. Is gtk's behavior a feature? (Other than for getting from "Afghanistan"
> to "United States" faster?) Normal window behavior seems to be that
> Tab/Shift-Tab wrap around, but arrow keys do not. This is easy enough to change
> though since we override a bunch of other menu keynav behavior.

Hard to say if it's a feature or not. I tried to match what other toolkits (Windows, Qt) did in 1998 or so when I probably added this to GtkMenu.
 
> >  - Unlike GTK+, if you have a toggle menu item, and hit space on it, the menu
> > doesn't stay up (GTK+ - space toggle, return toggle and close menu)
> 
> OK... so we want that?

I noted it because it surprised me and struck me as broken. I may be the only person who knows about the GTK+ feature and has such expectations, however.
Comment 23 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-10-21 14:59:00 UTC
I have been testing the patches a little, and I have some question from the user POV:

a) I noticed that when you open the overview, you can't navigate on the dash items (applications, places, etc) until you start a search.

Is a) the desired behaviour? If not, I guess that it would be solved on bug 618887 (specifically bug 587071).

b) Related to the previous, it is not possible to open the All applications menu, or navigate inside it.

I guess that this is a real issue to solve on the other bugs.

PD: From the developer POV, last patch (attachment 172378 [details] [review]) fails if you apply it using git am against the master.
Comment 24 Dan Winship 2010-10-21 16:36:39 UTC
the latest patch doesn't make any attempt to improve overview navigability, since the overview is being rewritten (see the overview-relayout branch in git). Eventually, yes, it will be navigable
Comment 25 Alejandro Piñeiro Iglesias (IRC: infapi00) 2010-10-21 19:16:57 UTC
Ok, first sorry for the noise of my previous comment. I supposed that the current patches on this bug were required to get the basic keyboard navigation that I checked on comment 23.

The main reason of this error was that applying the old patches were required in order to get orca speaking, as we made on GUADEC. But now, doing as on GUADEC, applying this patches and the a11y one (612599 626658), orca doesn't speech out anything (although still works if I just rebase to the master using the old patches).

In fact, applying this patches is not enough to test it. As far as I see st_widget_navigate_focus is not called (a good moment to ask if this is normal).

(BTW: with the old patches, I have keynav without requiring the search)

What I appreciated on comment 23 was just the current ad-hoc item list navigation that you can find on dash.js (master) or on viewSelector.js (overview-relayout). 

The advantage that I see to this approach is that the key focus is still on the entry, in order to keep writing the search string, while the user can use the arrows to select an item. With the old patches, the entry lose the focus if you try to navigate.

But from the a11y point of view it is not accessible, as this ad-hoc lacks any signal saying that the "focus" (in this case visible focus, not key focus), has changed from a item to other. 

So, now, in relation with this bug:

  * Right now the dash-viewSelecter has his own keynav
  * You are working on a general focus navigation implementation

When this bug is finished, I guess that the ad-hoc keynav will be removed and it will be used use this general one. Right?

The only reason to not do that is keep the feature to maintain the key focus on the entry, while allowing the user to move on the app-list. But in this case, it would be required a way to get the item focused. And not sure how to manage two elements focused at the same time.
Comment 26 Dan Winship 2010-10-21 19:59:47 UTC
(In reply to comment #25)
> Ok, first sorry for the noise of my previous comment. I supposed that the
> current patches on this bug were required to get the basic keyboard navigation
> that I checked on comment 23.

No, that functionality no longer exists anywhere.

> In fact, applying this patches is not enough to test it.

Sorry, there's not really any expectation that these patches are immediately useful to users. This is just low-level API that's setting the groundwork for later features.

> When this bug is finished, I guess that the ad-hoc keynav will be removed and
> it will be used use this general one. Right?

Well, that will be bug 618887, but yes. (If we really feel the type-and-navigate-at-the-same-time behavior is useful for some reason, we could just make it so that if you type an alphanumeric key when the entry doesn't have the focus, then it gets refocused.)
Comment 27 Dan Winship 2010-10-26 15:53:55 UTC
Created attachment 173273 [details] [review]
St: add keyboard focus navigation support

Clarify the semantics of can-focus containers, both in the
    st_widget_navigate_focus() docs and in the implementations. StBin and
    StContainer both require that either the container can be focusable,
    or its children can be focused, but not both (but subclasses could
    override that).
    
    Clarify that st_container_get_focus_chain() returns
    CLUTTER_ACTOR_IS_VISIBLE() children. (Previously it said "mapped",
    which is the same thing unless you're calling get_focus_chain() on an
    unmapped container, which would be weird, but...)
    
    Add a few comments about not dealing with transformations.
Comment 28 Dan Winship 2010-10-26 15:54:22 UTC
Created attachment 173274 [details] [review]
St: add StFocusManager, to handle keyboard navigation

change g_object_get/set_data usage
Comment 29 Dan Winship 2010-10-26 15:55:08 UTC
Created attachment 173275 [details] [review]
PopupMenu: redo keynav using St.FocusManager

add comments to submenu keypresshandler, fix a variable name
Comment 30 Owen Taylor 2010-10-27 19:46:04 UTC
Review of attachment 173273 [details] [review]:

::: src/st/st-bin.c
@@ +235,3 @@
+  ClutterActor *bin_actor = CLUTTER_ACTOR (widget);
+
+  if (from == bin_actor || from == priv->child)

Still don't really understand the priv->child check - it seems to contradict your comment in bugzilla that "subclasses could override" - why not just give the child itself the chance to decide what it wants to do? If it's the case where the child is a ClutterActor not a StWidget, then I'd add that in an else to ST_IS_WIDGET (priv->child)

::: src/st/st-widget.c
@@ +1754,3 @@
+ *
+ * If a container type is marked #StWidget:can-focus, the expected
+ * behavior is that it will only take up a single slot on the focus

If StWidget classes aren't required to have this behavior - if it's about the StBin/StContainer impemenetation then why is it documented here?
Comment 31 Owen Taylor 2010-10-27 19:49:14 UTC
Review of attachment 173274 [details] [review]:

Looks good
Comment 32 Owen Taylor 2010-10-27 20:15:47 UTC
Review of attachment 173275 [details] [review]:

Marking a-c-n. Haven't retested for behavior in the overview, popup menus ,but I'll assume that if it's not fixed here we an fix later
Comment 33 Dan Winship 2010-10-27 20:42:56 UTC
(In reply to comment #30)
> Still don't really understand the priv->child check

um... that would be because it's wrong :)

> If StWidget classes aren't required to have this behavior - if it's about the
> StBin/StContainer impemenetation then why is it documented here?

It's not just about StBin/StContainer. Classes aren't *required* to have that behavior, but they're *assumed* to have it. It's the default behavior for StWidgets. So containers that behave that way don't need to document that fact, but containers that do something different do.
Comment 34 Dan Winship 2010-10-27 21:26:33 UTC
Created attachment 173357 [details] [review]
St: add keyboard focus navigation support

fix st_bin_navigate_focus
Comment 35 Owen Taylor 2010-10-28 18:37:16 UTC
(In reply to comment #33)
> If StWidget classes aren't required to have this behavior - if it's about the
> > StBin/StContainer impemenetation then why is it documented here?
> 
> It's not just about StBin/StContainer. Classes aren't *required* to have that
> behavior, but they're *assumed* to have it. It's the default behavior for
> StWidgets. So containers that behave that way don't need to document that fact,
> but containers that do something different do.

OK. I guess "assumed" to me implied that there are assumptions in the code that break that if they don't - not that something is just the normal case. But English isn't a programming language... everybody can have their own interpretation. :-)
Comment 36 Owen Taylor 2010-10-28 18:39:36 UTC
Review of attachment 173357 [details] [review]:

Looks good
Comment 37 Dan Winship 2010-10-29 12:39:45 UTC
Attachment 173274 [details] pushed as 5a83ef8 - St: add StFocusManager, to handle keyboard navigation
Attachment 173275 [details] pushed as 548a23a - PopupMenu: redo keynav using St.FocusManager
Attachment 173357 [details] pushed as d2b968a - St: add keyboard focus navigation support