Ok, found the problem: the message tray keyboard source is destroyed when the keyboard is shown. This immediately resets the focus on the stage, and causes the keyboard to hide again.

The stuck grab is probably a duplicate of of 683537 and unrelated to the keyboard.

Oh and there is also a bigger problem here: the keyboard is meant to overlay active windows, but the message tray pushes them up. This confuses the message tray code and generates a mess with a window_group clone partially clipped and offset towards the top...

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

Created attachment 223909 [details] [review]
Keyboard: update for the message tray changes

The message tray now gets keyboard focus, so a few changes are needed to
the keyboard to avoid showing / hiding it in a cycle.
Additionally, the message tray is now modal and pushes the view up, but the
keyboard is shown below it. Solve this by applying a special styling to
the keyboard and message tray combination, and by not pushing the windows
up when the keyboard is shown.

Almost working. Problem is, the GrabHelper isn't helping, and I often
get a stuck grab. Could be the same as 683537 or not, I don't know; I'm
attaching the patch in case someone notices something I'm not seeing.

Found the cause of stuck grabs. They're not related to bug 683537.
Small debug log to prove what I'm about to patch:

      JS LOG: _fullGrab(false, true, false)
      JS LOG: _fullGrab(true, true, false)
      JS LOG: exit from _fullGrab(true, true, false)
      JS LOG: _modalCount: 1
      JS LOG: _grabFocusCount: 0
      JS LOG: exit from _fullGrab(false, true, false)
      JS LOG: _modalCount: 2
      JS LOG: _grabFocusCount: 0
      JS LOG: _fullUngrab(true)

Created attachment 223931 [details] [review]
GrabHelper: fix reentrancy issue with the message tray and the keyboard

GrabHelper._fullGrab causes a stage_input_mode change that affects the
keyboard, triggering a state update in the message tray and thus grabbing
again. This would result in one extra pushModal that was never popped.
I also fixed the actual issue in message tray, in a way that it no longer
calls .grab() reentrantly, but the GrabHelper changes should improve
robustness.

This fixes the grab problem, but then it doesn't like bug 664309, and
results in the keyboard hiding itself if a gtk3 app is focused (because
the commands from caribou-gtk3-module take precedence over the tray button)

Review of attachment 223931 [details] [review]:

The _fullGrab thing is wrong and just a result of me not understanding Dan's code when I went to update it, and I've been meaning to remove it. The correct way to do it is to take the specific kind of grab (pushModal, focus grab) the first time it appears on the stack. pushModal doesn't need anything other than the escape/press outside handler, and focus grab doesn't need anything other than the key focus/window focus changed handlers. If you have time, I'd set up _takeFocusGrab, _untakeFocusGrab, _takeModalGrab, _untakeModalGrab or whatever. Take one when the counter for something is 0 and the stack params wants it, untake one when we remove the last of its grab. Keep the navigation/hadFocus/fancy stuff in grab().

(I haven't looked at the patch at all, just saying)

Can we get this reviewed and landed for .92 ?

Review of attachment 223931 [details] [review]:

"affects the keyboard"? I don't see where the keyboard cares about stage-input-mode at all.

Created attachment 224384 [details] [review]
grabHelper: Rework grabbing code to properly handle stacking

When Dan Winship wrote the GrabHelper code originally, it didn't
handle a grab stack. I wrote the grab stack code hastily when landed
the message tray, not understanding all of the code that was involved
here.

Fix it so that we properly do the operations for each type of grab
when we first need to, and not sometimes when the first grab is taken.

OK, I implemented the thing I talked about in comment #7.

Review of attachment 224384 [details] [review]:

Ok, I tested this, seems to work fine, and the code makes sense.

Comment on attachment 224384 [details] [review]
grabHelper: Rework grabbing code to properly handle stacking

Attachment 224384 [details] pushed as b203a95 - grabHelper: Rework grabbing code to properly handle stacking

Created attachment 224426 [details] [review]
grabHelper: Remove unused parameters

Some left-overs from commit b203a95a7 ...

Review of attachment 224426 [details] [review]:

Whoops. It seems I forgot to set _grabbedFromKeynav. This fix is incorrect.

Created attachment 224537 [details] [review]
grabHelper: Set _grabbedFromKeynav

This one got lost in commit b203a95a7.

Created attachment 224538 [details] [review]
grabHelper: Remove unused parameters

Should have been 'needs-work' rather than rejected - this part is apparently fine.

Review of attachment 224537 [details] [review]:

Yes. These should probably be tracked on the focus stack item rather than "this", but we don't have time and the code works fine without that.

Review of attachment 224538 [details] [review]:

Sure.

Attachment 224537 [details] pushed as f6645a4 - grabHelper: Set _grabbedFromKeynav
Attachment 224538 [details] pushed as ff31ccd - grabHelper: Remove unused parameters

So, with the grab problem out of the way, can we proceed to make the osk work again ?

Testing again with .92, the on-screen keyboard and message tray still don't get along at all, to the point of stuck grabs that force me to restart the shell.

Clearly a blocker.

Florian, are you working on this ?

Yup. Giovanni's patch still applies mostly cleanly, but doesn't appear to fix all OSK related problems, e.g. I still get easily in a weird grab state where nothing short of killing the shell fixes it :-(

Florian, any update on this ?

Yes. While not working perfectly, I got it in a mostly usable state (no faulty grabs!), so I'll attach updated patches in a bit.

Created attachment 225078 [details] [review]
Keyboard: update for the message tray changes

The message tray is now modal and pushes the view up, but the keyboard
is shown below it. Solve this by applying a special styling to the
keyboard and message tray combination, and by not pushing the windows
up when the keyboard is shown.


Some parts of the old patch were incredibly hackish or didn't work at all - those are "good" parts split out to keep the original attribution.

Created attachment 225079 [details] [review]
messageTray: Only update keyboardVisible as necessary

This fixes a case of _updateState() being called recursively,
resulting in stray grab()/ungrab() calls the leave the entire
desktop in a stuck focus state.


(I know that conceptually this should be squashed into the previous patch, but it took me quite a while to track this down ...)

Created attachment 225080 [details] [review]
grabHelper: Ignore events from On-Screen-Keyboard

GrabHelper automatically releases grabs when the user clicks outside
the grabbed actors. However at least for the message-tray (which is
the only user of grabHelper at the moment), we must ignore any events
from the On-Screen-Keyboard, to prevent the tray from hiding at every
key press.

Created attachment 225081 [details] [review]
keyboard: Keep tray after clicking summary item

Currently if a summary item signals that it has handled a click
itself, the tray hides itself. This behavior is wrong for the
On-Screen-Keyboard, which appears as a unit with the tray, so add
a property to opt-out of the default behavior.

Created attachment 225082 [details] [review]
keyboard: Ignore focus changes caused by tray showing/hiding

The keyboard is shown/hidden automatically when (un)focusing a
ClutterText actor. This behavior breaks with the message tray now
grabbing/releasing key focus when toggled. Fix this by ignoring
all focus changes to or from the message tray.

Created attachment 225083 [details] [review]
keyboard: Fix check for extended keys

The existing check tested for non-existent properties.

(This patch is unrelated, I just noticed the problem when writing "Ignore events from On-Screen-Keyboard")

Created attachment 225086 [details] [review]
keyboard: Ignore focus changes from extended keys

The keyboard is shown/hidden automatically when (un)focusing a
ClutterText actor. This behavior is unwanted when opening the
extended keys popup, so focus changes to the popup are ignored.
However, we also want to ignore focus changes from the popup
to avoid the keyboard hiding itself after pressing an extended
key.

This fixes a rather annoying problem that presumably existed before the message-tray changes (too lazy to actually get an old shell version and test).

Some problems I'm still seeing:

  - the "tray" button doesn't work properly, it often fails
    to hide the tray while the keyboard is up

  - extended keys are broken - I can bring up the popup and
    click on a key, but nothing is actually typed (probably
    an existing bug and not a regression)

This is scheduled for 3.6.1 right?  Or are you aiming for 3.6.0, i.e. today?

Can you characterize these patches as to risk to parts of gnome-shell other than the OSK?  It looks like you're touching the message tray and grab helper bits?

(In reply to comment #35)
> This is scheduled for 3.6.1 right?  Or are you aiming for 3.6.0, i.e. today?
> 
> Can you characterize these patches as to risk to parts of gnome-shell other
> than the OSK?  It looks like you're touching the message tray and grab helper
> bits?

We can't really release 3.6.0 with a shell that's prone to stuck grabs.

(In reply to comment #35)
> This is scheduled for 3.6.1 right?  Or are you aiming for 3.6.0, i.e. today?

Good question. This bug is on the blocker list for 3.6, and rightfully so - currently just popping down the OSK will break the shell badly (a very unfortunate combination of key-focus and stage-input-mode that's almost impossible to get out without killing the shell). On the other hand, those are not exactly trivial one-liners ...


> Can you characterize these patches as to risk to parts of gnome-shell other
> than the OSK?  It looks like you're touching the message tray and grab helper
> bits?

I consider my patches safe - there are two changes to something other than the OSK itself, one of the form
  if (item.newPropertyOnlyUsedByKeyboard)
    doSomethingDifferent();
  else
    doWhatWasThereBefore();

the other boils down to this additional check:

  if (keyboard.contains(event.get_source()))
     return false;


Giovanni's patch is definitively riskier, as it makes changes to the non-keyboard path as well - namely, it decouples showing/hiding the message tray from shifting the desktop up/down. That said, the change is not that intrusive - in fact, for the non-keyboard case it basically boils down to a simple function split. I've also given the code a good share of testing, and while I had a bunch of problems with the keyboard enabled (hopefully fixed by my patches), I haven't experienced any while the OSK was disabled.


So I tend to think that the seriousness of the problem in question outweighs the risk the changes may have on the non-keyboard case - still, it's not that clear-cut that I couldn't understand the concerns.

In case we want to defer the fixes to 3.6.1, an alternative would be to disable the OSK completely for 3.6.0 (which should be fairly straightforward and safe).

I've tested these patches a bit - _much_ better. One can actually use the osk again, without risking struck grabs.

Here's a few things I've noticed, none of them serious problems for basic functionality:

- the osk works on both the login screen and the lock screen, but it doesn't seem to push the contents up - the unlock button ended up under the osk

- it is a little odd that the tray button is enabled and works on the login or lock screen. Imo, we should disable it there.

- automatic hiding/showing of the osk works for shell entries, but not for gtk apps.

- the graying out of the desktop does not kick in when showing the message tray on top of the osk

Here is the first serious issue I've found: turning the osk on, then off, and then on again gives me a shell crash.

Created attachment 225094 [details]
stacktrace

(In reply to comment #38)
> Here's a few things I've noticed, none of them serious problems for basic
> functionality:
> 
> - the osk works on both the login screen and the lock screen, but it doesn't
> seem to push the contents up - the unlock button ended up under the osk

Right, but we don't have any code to push up content (to avoid obscuring it with the keyboard) at the moment. So definitively out of scope for 3.6.


> - it is a little odd that the tray button is enabled and works on the login or
> lock screen. Imo, we should disable it there.

Good point. Not directly related to this bug, but should be easy to fix anyway.


> - automatic hiding/showing of the osk works for shell entries, but not for gtk
> apps.

It actually works occasionally, but I haven't tracked it down. Bug 681662 sounds related (which suggests that it was already flaky before the message-tray changes).


> - the graying out of the desktop does not kick in when showing the message tray
> on top of the osk

Oh right, it probably should.

Created attachment 225095 [details] [review]
Keyboard: update for the message tray changes

Gray out the desktop while showing the tray outside the overview, not only while the desktop is pushed up.

Created attachment 225096 [details] [review]
keyboard: Disable "tray" button in lock/login screen

It is not possible to summon the tray via shortcut or dwelling
while the screen is locked, so it is odd to allow it from the
on-screen-keyboard.

(In reply to comment #39)
> Here is the first serious issue I've found: turning the osk on, then off, and
> then on again gives me a shell crash.

Mmh, I'm unable to reproduce this using either the menu or the keyboard shortcut (in the latter case, I get a "Accessibility needs to be enabled to use Caribou"-popup - I though a11y was now always turned on?)

Review of attachment 225095 [details] [review]:

::: js/ui/messageTray.js
@@ +2133,3 @@
+            this._inCtrlAltTab = false;
+        } else {
+            this._lightbox.hide();

lightbox.hide is a no-op, so move it outside the "in ctrl alt tab" case. It doesn't really make sense to tie "in ctrl alt tab" to "inside overview".

Review of attachment 225079 [details] [review]:

Sure.

Review of attachment 225096 [details] [review]:

Sure.

Review of attachment 225086 [details] [review]:

Sure.

Review of attachment 225083 [details] [review]:

Makes sense to me.

Review of attachment 225081 [details] [review]:

I'd prefer something that used the return value of handleSummaryClick.

(In reply to comment #45)
> ::: js/ui/messageTray.js
> @@ +2133,3 @@
> +            this._inCtrlAltTab = false;
> +        } else {
> +            this._lightbox.hide();
> 
> lightbox.hide is a no-op, so move it outside the "in ctrl alt tab" case.

In theory yes. In practice it's what I used first, and it breaks the "tray" button ...

(In reply to comment #50)
> Review of attachment 225081 [details] [review]:
> 
> I'd prefer something that used the return value of handleSummaryClick.

Giovanni did that in his original patch by returning *two* booleans from handleSummaryClick - sorry, but I really hate that solution; while return-true-if-handled is a commonly used convention, that oddity requires you to jump to the place where the function is actually used to figure out what the hack those return values are supposed to mean, which breaks the flow unnecessarily (not to mention that the 2nd value may be completely ignored depending on the 1st one, which does not really add to clarity).
So while I'm not a big fan of the additional property myself, I very much prefer it to the double return value ...

(In reply to comment #52)
> (In reply to comment #50)
> > Review of attachment 225081 [details] [review] [details]:
> > 
> > I'd prefer something that used the return value of handleSummaryClick.
> 
> Giovanni did that in his original patch by returning *two* booleans from
> handleSummaryClick - sorry, but I really hate that solution; while
> return-true-if-handled is a commonly used convention, that oddity requires you
> to jump to the place where the function is actually used to figure out what the
> hack those return values are supposed to mean, which breaks the flow
> unnecessarily (not to mention that the 2nd value may be completely ignored
> depending on the 1st one, which does not really add to clarity).
> So while I'm not a big fan of the additional property myself, I very much
> prefer it to the double return value ...

Perhaps we could do something like DndResult?

    MessageTray.ClickResponse.CLOSE_TRAY
    MessageTray.ClickResponse.KEEP_TRAY_OPEN
    MessageTray.ClickResponse.SHOW_NOTIFICATION_STACK

(In reply to comment #53)
> Perhaps we could do something like DndResult?
> 
>     MessageTray.ClickResponse.CLOSE_TRAY
>     MessageTray.ClickResponse.KEEP_TRAY_OPEN
>     MessageTray.ClickResponse.SHOW_NOTIFICATION_STACK

Sure, but not before branching. Officially we have about 15 minutes left to roll the stable tarball (yes, we'll miss that deadline again), which is clearly the wrong time for refactoring - at this point I'd like to keep code changes as much as possible to the parts that require fixing and not throw away tested and working code just because the more unintrusive fix is less elegant than a more intrusive approach.

don't worry too much about the deadline, I already know of some tarballs happening tomorrow.

but I agree with the general sentiment, of course.

(In reply to comment #44)
> (In reply to comment #39)
> > Here is the first serious issue I've found: turning the osk on, then off, and
> > then on again gives me a shell crash.
> 
> Mmh, I'm unable to reproduce this using either the menu or the keyboard
> shortcut (in the latter case, I get a "Accessibility needs to be enabled to use
> Caribou"-popup - I though a11y was now always turned on?)

Hmm, it actually looks like the crash is just more of that need == need
intel mesa issue; that was supposed to be fixed:

+ Trace 230900

Attachment 225079 [details] pushed as 6611d63 - messageTray: Only update keyboardVisible as necessary
Attachment 225080 [details] pushed as 2e63709 - grabHelper: Ignore events from On-Screen-Keyboard
Attachment 225081 [details] pushed as 2ed7ee8 - keyboard: Keep tray after clicking summary item
Attachment 225082 [details] pushed as 6c1bd95 - keyboard: Ignore focus changes caused by tray showing/hiding
Attachment 225083 [details] pushed as f8ce788 - keyboard: Fix check for extended keys
Attachment 225086 [details] pushed as ef9f63f - keyboard: Ignore focus changes from extended keys
Attachment 225095 [details] pushed as fe124e6 - Keyboard: update for the message tray changes
Attachment 225096 [details] pushed as 18eedbc - keyboard: Disable "tray" button in lock/login screen

(Tentatively leaving open for post-branching cleanups)

moving off blocker list

Should this be closed, then ? I don't expect post-branching cleanups to happen, ever...

as predicted...