Did you open the tray with the overview, hot corner, or Super+M? This is a really important detail.

(In reply to comment #1)
> Did you open the tray with the overview, hot corner, or Super+M? This is a
> really important detail.

You're right, and I mischaracterised this bug. The previous selection has nothing to do with it - the first tray item (counting from the left) always has the selection highlight if you open the tray using super+M.

I'm renaming the bug accordingly.

By the way, it was intentional that we selected the leftmost item when coming from Super+M. What behavior for keyboard navigation would you like?

But keynav seems broken currently.
Regardless how I open the tray, I can't move the focus around with any keys.

any news about this ? I still consider basic keynav to be a high-priority thing we should get working for 3.6...

Give me a set of behaviors for keynav, and I will implement them.

in the tray:
left arrow moves focus left,
right arrow moves focus right,
space opens notification
up arrow does the same

in an open notification:
arrows move focus between actions / entries
enter triggers the focused action
down arrow closes it
delete triggers the close button

uncertain:
what should escape should do
is there a default action ?

The keynav matthias outlines sounds reasonable. I would perhaps not focus an item even when message tray is brought up with <super>M. Only after pressing tab, or arrow keys, the last item would focus.

Esc should dismiss the message tray, just like <Super>M should (should be a toggle). In case a bubble is open, Esc dismisses the bubble only, an additional Esc is needed to dismiss the message tray.

(In reply to comment #7)
> in the tray:
> left arrow moves focus left,
> right arrow moves focus right,
> space opens notification
> up arrow does the same
> 
> in an open notification:
> arrows move focus between actions / entries
> enter triggers the focused action
> down arrow closes it

Up/Down arrows are used for history in telepathy chat, like readline.

> delete triggers the close button
> 
> uncertain:
> what should escape should do
> is there a default action ?

(In reply to comment #8)
> The keynav matthias outlines sounds reasonable. I would perhaps not focus an
> item even when message tray is brought up with <super>M. Only after pressing
> tab, or arrow keys, the last item would focus.
> 
> Esc should dismiss the message tray, just like <Super>M should (should be a
> toggle). In case a bubble is open, Esc dismisses the bubble only, an additional
> Esc is needed to dismiss the message tray.

Should clicking outside the bubble only dismiss the bubble, or dismiss the entire tray? Right now we have "escape" and "click outside" tied to the same action, so it would require a bit of a code rework to fix that one.

(And should the same be true for popup menus as well? Right now, clicking outside of the combo box in the user menu will only close the combobox)

Created attachment 224250 [details] [review]
messageTray: Enable basic key navigation in the summary

Instead of getting a key grab on the summary directly, try to navigate
focus first in order to make use of StWidget's builtin keynav.

(In reply to comment #7)
> in the tray:
> left arrow moves focus left,
> right arrow moves focus right,
> space opens notification
> up arrow does the same

Attached patch takes care of the above with the exception of the the up arrow.

Review of attachment 224250 [details] [review]:

The grab here isn't supposed to actually grab anything, it's simply supposed to set hadFocus so that the GrabHelper will navigate for us. Can you try and figure out why GrabHelper isn't navigating, instead?

Oh, an interesting observation: keynav works as expected until I enter the overview for the first time ...

(I'm commuting now, so can't dig into it immediately)

(In reply to comment #13)
> Oh, an interesting observation: keynav works as expected until I enter the
> overview for the first time ...
> 
> (I'm commuting now, so can't dig into it immediately)

Sounds like it might be fixed by

http://git.gnome.org/browse/clutter/commit/?id=3398f3acdfcbb727db12abac30b4f09629edef1d

Nope, my Clutter checkout is more recent.

Created attachment 224341 [details] [review]
messageTray: Fix navigation between summary items

While the overview is visible, the tray is added to the ctrl-alt-tab
popup, and removed when the overview is hidden - as the latter also
removes the tray from the focus_manager, keynav between summary items
stops working after showing the overview for the first time unless we
re-add the tray to the focus_manager.

(I'm attaching two different fixes, this one is the safe+unintrusive one)

Created attachment 224342 [details] [review]
focus-manager: Make groups "refcounted"

Rather than unconditionally removing a focus root in remove_group(),
decrement a counter that add_group() increments, and only actually
remove a focus root when the counter drops to 0.


This is a more general approach, but also riskier given that it changes the semantics of st_focus_manager_remove_group() - grepping though the sources shows we hardly ever use remove_group()/removeGroup() anywhere and the patch looks safe with the few places where we do, but still ...

(In reply to comment #10)

> Should clicking outside the bubble only dismiss the bubble, or dismiss the
> entire tray? Right now we have "escape" and "click outside" tied to the same
> action, so it would require a bit of a code rework to fix that one.

I'm afraid I would diverge there. As we have explicit close button for bubbles, clicking outside of the bubble should dismiss the tray as well as the bubble...
 
> (And should the same be true for popup menus as well? Right now, clicking
> outside of the combo box in the user menu will only close the combobox)

Yea I don't think that's appropriate for mouse. Clicking outside of the menu should dismiss the whole thing.

Review of attachment 224342 [details] [review]:

Interesting. Why are we double adding/removing the message tray?

::: src/st/st-focus-manager.c
@@ +186,3 @@
                     G_CALLBACK (remove_destroyed_group),
                     manager);
+  count = GPOINTER_TO_INT (g_hash_table_lookup (manager->priv->groups, root));

not a fan of the NULL to 0 cast here. I'd rather do:

    int count;
    gpointer count_p = g_hash_table_lookup (...);
    if (count_p != NULL)
        count = GPOINTER_TO_INT (count_p);
    else
        count = 0;

@@ +205,3 @@
+  count = GPOINTER_TO_INT (g_hash_table_lookup (manager->priv->groups, root));
+  if (count == 0)
+    return;

Same here.

(In reply to comment #18)
> (In reply to comment #10)
> 
> > Should clicking outside the bubble only dismiss the bubble, or dismiss the
> > entire tray? Right now we have "escape" and "click outside" tied to the same
> > action, so it would require a bit of a code rework to fix that one.
> 
> I'm afraid I would diverge there. As we have explicit close button for bubbles,
> clicking outside of the bubble should dismiss the tray as well as the bubble...

That's different. Clicking the close button will destroy the notification entirely, not just hide it.

Created attachment 224351 [details] [review]
focus-manager: Make groups "refcounted"

(In reply to comment #19)
> Review of attachment 224342 [details] [review]:
> 
> Interesting. Why are we double adding/removing the message tray?

See the commit message of the other patch.


Somehow I made an error when testing, and it turns out that this patch is not enough to fix the problem - _hideTray() (which calls removeGroup()) is actually run twice when leaving the overview.

Created attachment 224352 [details] [review]
messageTray: Fix _hideTray() being called recursively

_hideTray() is called by _updateState() when the tray is visible
but should be hidden; however, _updateState() may be called again
from within _hideTray() when releasing the GrabHelper grab, so
unless we update the _trayState variable before that, _hideTray()
will be called a second time.

I do wonder if we should go with the simple keynav fix though - with the message tray state being modified all over the place, including non-obvious ones, something like this is almost guaranteed to happen again ...

(In reply to comment #20)
> That's different. Clicking the close button will destroy the notification
> entirely, not just hide it.

I initially intended the close button on the bubble to just close it and not dismissing the MT along with it. When the message tray does get closed, however, a clean up would occur. All transient messages would be cleared ("destroyed"). But while in the MT, closing a bubble would not result in not being able to get back to it. We might render the icons that will be cleared on MT close differently, like opacity: 0.5; Worth noting is that conversations are resident (they remain in tray even after the user has seen them) and have to be explicitly removed using the context menu.

I apologise for omitting an important detail in the wiki, I thought I did put it down.

Review of attachment 224352 [details] [review]:

Yes.

Review of attachment 224351 [details] [review]:

I like this.

Attachment 224351 [details] pushed as 94c1d5a - focus-manager: Make groups "refcounted"
Attachment 224352 [details] pushed as afcd90a - messageTray: Fix _hideTray() being called recursively

This is RESOLVED FIXED now, right?

No, not really:

(In reply to comment #7)
> in the tray:
> left arrow moves focus left,
> right arrow moves focus right,
> space opens notification

^^^ only that part.

The part when the summary boxpointer is shown is still completely missing, so right now there's only Escape Ctrl-M to "navigate" from one "open" summary item to another.

Created attachment 224372 [details] [review]
messageTray: Allow to open summary item with up arrow

When using keynav in the top bar, menus may be opened using the
down arrow; in a similar fashion, allow to open the summary
boxpointer with the up arrow.

Review of attachment 224372 [details] [review]:

I'm not sure that this is a good idea, given that Telepathy history is navigable with up/down. Designer input on this?

(In reply to comment #30)
> Review of attachment 224372 [details] [review]:
> 
> I'm not sure that this is a good idea, given that Telepathy history is
> navigable with up/down. Designer input on this?

Telepathy history is irrelevant for that patch. It is only a concern for *closing* the boxpointer with the down arrow (in which case the entry would take precedence over the boxpointer anyway), though it would actually be consistent with the top bar to only use Escape for closing.

(In reply to comment #31)
> (In reply to comment #30)
> > Review of attachment 224372 [details] [review] [details]:
> > 
> > I'm not sure that this is a good idea, given that Telepathy history is
> > navigable with up/down. Designer input on this?
> 
> Telepathy history is irrelevant for that patch. It is only a concern for
> *closing* the boxpointer with the down arrow (in which case the entry would
> take precedence over the boxpointer anyway), though it would actually be
> consistent with the top bar to only use Escape for closing.

Right. That's what I meant. If the "up arrow opens notification" is supposed to be symmetric with "down arrow closes notification", we can't implement both. That's why I wanted design input.

(In reply to comment #32)

> 
> Right. That's what I meant. If the "up arrow opens notification" is supposed to
> be symmetric with "down arrow closes notification", we can't implement both.
> That's why I wanted design input.


I don't think that this is a very important concern. Try it in the top panel: down arrow opens a menu, up arrow doesn't close it. The world doesn't end...

Created attachment 224539 [details] [review]
grabHelper: Ungrab the entire stack on "outside clicks"

Currently clicks outside the grabbed actors are handled the same as
the user pressing Escape - a single actor is popped from the grab stack.
However according to the design, outside clicks should release all grabs.

Created attachment 224540 [details] [review]
messageTray: Add shortcuts to summary boxpointer

Currently opening the summary boxpointer acts as a stop gap for
keynav - the only shortcut still working is "Escape" to hide the
tray altogether.
Change the handling of Escape to only close the summary boxpointer
and allow to use the down arrow as alternative (unless the boxpointer
already processes the key press itself of course, like the chat
entry does). Also add a Delete shortcut to dismiss the open summary
item.

Obviously we can remove the handling of the down arrow from the patch, though I don't think it's too weird that it doesn't work for chat windows due to history (getting fancy, HistoryManager could not block arrow events when at the start/end of history, so down arrow would actually work for chat entries unless the user is actually scrolling through history)

Created attachment 224542 [details] [review]
grabHelper: Remove support for untracked grabs

Created attachment 224543 [details] [review]
messageTray: Allow to start keynav with Tab

Currently it is only possible to use keynav inside the tray if it
has been triggered with the keyboard shortcut. Make it possible to
initiate keynav by hitting Tab in other cases as well.

Created attachment 224544 [details] [review]
messageTray: Move focus ourselves when entering by keybinding

When the tray is triggered by keybinding rather than dwelling, the
first summary item should be given key focus. Currently this is
achieved by grabbing the focus before toggling the tray, so that
the grabHelper will move the focus for us. However this interferes
with the grabHelper's focus save/restore mechanism - for instance,
after using the keybinding once, the tray will always come up with
the first item focused.

Review of attachment 224539 [details] [review]:

::: js/ui/grabHelper.js
@@ +334,3 @@
                 this._ignoreRelease = true;
+            while (this._grabStack.length)
+                this.ungrab();

What you wanted was this.ungrab(this._grabStack[0]);

Review of attachment 224540 [details] [review]:

Code looks fine.

::: js/ui/messageTray.js
@@ +2440,3 @@
+   _onSummaryBoxPointerKeyPress: function(actor, event) {
+       switch (event.get_key_symbol()) {
+           case Clutter.KEY_Down:

Have we gotten an OK from the designers on this? Again, this will collide with Telepathy chats.

Actually, I wonder if we could do something where if you press down when you're at the last history entry, it would pop back down for you.

@@ +2445,3 @@
+               this._updateState();
+               return true;
+           case Clutter.KEY_Delete:

Have we gotten an OK from the designers on this? Again, this will collide with Telepathy chats.

Review of attachment 224542 [details] [review]:

Sure.

Review of attachment 224542 [details] [review]:

Sure.

Review of attachment 224543 [details] [review]:

Setting the focus to the tray universally like you sort of do in the next patch should work just as well, no?

That would mean that when you press Tab, the StFocusManager code would try to navigate inside it?

(In reply to comment #40)
> Actually, I wonder if we could do something where if you press down when you're
> at the last history entry, it would pop back down for you.

See comment #35. And yes, it works quite nicely IMHO, so I'll attach a patch.

Created attachment 224548 [details] [review]
history: Allow events to bubble up when arrowing past the first/last item

Currently the HistoryManager consumes all arrow up/down key presses
unconditionally. Change this to only consume the event if the entry
text was actually changed, e.g. not when trying to move past the
first/last item.

Review of attachment 224548 [details] [review]:

Yeah.

Now what about Delete? Opening a Telepathy chat and hitting Delete to remove the bubble, only to find that it doesn't work sounds a bit poor.

(In reply to comment #39)
> What you wanted was this.ungrab(this._grabStack[0]);

this.ungrab({ actor: this._grabStack[0].actor }); actually, but thanks for the pointer - fixed locally.

Created attachment 224557 [details] [review]
telepathyClient: Bubble up unhandled Delete key presses

In the message tray's summary, delete is used to remove a notification.
This does not work for chat notifications, as the focused entry will
consume the event - however, as the delete key doesn't do anything
useful in the entry when the cursor is positioned at the end, manually
bubble up the event in that case which allows the shortcut to work.

This is definitively a hack, and to be honest, I think it is better to have the delete key not working in chat notifications - going from "delete some text" to "delete the entire conversation" due to an unintended extra key press is just too unforgiving.

Created attachment 224558 [details] [review]
messageTray: Allow to start keynav with Tab

(In reply to comment #43)
> Setting the focus to the tray universally like you sort of do in the next patch
> should work just as well, no?

Not "just", but almost, yeah

(In reply to comment #49)

> This is definitively a hack, and to be honest, I think it is better to have the
> delete key not working in chat notifications - going from "delete some text" to
> "delete the entire conversation" due to an unintended extra key press is just
> too unforgiving.

Yeah, don't do that. It will be irritating to have a hidden, more destructive meaning for 'Delete' depending on where the text cursor happens to be.

Yeah, the patch was more meant as an illustration that sometimes inconsistency is the lesser evil. The question remains if the shortcuts outlined in comment #7 are what we want (even if Delete won't work in chat notifications) or whether we want something else ...

Yeah, for some reason, every time I say that those shortcuts are fine, somebody comes in and asks for 'designer approval'.

Can't we just get some shortcuts in, please?

We can always improve things if it turns out to be not quite perfect.

Fine with me of course. For what it's worth, I don't consider the Delete conflict too serious - while users may get confused about the Down arrow if they don't know about the history feature, it should be obvious that Delete "does something" in the entry and is therefore already taken. If they figure out that Delete can be used to remove non-chat notifications in the first place of course ...

Review of attachment 224372 [details] [review]:

Looks good. The char history doesn't seem relevant here since this is only triggered when the summary item has the keyboard focus already.

Review of attachment 224544 [details] [review]:

Looks good.

Review of attachment 224558 [details] [review]:

Yes, seems good.

Attachment 224372 [details] pushed as a5d6005 - messageTray: Allow to open summary item with up arrow
Attachment 224539 [details] pushed as 809cbf5 - grabHelper: Ungrab the entire stack on "outside clicks"
Attachment 224540 [details] pushed as 906368d - messageTray: Add shortcuts to summary boxpointer
Attachment 224542 [details] pushed as ef7b74a - grabHelper: Remove support for untracked grabs
Attachment 224544 [details] pushed as 6ef3c62 - messageTray: Move focus ourselves when entering by keybinding
Attachment 224548 [details] pushed as 2c130c8 - history: Allow events to bubble up when arrowing past the first/last item
Attachment 224558 [details] pushed as f2af0be - messageTray: Allow to start keynav with Tab

I agree with Jimmac's points in #c23. The tray exists as a place where persistent notifications reside. Someone might want to repeatedly go back to an item, and they might want to look at several conversations while the tray is open.

If someone clicks a close button, we shouldn't remove it from the tray, and the tray should remain open. It's simply a way of hiding the bubble.

Mmmh, not sure about that to be honest - in any case, let's take this to bug 682237 (or a new one if you don't consider that a good fit).

Is there anything that stands in the way of making Super+M a toggle?

I have local patches which fix that, but they are pretty intrusive - it's not a trivial problem.

(In reply to comment #62)
> I have local patches which fix that, but they are pretty intrusive - it's not a
> trivial problem.

We could do the hack solution and set up a key-press-event that checks for <Super>M, no?

Also, would you mind releasing your patches somewhere? I know both Giovanni and I have made super similar patches (allowing key bindings inside of modals), so it would be helpful to see how your patches are different.