Created attachment 173717 [details] [review]
Enables navigation of buttons using arrow up, arrow down, and arrow right in notifications with buttons.

Users can now use the keyboard to navigate among buttons in notifications, instead of entirely using only mouse.

Partial fix for arrow down.

Review of attachment 173717 [details] [review]:

Marking needs-work since I don't think we'd want to land a patch that handled only "Down". You should check with Dan Winship (danw on IRC) on how this relates to the focus navigation work he's been doing. It may be possible to treat keynav in the message tray as part of that, or it may need special handling of keys directly.

Assigning to Dan for review of future patches.

Created attachment 175823 [details] [review]
Enables navigation using arrow keys in notification with buttons.

Users can now use the keyboard to navigate with buttons in notifications, instead of entirely using mouse.

Code has my own additions in help to understanding this patch, plus plenty of notes made to explain why I did this/that. I wonder if this is allowed... anyway, this is a very rough patch, .. i think.

Review of attachment 175823 [details] [review]:

Dan, is the general way we are going about handling the key presses the right one?

Hellyna, this is a good start! Let's wait for a confirmation from Dan that a patch along these lines is in fact in agreement with our overall keyboard navigation handling plan. If that's the case, following up on the comments below should get this patch closer to getting committed :).

For the final patch, you will need to remove the explanation about the comments in the patch from the commit message. You can always add a Bugzilla comment when committing a message by passing "-e" option to git bz attach. You then can add the extra text below the commit message or replace the commit message with your comment leaving the commit title line in place.

You will need to remove your name from the comments that were added by you to the code. You can add as many comments about things that are non-obvious as you like. You will also need to remove all the log statements.

::: data/theme/gnome-shell.css
@@ +846,3 @@
 }
 
+.notification-button:hover, .notification-button:selected {

You also need to add .notification-icon-button:selected and .notification-icon-button:activated to the corresponding lines for .notification-icon-button .

::: js/ui/messageTray.js
@@ +488,2 @@
         this._buttonBox.add(button);
+        button.connect('clicked', Lang.bind(this, function() { this._onButtonClicked(id) }));

The user should be able to move the selection with the arrow keys and hovering interchangeably. For that, you now need to connect to 'notify::hover' on buttons, so that if the button is hovered over, we remove 'selected' pseudo class from any other button, set it on the button that is being hovered over, and update this._selectedButtonIndex accordingly.

If we always keep one button selected, as I'm proposing in my comment below, the selection should stay on the button if the user hovered to it, and then hovered away. That's the reason for setting 'selected' pseudo class on the buttons that are hovered over. This effectively makes having '.notification-button:hover' in CSS unnecessary, but harmless.

@@ +501,3 @@
+    _onButtonClicked: function(id) {
+        log("_onButtonClicked: current actor is " + id );
+        this.emit('action-invoked', id);

This function is unnecessary as it only adds a log statement. The signal can be emitted inline when connecting to 'clicked' as before and in _onKeyPress() when we get Clutter.Return .

@@ +708,3 @@
         // TODO: hellyna: If it is a scrollable notification, don't capture Clutter.Down event to collapse the tray. Reasons
         // being, Clutter.Down is used to scroll the windows downwards. Either that or use KISS, omit the Clutter.Down
         // condition altogether.

You need to squash your previous patch that adds the check for Clutter.Down with this one. You comment brings up a valid point about a possible different expectation about what clicking a Down arrow would do. However, because it's more of a design question than a definite plan for the code, it should not be left in the code.

@@ +710,3 @@
         // condition altogether.
+        // hellyna: get the buttonCount so we can use it later in this function
+        this._buttonCount = this._buttonBox.get_children().length;

If this is a variable that is only used locally, you should define it with "let". As in "let buttonCount =". I think numberOfButtons would be a better name since for each notification it's a predefined number that normally doesn't get updated, like a count would.

@@ +711,3 @@
+        // hellyna: get the buttonCount so we can use it later in this function
+        this._buttonCount = this._buttonBox.get_children().length;
+        if (this._selectedButtonIndex != undefined) {

You can just check if (this._selectedButtonIndex)

@@ +713,3 @@
+        if (this._selectedButtonIndex != undefined) {
+            log("_onKeyPress: previous button index =" + this._selectedButtonIndex);
+            this._buttonBox.get_children()[this._selectedButtonIndex].remove_style_pseudo_class("selected");

You shouldn't remove the 'selected' pseudo-class from the currently selected button on any key press, just on Right and Left arrows.

Also, double quotes are reserved for strings that need to be translated, and you should use single quotes here. It's one of the few things mentioned here: http://live.gnome.org/GnomeShell/StyleGuide

@@ +718,3 @@
             Main.messageTray.escapeTray();
+            this._selectedButtonIndex = undefined;
+            this._buttonCount = undefined;

We usually set variables back to null, rather than undefined. Because you will make buttonCount, or numberOfButtons, a local variable, you won't need to unset it.

@@ +725,3 @@
+                // hellyna: if this._selectedButtonIndex is not defined, or if the index has already reached 0,
+                // we set the current selected button to be the right most button. Else we just move the button one
+                // step leftwards (ie, selectedButtonIndex--;)

I would like to propose that we select a left-most button by default when the first button is being added (i.e. this._buttonBox number of children is 0) in addButton(). You would need to set this._selectedButtonIndex to 0 there and add the 'selected' pseudo class to the button there. Then you should never have a situation when this._selectedButtonIndex should be set back to null or is null for the notification that has buttons. this._selectedButtonIndex should only be reset to null when we set this._buttonBox to null.

I also prefer to initialize all the class variables in the _init, even if that just means setting them to null. However, since it looks like this._buttonBox is not initialized there, it's ok to not initialize this._selectedButtonIndex there either.

@@ +727,3 @@
+                // step leftwards (ie, selectedButtonIndex--;)
+                if (this._selectedButtonIndex === undefined || this._selectedButtonIndex == 0) {
+                    this._selectedButtonIndex = this._buttonCount - 1;

Our convention is not to use curly braces for clauses that just have one statement, unless there is a corresponding clause that has more than one statement. E.g.

if (x)
    y;
else
    z;

if (x) {
    y;
}
else {
    v;
    z;
}

@@ +747,3 @@
+            // If Enter(Return) is pressed, emit the button signal by calling this._onButtonClicked(id)
+            log("_onKeyPress: Clutter.Return: Button ID is " + this._buttonBox.get_children()[this._selectedButtonIndex].id)
+            this._onButtonClicked(this._buttonBox.get_children()[this._selectedButtonIndex].id);

Just call this.emit('action-invoked', this._buttonBox.get_children()[this._selectedButtonIndex].id); here.

You also want to remove 'selected' pseudo class, and add 'activated' pseudo class here. (That's the point of adding notification-button:activated in CSS.)

@@ +752,3 @@
+        if (this._selectedButtonIndex != undefined) {
+            log("_onKeyPress: current button index=" + this._selectedButtonIndex);
+            this._buttonBox.get_children()[this._selectedButtonIndex].add_style_pseudo_class("selected");

Again, you want to add/remove the pseudo class only if the change was made.

Created attachment 176555 [details] [review]
Enables navigation using arrow keys in notifications with buttons.

Users can now use the keyboard and mouse to navigate with buttons interchangeably, instead of entirely using only mouse.

A huge cleanup both on visibility and code optimization. I hope that this is complete :).

Created attachment 178263 [details] [review]
Enables navigation using arrow keys in notifications with buttons.

Hellyna's patch updated to apply to master.

Review of attachment 178263 [details] [review]:

Suggested commit message:
"Enable navigation of notification buttons using arrow keys
Users can now use the keyboard and mouse to navigate the buttons interchangeably, instead of only using the mouse.
Down arrow results in the notification being closed and the next notification shown if one is queued, same as Esc."

The following commit http://git.gnome.org/browse/gnome-shell/commit/?id=4dd4c9f99f528667eb0b571a71ed23f61919f44b removed the usage of _onKeyPress() and I have just removed the function itself which was a left-over. You need to move all the key-handling you were doing there to _onCapturedEvent(). The comments I made about that part of the code itself still apply.

Most other comments are stylistic and very straight-forward.

::: js/ui/messageTray.js
@@ +455,3 @@
             if (this._buttonBox)
                 this._buttonBox = null;
+                this._selectedButtonIndex = -1;

You need curly braces around these two statements to make it logical. Alternatively, checking this._buttonBox doesn't actually give us anything if we are setting this._buttonBox to null anyway, so you can just remove the condition.

@@ +501,3 @@
         }
 
+        // hellyna: we set the id here so we know what signal is the specific button related to.

Suggested comment:
// We save the action id because we need to know which action id each button corresponds to.

@@ +506,2 @@
         this._buttonBox.add(button);
+        // Setting the selected button to the left-most button

Suggested comment:
// Setting the first button that is added to be selected.

@@ +507,3 @@
+        // Setting the selected button to the left-most button
+        this._selectedButtonIndex = 0;
+        this._buttonBox.get_children()[this._selectedButtonIndex].add_style_pseudo_class('selected');

You should only set this._selectedButtonIndex = 0 and add 'selected' pseudo class when the first button being added. Can check that either with this._buttonBox.get_children().length == 1 or with this._selectedButtonIndex < 0. I think this._selectedButtonIndex < 0 is a slightly nicer way to check. To use it, you need to first set this._selectedButtonIndex to -1 in _init(). Right after "this._keyPressId = 0;" is a good place to add it.

Btw, you can use this._setSelectedButton(0); here.

@@ +541,3 @@
+        if (button.hover) {
+            this._setSelectedButton(this._buttonBox.get_children().indexOf(button));
+        }

No need for curly braces.

@@ +738,3 @@
         }
+
+        if (this._selectedButtonIndex != -1 && this._buttonBox) {

Just as a preference, I'd check them in the other order
if (this._buttonBox && this._selectedButtonIndex >= 0)
because this._buttonBox has to be there to have this._selectedButtonIndex set. I would also check this._selectedButtonIndex >= 0 rather than this._selectedButtonIndex != -1, because what we really care about is whether it is set to a valid value, rather then whether it is not a particular invalid value we use to mean that it is unset.

@@ +739,3 @@
+
+        if (this._selectedButtonIndex != -1 && this._buttonBox) {
+            // We get the buttonCount so we can use it later in this function

This comment is self-evident, so it should be removed.

@@ +744,3 @@
+                    // If the index has already reached 0,
+                    // we set the current selected button to be the right most button. Else we just move the button one
+                    // step leftwards (ie, selectedButtonIndex--;)

Please right-align the text in the comment a little better and end it with a dot.

@@ +749,3 @@
+                    } else {
+                        this._setSelectedButton(this._selectedButtonIndex - 1);
+                    }

No need for curly braces.

@@ +753,3 @@
+                // If the index has already reached buttonCount -1,
+                // we set the current selected button to be the left most button. Else we just move the button one
+                // step rightwards (ie, selectedButtonIndex++;)

Right align better, end with a dot, and it also should be "numberOfButtons - 1" rather than "buttonCount -1".

@@ +758,3 @@
+                } else {
+                    this._setSelectedButton(this._selectedButtonIndex + 1);
+                }

No need for curly braces.

@@ +760,3 @@
+                }
+            } else if (symbol == Clutter.Return) {
+                this._buttonBox.get_children()[this._selectedButtonIndex].add_style_pseudo_class('activated');

You should remove the 'selected' pseudo class first.

@@ +761,3 @@
+            } else if (symbol == Clutter.Return) {
+                this._buttonBox.get_children()[this._selectedButtonIndex].add_style_pseudo_class('activated');
+                // If Enter(Return) is pressed, emit the button signal

The comment is unnecessary.

@@ +762,3 @@
+                this._buttonBox.get_children()[this._selectedButtonIndex].add_style_pseudo_class('activated');
+                // If Enter(Return) is pressed, emit the button signal
+	            this.emit('action-invoked', this._buttonBox.get_children()[this._selectedButtonIndex].id);

We now have _onActionInvoked() function, which takes action id. However, since we now keep button.id, you can remove the 'id' argument from that function and use actor.id in it instead. You can then call
this._onActionInvoked(this._buttonBox.get_children()[this._selectedButtonIndex]);
here.

Comment on attachment 178263 [details] [review]
Enables navigation using arrow keys in notifications with buttons.

OK, so since this was started we've gotten "real" keyboard navigation support in St, so the way that this all works should change to match that. (Then it will also integrate correctly with Ctrl-Alt-Tab when we make it so you can Ctrl-Alt-Tab to the message tray.)

The general idea is:

    - create a "focus group" on the notification; call
      St.FocusManager.get_for_stage(global.stage) to get the focus
      manager, and then call .add_group(this.actor) on that

    - set the "can_focus" property to true on the St.Buttons.

    - for CSS, use the "focus" pseudoclass rather than "selected";
      "focus" gets added and removed from the widget automatically
      when it receives/loses the keyboard focus

    - when grabFocus() runs, call .grab_key_focus() on whatever
      button should have the focus.

    - none of the rest of _onKeyPress should be needed; StFocusManager
      will handle left/right, and StButton will flash and emit 'clicked'
      when the user types Return or spacebar.

I've started implemented as what you've suggested dan. However I've noticed that hovering the mouse over the buttons does not really update the focused button to the one hovered. Hope I can catch you on IRC later today.

Created attachment 178809 [details] [review]
Here's the patch anyway, completely rewritten from scratch.

Sorry marina >.<.

Hellyna, so based on the IRC discussion it sounded like you should update the patch so that the buttons have one style of prelight on hover, and another one when they are selected. You can see, for example, how these are two distinct styles in GTK if you look at the dialog that appears if you try to close gedit with an unsaved file. The button that is hovered over should not be activated when you press enter, rather the button that is selected should. You can use :hover pseudo-class that St.Button already sets on hover to style the hover prelight.

Created attachment 179776 [details] [review]
Done. Hope the colors are ok.

looks good!