microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.47k stars 29.36k forks source link

Disable macOS animation when drag operation cannot be completed #199635

Open deepak1556 opened 11 months ago

deepak1556 commented 11 months ago

With floating windows support in VSCode we now have scenarios to drag an editor tab outside the application without having to complete the drop on external applications. This leads to macOS performing the default slide animation to return the dragged image back to its source location as seen in the recording below,

https://github.com/microsoft/vscode/assets/964386/319441db-aed1-4162-8210-37796d9162c3

This causes an undesirable delay before the new editor window gets created and we would like to avoid that if possible. I was looking around the NSDraggingSession documentation and found the following property animatesToStartingPositionsOnCancelOrFail would allow us to control the behavior. As a quick test, the following patch always disables the animation and the result looks much better,

diff --git a/content/app_shim_remote_cocoa/web_contents_view_cocoa.mm b/content/app_shim_remote_cocoa/web_contents_view_cocoa.mm
index c050c86271d6d..9aba95b1d1b51 100644
--- a/content/app_shim_remote_cocoa/web_contents_view_cocoa.mm
+++ b/content/app_shim_remote_cocoa/web_contents_view_cocoa.mm
@@ -277,9 +277,10 @@ - (void)startDragWithDropData:(const DropData&)dropData
   _dragOperation = operationMask;

   // Run the drag operation.
-  [self beginDraggingSessionWithItems:@[ draggingItem ]
+  auto* drag_session = [self beginDraggingSessionWithItems:@[ draggingItem ]
                                 event:dragEvent
                                source:self];
+  drag_session.animatesToStartingPositionsOnCancelOrFail = NO;
 }

 // NSDraggingSource methods

https://github.com/microsoft/vscode/assets/964386/d07d4521-9441-4d2b-a744-5e17562a8782

Interestingly, I found that firefox did a similar change earlier this year https://hg.mozilla.org/mozilla-central/rev/b2954fa754d2 that disables the animation for tabs dragged out of the window or if the drag operation has an empty data transfer object. I am now thinking how we can upstream this as a proper change to Chromium, in our case we cannot set an empty data transfer object since we need the data when dragging editors between windows of the application and also data transfer object can only be set on drag start and is immutable once set which does not give enough options for us to change the data based on drop destination. Options I can think of,

1) A new property on DataTransfer event.dataTransfer.webkitShowFailAnimation which applications can control

2) Always disable the animation when drop is performed outside the application bounds ? Wondering if there are scenarios that could counter this.

/cc @zcbenz @bpasero

bpasero commented 11 months ago

👍 , would be great to upstream. I guess for Chrome to accept this eventually, it would have to be an opt-in thing to not break existing behaviour (thats just my assumption).

Maybe this could be done on the level of the drag image APIs? For example, when you set a drag image with a HTMLElement that has a new CSS property that is specific to webkit that tells it to not animate. Though that would probably make the patch much larger than just disabling it.

deepak1556 commented 11 months ago

I guess for Chrome to accept this eventually, it would have to be an opt-in thing to not break existing behaviour (thats just my assumption).

Yup that is what I am going for in the Option 1), adding new webkit property on the data transfer object. Borrowed the idea from the firefox patch.

zcbenz commented 11 months ago

If you are good with writing a W3C proposal and then going through the procedure to get it approved, then adding a event.dataTransfer.showFailAnimation property should be the best way. Adding a private webkitShowFailAnimation API is unlikely to be approved in Chromium unless it is Google who wants it.

Have you considered using the startDrag API of Electron? https://www.electronjs.org/docs/latest/api/web-contents#contentsstartdragitem

It is much easier adding an option for animatesToStartingPositionsOnCancelOrFail in Electron's API.

bpasero commented 11 months ago

@zcbenz my understanding is that startDrag API in Electron is for file transfers specifically. In this case we do not want a file data transfer to prevent unwanted side effects (such as a file being created on desktop when you drop a VS Code tab on the desktop).

zcbenz commented 11 months ago

Yeah you are right the API is currently only for files, I forgot that. And this API lacks the ability to handle end of dragging.

I'm mentioning startDrag because I believe this problem can be fixed without involving Chromium. Dragging a thing on macOS is basically calling the beginDraggingSessionWithItems API with a NSDraggingSource, and the NSDraggingSource provides events equivalent to the dragstart/dragend events of DOM. So it is possible to control every detail of the drag and drop experience by writing a native module. (People did this before.)

The downside is, it will make drag and drop code much more complicated when all you need is just animatesToStartingPositionsOnCancelOrFail = NO.

zcbenz commented 11 months ago

Note that the native module itself is easy to write if you only want to do this approach on macOS, the problem is your drag drop code's structure would become something like this:

For Linux/Windows, just do drag and drop in DOM. For macOS, dispatch the drag and drop events to main process, use the native module there, and then dispatch the events from native module back to renderer process.

I can write a demo of the native module if you decide to go this approach.

bpasero commented 11 months ago

Wow, thats an incredibly detailed blog post, very helpful, thanks for sharing!

Thanks for your offer, but I think I would not want to go down the path of a native module, given our ability to disable this animation in our builds easily.

However, there is still one issue on Windows that I am not yet sure if we can solve it: using drag and drop without data transfer means that Windows will signal back that dropping is not possible (showing a red icon). You can see that this is even an issue with Firefox today:

Recording 2023-11-30 at 12 02 18

I was not able to find a workaround using HTML APIs and I wonder if even with Windows it would be possible to trick Windows into believing the drop is possible, even when there is no transfer associated that the target can handle.

My only idea for trying to fix this would be to open a little transparent window in the background that moves with the mouse cursor and always signals back that dropping is possible. However, I think that during drag and drop operations, we are not even getting the mouse position as events back, so it might be hard to get this right.

zcbenz commented 11 months ago

In native code you can set the dataTransfer to a URL so the explorer.exe accepts it, and then cancel the drop when user releases mouse, so explorer.exe would not actually write a URL file. But it is probably not possible with HTML APIs, Chromium seems to always do the drop when mouse is released: https://source.chromium.org/chromium/chromium/src/+/main:ui/base/dragdrop/drag_source_win.cc;l=21-24;drc=8e78783dc1f7007bad46d657c9f332614e240fd8

Also after testing with Chrome and Edge's tab dragging, I believe they are not using system drag drop, and Chromium's tab dragging code seems to confirm so: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/tabs/tab_drag_controller.cc;l=620-629;drc=8e78783dc1f7007bad46d657c9f332614e240fd8 (the code says they only use system drag drop for Wayland)

It should be possible to duplicate Chrome's tab dragging behavior with js code. When user drags a tab in DOM, don't use the HTML drag drop APIs, instead call setPointerCapture to capture mouse mouse movement, create a thumbnail window that moves beside the cursor, and when users releases the mouse create a new window and move the tab into it.

bpasero commented 11 months ago

I had thought about dropping our drag-and-drop solution for a mouse-event only solution but there are downsides: we do allow to drop a tab into an existing window of VS Code to have the tab open in there. For that we do set a data transfer that is specific to VS Code. I am not sure I could replicate this behaviour using mouse-events alone.

But thanks for the pointer, I was not aware of that API yet 🙏

zcbenz commented 11 months ago

Chromiums implements tab dragging between existing windows by getting the window below the cursor when mouse is released: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/tabs/tab_drag_controller.h;l=549-553;drc=8e78783dc1f7007bad46d657c9f332614e240fd8

It does not do data transfer between windows, instead it handles mouse movements directly and operates on windows directly.

Chromium's approach is really complicated and error-prone to implement in an Electron app since there is extra layer of inter process communication, but if you want a perfect tab dragging experience it is probably the practical way to go. It is not hard to add an Electron API to return the BrowserWindow under the cursor.