Created attachment 228801 [details] [review]
modalDialog: Also block key-press events for button actions

As we handle actions on key-release, we currently allow other listeners
to process the same keys on key-press for something else - for instance,
when opening the run dialog in the overview, Escape will dismiss the
dialog on release, while the corresponding press will close the
overview.
Keep handling actions on key-release, but make sure to eat the
corresponding key-press as well to avoid those side effects.

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

Created attachment 229654 [details] [review]
modalDialog: Also block key-press events for button actions

Rebased to master

Review of attachment 229654 [details] [review]:

Why are events reaching anything below the modal dialog? Shouldn't we have something like

this._group.connect('event', function() { return true; });

when !this._shellReactive ? Or maybe even guaranteeing that should be pushModal()'s job?

Created attachment 229909 [details] [review]
viewSelector: Ignore key-presses during non-overview modals

Since commit 0c807bddaf4d, the run dialog no longer handles Escape
key presses itself but uses the action key mechanism of modal dialogs.
As the latter uses key-release events, our own handling of the Escape
key runs on key-press.
Fix this by bailing out early if anything has pushed a modal in addition
to the overview (like system modals, looking glass, ...).


(In reply to comment #4)
> Why are events reaching anything below the modal dialog?

Because it's hard I guess - also in the case of key presses, there's not really a "below" :-)

(in the case of modal dialogs, the (normal) actor hierarchy is stage->Main.uiGroup->modalDialog, e.g. not too many possibilities to hook into event bubbling)


> Shouldn't we have something like
> 
> this._group.connect('event', function() { return true; });
> 
> when !this._shellReactive ?

Well, that would fix the problem at hand, but introduce different ones. Most notably that button keys are no longer handled at all (something like

this._group.connect('event',
                    function(actor, event) {
                        return event.type() != Clutter.EventType.KEY_RELEASE;
                    });

should work, but meh ...)


> Or maybe even guaranteeing that should be pushModal()'s job?

I don't see how that would be done, except by using Clutter.grab_keyboard() instead of global.stage.set_key_focus(), which is discouraged by the Clutter documentation and would break all over the place (e.g. everywhere where we expect some actor not directly passed to Main.pushModal() to receive keyboard events, e.g. the overview's search entry, entries in system modals / notifications, ...)


Anyway, here's an alternative approach ...

Review of attachment 229909 [details] [review]:

Right, this isn't as easy as I thought. Anyway I like this one better, let's get this fixed.

Attachment 229909 [details] pushed as 2a5eed1 - viewSelector: Ignore key-presses during non-overview modals