Created attachment 245138 [details] [review]
dnd: Use pushModal() to grab the keyboard

Currently we "only" grab the keyboard when starting a drag operation,
which does not impede keybindings to be processed. This is at best
not harmful (like workspace switching), but may have unintended effects
otherwise - for instance, the hot corner is disabled, so having the
corresponding keyboard shortcut still active is fairly odd (not to
mention that it leaves the system in a confused state).
Fix this by switching to pushModal()/popModal(), which will push a
dedicated keybinding mode for us.

(In reply to comment #1)
> Created an attachment (id=245138) [details] [review]
> dnd: Use pushModal() to grab the keyboard

Works great here (though instead of cancelling the drag, the keybinding is ignored, which is fine).

(In reply to comment #2)
> (though instead of cancelling the drag, the keybinding is ignored, which is 
> fine).

Yeah, I'm not really comfortable to deal with any keybindings that might go wrong on a case-by-case basis (lock screen etc.). In fact, the only keybinding that I can think of that genuinely would still make sense are the screenshot ones - those are disabled by the patch as well, though we could fix it easily by adding an explicit DRAG_OPERATION mode if deemed important.

Created attachment 245150 [details]
WIP: Cancel any ongoing drag when the overview is hidden

Just to illustrate another possibility.

Ignoring all key bindings while a drag is active will break power user features such as switching workspaces or using alt-tab during dnd ?

If true, don't think thats acceptable. It'll get us another round of 'you removed my favourite feature' cries.

(In reply to comment #5)
> Ignoring all key bindings while a drag is active will break power user features
> such as switching workspaces or using alt-tab during dnd ?

Yes, though we could use a dedicated keybinding mode to allow some keybindings. Note however that we are strictly talking about dnd in the shell, e.g. extensions aside, it's an overview-only change - alt-tab is already disabled there anyway, and I'd argue that dropping on the workspace switcher is easier than fiddling with keybindings during the operation.

(In reply to comment #6)
> (In reply to comment #5)

> Note however that we are strictly talking about dnd in the shell, e.g.
> extensions aside, it's an overview-only change - alt-tab is already disabled
> there anyway, and I'd argue that dropping on the workspace switcher is easier
> than fiddling with keybindings during the operation.

Ah, ok. Ignore my comment then. It is probably correct to prevent application drags in the overview from escaping out of the overview. Just keep keynav working for people wanting to drag links from their browser to their email client, etc.

Review of attachment 245138 [details] [review]:

::: js/ui/dnd.js
@@ +149,3 @@
         if (!this._eventsGrabbed) {
             Clutter.grab_pointer(_getEventHandlerActor());
+            this._eventsGrabbed = Main.pushModal(_getEventHandlerActor());

I can't think how this could fail right now, but if it does then a call to _ungraEvents() won't undo the clutter pointer grab. I'd conditionalize the Clutter.grab_pointer() here on the success of pushModal().

Created attachment 246080 [details] [review]
dnd: Use pushModal() to grab the keyboard

Can't think of anything concrete that might go wrong either (e.g. all situations where pushModal() may fail should prevent DND), but sure, nothing wrong with playing it safe ...

Review of attachment 246080 [details] [review]:

ok

Attachment 246080 [details] pushed as 9e56e66 - dnd: Use pushModal() to grab the keyboard