Created attachment 172380 [details] [review]
main: make pushModal/popModal handle global.stage_input_mode

In particular, if you pushModal() when the stage is in
Shell.StageInputMode.FOCUSED, then when you later popModal(), it
should go back to that mode, rather than NORMAL. Needed to make panel
menu keynav work correctly.

Created attachment 172381 [details] [review]
A11yTabManager: implement Ctrl-Alt-Tab

Add A11yTabManager, which allows tabbing between StFocusManager groups,
and fix up the panel to be keyboard navigable.

FIXME: we need proper keyboard focus indication in the panel; the current
indicator is just a temporary hack

depends on bug 621671. note that the ctrl-alt-tab dialog pretends to let you tab to the message tray, but that doesn't really do anything useful yet

as noted by the FIXME, this is still somewhat in progress, but it's reviewable

Review of attachment 172380 [details] [review]:

This doesn't smell right to me - there's nothing to enforce that changes to the input mode are nested with respect to pushModal calls.

I think probably the right thing to do is to add a main.js API for adjusting the input mode, memorize but don't set if that is called during a modal grab, restore to the "current mode" at the end of a grab stack, and have a policy of never calling global.set_stage_input_mode() directly from anywhere other than the main.js code. (Or alternatively, move pushModal/popModal to ShellGlobal and do all the logic in C code.)

Review of attachment 172381 [details] [review]:

Basic code structure looks fine to me, seems to work pretty well except for the problem noted on IRC (only works correctly if you start from the "normal" focus mode), and the FIXME focus indicator. Few more comments below.

::: data/theme/gnome-shell.css
@@ +1166,3 @@
+/* FIXME: temporary ugly hack */
+:focus {
+    outline: 1px solid #00ffff

Definitely needs fixing before we land :-)

::: js/Makefile.am
@@ +11,3 @@
 	perf/core.js		\
 	prefs/clockPreferences.js \
+	ui/a11yTab.js		\

is this really the best name? Yes, control-alt-tab is basically useful only for people who *can't* use a mouse, but "accessibility" doesn't seem to be fundamental to its nature. 'controlAltTb.js' 'globalAltTab.js' 'chromeAltTab.js' ?

::: js/ui/a11yTab.js
@@ +43,3 @@
+
+    focusGroup: function(root) {
+        global.set_stage_input_mode(Shell.StageInputMode.FOCUSED);

This isn't right if we are in the overview?

@@ +51,3 @@
+                    global.stage_input_mode == Shell.StageInputMode.NORMAL) {
+                    global.disconnect(id);
+                    global.stage.set_key_focus(null);

This seems likely to interfere with code trying to do stuff with focus - if we say that we reset key focus to no widget focused in NORMAL/NONREACTIVE that should be done centrally in main.js (or ShellGlobal as discussed in my comment on the other patch)

@@ +148,3 @@
+    _keyReleaseEvent : function(actor, event) {
+        let [x, y, mods] = global.get_pointer();
+        let state = mods & Clutter.ModifierType.MOD1_MASK;

Wow, that's an ugly hack round-tripping on every keyrelease event. But I guess it's copied straight-over from altTab.js so likely is as-good-as-possible. Though it doesn't look like the usage of global.get_pointer() was even considered for bug 629368 ... yeah, the Mutter code to "get it right" is complex, but it would be better to export it out than just doing something cheesy, slow, and racy. Not a blocker for this patch, but I'd appreciate it if we had a bug filed. (OK, maybe this is better-enemy-of-good-enough, but if we already *have* the better code.)

@@ +182,3 @@
+
+    _onDestroy : function() {
+        this._popModal();

Should we be doing all this stuff before we show the fade out? - doing it in this order means that if I control-alt-tab and then start typing anything I type during the animation is lost.

@@ +214,3 @@
+                                     vertical: true });
+
+        let icon = St.TextureCache.get_default().load_icon_name(iconName, St.IconType.SYMBOLIC, POPUP_APPICON_SIZE);

Should this really be symbolic? I guess that's a designer question. (Whether you want to emphasize that these aren't apps but a different sort of creature, or you want a general visual similarity with the alt-tab switcher. I'd also wonder if the symbolic iconss will look OK huge.)

::: js/ui/main.js
@@ +208,3 @@
+function _makeDesktopProxy() {
+    let proxy = new St.DrawingArea({ name: "desktop-proxy",
+                                     can_focus: true });

St.DrawingArea is a messy actor that does significant work on paint/allocate. Doesn't seem like a good choice for an invisible actor - we'll probably end up creating 0x0 cairo surfaces and clutter textures, etc.

::: js/ui/windowManager.js
@@ +527,3 @@
 
+    _startA11ySwitcher : function(shellwm, binding, window, backwards) {
+        Main.a11yTabManager.popup();

I guess Shift-Control-Alt-Tab is basically impossible to hit :-)

(In reply to comment #4)
> Review of attachment 172380 [details] [review]:
> 
> This doesn't smell right to me - there's nothing to enforce that changes to the
> input mode are nested with respect to pushModal calls.

ok, this was part of trying to enforce that a11yTab can detect when the panel, etc, has lost the focus, but it turns out we don't really need that. So this whole patch can go.

(In reply to comment #5)
> +/* FIXME: temporary ugly hack */
> +:focus {
> +    outline: 1px solid #00ffff
> 
> Definitely needs fixing before we land :-)

replaced this with a panel-button-specific focus indication (the same as the pressed indication). We will need to add additional actor-specific focus indicators as we extend keynavigability to other parts of the shell.

> +    ui/a11yTab.js        \
> 
> is this really the best name? Yes, control-alt-tab is basically useful only for
> people who *can't* use a mouse, but "accessibility" doesn't seem to be
> fundamental to its nature. 'controlAltTb.js' 'globalAltTab.js'
> 'chromeAltTab.js' ?

I see the feature as being fundamentally more about accessibility than it is about the specific set of modifier keys. But sure. ("chromeAltTab" doesn't work because it's also how you get to the desktop. Well, until the desktop goes away I guess...)

> +    _keyReleaseEvent : function(actor, event) {
> +        let [x, y, mods] = global.get_pointer();
> +        let state = mods & Clutter.ModifierType.MOD1_MASK;
> 
> Wow, that's an ugly hack round-tripping on every keyrelease event.

We can fix it when we get the bug reports from people who type faster than we can round-trip to the server...

> +    _onDestroy : function() {
> +        this._popModal();
> 
> Should we be doing all this stuff before we show the fade out?

in the normal case it does, because _finish() calls popModal before starting the tween. But in other cases it's messed up. Will Fix.

> +        let icon = St.TextureCache.get_default().load_icon_name(iconName,
> St.IconType.SYMBOLIC, POPUP_APPICON_SIZE);
> 
> Should this really be symbolic?

Given that it is stuff related to the shell rather than related to apps, the answer, as I understand it, is yes. However, the design of the a11ytab popup is assumed to be subject to change.

Created attachment 173393 [details] [review]
St: add keyboard support to StClickable and StButton

needed to make the Activities button work

Created attachment 173394 [details] [review]
A11yTabManager: implement Ctrl-Alt-Tab

Add A11yTabManager, which allows tabbing between StFocusManager groups,
and fix up the panel to be keyboard navigable.

Comment on attachment 173394 [details] [review]
A11yTabManager: implement Ctrl-Alt-Tab

doesn't yet address all of the problems fro mthe previous review

Review of attachment 173393 [details] [review]:

Some corner cases in StButton that look questionable to me with multiple keys/button getting hit at once, otherwise looks good to me

::: src/st/st-button.c
@@ +175,3 @@
         return FALSE;
 
       clutter_ungrab_pointer ();

You are going to get a stuck grab if someone does the wrong combo of mouse press and key press. You probably should make the grab match is_pressed exactly  - move it into real_pressed/real_released

@@ +194,3 @@
+      event->keyval == CLUTTER_KEY_Return)
+    {
+      ST_BUTTON_GET_CLASS (actor)->pressed (ST_BUTTON (actor));

If return key and clicking occurs at the same time might be good to avoid pressed+pressed+release by adding a a check on _is_pressed here. (I don't think the details of when the pressed and released occur in that case matter, but subclass code might have subtle deps on pressed not happening when the button is already pressed).

(the same case occurs with a chord of space and return actually, so cat walking across the keyboard)

Comment on attachment 173393 [details] [review]
St: add keyboard support to StClickable and StButton

StButton has some other issues, I'm going to split this out

Created attachment 173796 [details] [review]
Use more actor.grab_key_focus() and less stage.connect('key-press-event')

Until recently, the clutter keyboard focus was almost always kept on
the stage, and bits of code that wanted to do stuff with the keyboard
would just watch for key-press-events on the stage. In several places,
the code wasn't even bothering to ensure that the focus was on the
stage, which caused problems with other actors that explicitly grabbed
focus.

A previous fix for this (f21403fd) was to always reset the focus to
the stage after calling pushModal(), but a better fix is to just
actually make use of the keyboard focus everywhere rather than having
everyone try to read events off the stage.

Now pushModal(actor) also does actor.grab_key_focus(), and various
bits of code have been changed to read key events off their own
toplevels rather than off the stage, meaning there's no chance of them
accidentally getting someone else's events.

Created attachment 173797 [details] [review]
ctrlAltTabManager: implement Ctrl-Alt-Tab

Add CtrlAltTabManager, which allows tabbing between StFocusManager
groups, and fix up the panel to be keyboard navigable.

(In reply to comment #12)
> Created an attachment (id=173796) [details] [review]
> Use more actor.grab_key_focus() and less stage.connect('key-press-event')

The largest part of this patch is the message tray chat stuff. AFAICT, it still works correctly in all the cases that used to work, but probably Marina should check since she's more familiar with what things are supposed to work.

So, noticed in testing.

 - If a panel menu was keynav triggered , I think it's probably good if escape takes you back to the panel. 
 - Desktop is in the control-alt-tab even if you don't have nautilus managing your desktop (or I might have nautilus managing my desktop but crashing on startup? I don't have a desktop window in any case)
 - Hitting return on the clock activates the preferences menu rather than the clock. (In theory the preferences menu should come up on Clutter_KEY_Menu and whatever the standard key shortcut is for that - Control-F10 maybe? but we're getting rid of that menu anyways)

Review of attachment 173796 [details] [review]:

Generally looks good to me, and I don't see any problems in testing for the parts I can test. I'll reassign the bug to Marina for some extra testing of the message tray key focus when I get done reviewing the other patch, since I don't even have a working Empathy client here at all. The changes to messageTray.js look like they *should* work.

::: js/ui/altTab.js
@@ +388,2 @@
         if (this._keyReleaseEventId)
+            this.actor.disconnect(this._keyReleaseEventId);

Not necessary, right? this.actor has been destroyed which will disconnect the signal handlers.

::: js/ui/lookingGlass.js
@@ +933,1 @@
             Lang.bind(this, this._globalKeyPressEvent));

Better to just connect in _init and never disconnect?

::: js/ui/panelMenu.js
@@ +32,3 @@
         this.menu.toggle();
+        if (this.menu.isOpen)
+            this.actor.grab_key_focus();

If the point of this is what I'm thinking - to take the focus back from the menu - then it could use a comment. If it is something else, it would evidently need a comment as well :-)

Review of attachment 173797 [details] [review]:

Think the Desktop-in-alt-tab problem-when-no-desktop problem needs to be fixed before committing, other than that, only one thing I see in here that raised a question.

::: js/ui/ctrlAltTab.js
@@ +94,3 @@
+
+        this._keyPressEventId = global.stage.connect('key-press-event', Lang.bind(this, this._keyPressEvent));
+        this._keyReleaseEventId = global.stage.connect('key-release-event', Lang.bind(this, this._keyReleaseEvent));

Wait, didn't you just patch the equivalent code in altTab.js in the other patch?

Assigning to Marina to test the Message tray focus behavior and make sure it's not regressing from what it was previously.

I have just tested this patches, and in general terms its works fine, but I found some issues:

* When you move the focus to the panel, you can focus on the 'Activities' button, but you can't activate it (at least not using the Enter button, like on the icons)
* You can use the down button in order to open any of the popups. But once focus on one of the popup items, you can't close the popup (ie: going up). A workaround is pressing Esc, although in this case you lose all the focus on the panel.
(Although it is true that if you open the popup with enter, if you press again enter it is closed)
* If a popup in any icon is open you can navigate to other icon. But in this case it seems that the 'Activities' button is out of the focus-chain. Is somewhat odd (IMHO) be able to navigate to 'Activities' in some cases, but not in others.

Not sure if those issues applies to this bug or should be managed in other one.

Finally, after test it with orca there are several missing information, so I will open a bug similar to bug 626660 but to this aspect.

(In reply to comment #19)
> * When you move the focus to the panel, you can focus on the 'Activities'
> button, but you can't activate it

You need bug 633853, sorry.

> * You can use the down button in order to open any of the popups. But once
> focus on one of the popup items, you can't close the popup (ie: going up).

This matches the behavior of Gtk menus and the GNOME 2.x panel

> * If a popup in any icon is open you can navigate to other icon. But in this
> case it seems that the 'Activities' button is out of the focus-chain. Is
> somewhat odd (IMHO) be able to navigate to 'Activities' in some cases, but not
> in others.

Mmm... this is also similar to GNOME 2.x panel navigation; once you enter the menus, it is difficult to get out of them and to some other panel applet.

Possibly worth changing anyway though.

*** Bug 634194 has been marked as a duplicate of this bug. ***

Created attachment 174093 [details] [review]
Use more actor.grab_key_focus() and less stage.connect('key-press-event')

Created attachment 174094 [details] [review]
popupMenu: fix up grab/ungrab handling

Fix the panel menus to avoid unnecessarily bouncing out of modal (bug
634194) and to do a better job of keeping the keyboard focus in the
right place

Created attachment 174095 [details] [review]
ctrlAltTabManager: implement Ctrl-Alt-Tab

Now instead of hardcoding "Desktop", we just add the complete list of
windows that plain metacity would have shown, so if you're running docky
or whatever, it will show up in the ctrl-alt-tab list

Created attachment 174802 [details] [review]
Little update on the "popupMenu: fix up grab/ungrab handling" patch

Today I rebased against the master and the ctrlAltTab popup didn't appear. This was caused by commit dc1e2350.

This little patch solves this by creating the icon directly with St.Icon (), instead of call st_texture_cache_load_icon_name

While working on the bug 634016 I found an issue. 

I was working with the fact that interacting with the Ctrl+Alt+Tab popup doesn't works at all with Orca (after applying all the a11y related patches).

The problem is that the keyboard navigation is ad-hoc, not using the core keyboard navigation support implemented on Bug 621671. So as there isn't any keyboard focus grab, cally doesn't emit any focus change signal.

I guess that this keyboard navigation was implemented ad-hoc in order to be homogeneous to the current Alt+Tab implementation. BTW: this means that Alt+Tab popup is affected by the same problem.

As now there are a common core I think that both should be migrated. A quick draft of things required to do (probably I'm missing something):
  * Define a new focus group for the popup
  * Set "can_focus=true" on each item of the switcher
  * Give the focus to the element selected when the popup is shown
  * Remove most of the ad-hoc selection code, and move to the core focus code (so "selected item"=>"focused item") for drawing and so on

I have a quick'n'dirt test patch doing some of things that I used to verify this. I will upload this patch on bug 634016 to avoid too much noise here.

The next question is if this issue would be solved here, on in other bug, as this also affect alt+Tab code (a bug like "migrate alt+tab/ctrl+alt+tab popup ad-hoc keynav to the core keynav support").

After a brief chat with Joanmarie, she pointed that probably it doesn't makes sense to give the keyboard focus to each element, and that technically is the switcher widget the one getting the key focus, and each item just selected (as the code itself says, btw)

After thinking it a little, I think that it makes sense, and this is something similar to what happens on a gtktreeview for example. So I would explore the option to just change the state of those objects to selected on bug 634016, while deciding if it makes sense to give the focus to each switcher item.

She also asked if there are a "can_selected" property, but I think that this shouldn't be required for the moment, as it is clear which object are selectable and which one is selected on the alt+tab and ctrl+alt+tab code.

Created attachment 175662 [details] [review]
Use more actor.grab_key_focus() and less stage.connect('key-press-event')

rebased and updated for overview-relayout

Created attachment 175663 [details] [review]
popupMenu: fix up grab/ungrab handling

rebased

Created attachment 175664 [details] [review]
ctrlAltTabManager: implement Ctrl-Alt-Tab

rebase, update to use StIcon

Review of attachment 175662 [details] [review]:

See minor comments on previous version of this that apparently got skipped.

Review of attachment 175663 [details] [review]:

Boolean explosion!

I have some vague notion that separating out "opening a menu" from "starting/ending menu navigation" would make this cleaner, but assuming all the booleans are passed correctly and not swapped around or something (read through it but could easily have missed something), then should work.

Review of attachment 175664 [details] [review]:

Looks good, no obvious bugs in testing (there are clearly places where the keynav is a little rough - e.g., the behavior of the volume menu where the arrow keys move the slider once that menu is open, but not areas to improve, not blocking this patch.)

(In reply to comment #33)
> (there are clearly places where the
> keynav is a little rough - e.g., the behavior of the volume menu where the
> arrow keys move the slider once that menu is open

yup, bug 635547

(one possibility there is to switch back to a vertical slider, although then we probably have to both (a) remove the menu item linking to the sound control panel (or just turn it into a control-panel icon rather than words?) and (b) move microphone input-level setting out into its own separate status icon)

Created attachment 176552 [details] [review]
Use more actor.grab_key_focus() and less stage.connect('key-press-event')

addresses previously-skipped-over comments from comment 16

Created attachment 176553 [details] [review]
popupMenu: fix up grab/ungrab handling

rewritten... now with less boolean explosion, and it actually works better
too... i'm not sure if other stuff changed since the original patch or
what, but when I was re-testing I found a few cases that didn't work right,
which now work better.

Created attachment 176554 [details] [review]
ctrlAltTabManager: implement Ctrl-Alt-Tab

no changes, just rebased

Review of attachment 176552 [details] [review]:

Looks good

Review of attachment 176553 [details] [review]:

Makes sense to me, wasn't able to test since some of this set seems to need a bit of rebasing, but if it works for you, OK to commit with a few small details noted below

::: js/ui/popupMenu.js
@@ +1048,3 @@
+            // sourceActor. Otherwise just reset the focus to where it
+            // was right before the ungrab.
+            if (!this.grabbed && this._preGrabInputMode == Shell.StageInputMode.FOCUSED) {

this.grabbed is always false here, since we've ungrabbed?

@@ +1062,3 @@
+        if (this._activeMenu) {
+            let oldMenu = this._activeMenu;
+            this._activeMenu = null;

Needs a comment here since setting activeMenu to null is not-obviouslt "don't drop grab"

@@ +1105,3 @@
     },
 
+    _isOnActiveMenu: function(actor) {

This needs a rename "On" seems to refer to an event being on something on the previous name "eventIsOnActiveMenu" this is more like isActiveMenuOrSource

Review of attachment 176554 [details] [review]:

marking a-c-n as a rebase of a previously reviewed patch

pushed with rebasing/fixes

Attachment 176552 [details] pushed as 4dd4c9f - Use more actor.grab_key_focus() and less stage.connect('key-press-event')
Attachment 176553 [details] pushed as f326595 - popupMenu: fix up grab/ungrab handling
Attachment 176554 [details] pushed as d3de4e3 - ctrlAltTabManager: implement Ctrl-Alt-Tab