Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable macOS animation when drag operation cannot be completed #199635

Open
deepak1556 opened this issue Nov 30, 2023 · 10 comments
Open

Disable macOS animation when drag operation cannot be completed #199635

deepak1556 opened this issue Nov 30, 2023 · 10 comments
Assignees
Labels
chromium Issues and items related to Chromium macos Issues with VS Code on MAC/OS X upstream Issue identified as 'upstream' component related (exists outside of VS Code) workbench-auxwindow Issues related to use of auxiliary ("floating") windows. workbench-dnd Drag-and-drop issues

Comments

@deepak1556
Copy link
Collaborator

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,

Screen.Recording.2023-11-30.at.15.38.22.mov

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
Screen.Recording.2023-11-30.at.15.48.03.mov

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

@deepak1556 deepak1556 added upstream Issue identified as 'upstream' component related (exists outside of VS Code) macos Issues with VS Code on MAC/OS X workbench-dnd Drag-and-drop issues chromium Issues and items related to Chromium labels Nov 30, 2023
@deepak1556 deepak1556 added this to the December 2023 milestone Nov 30, 2023
@deepak1556 deepak1556 self-assigned this Nov 30, 2023
@bpasero bpasero added the workbench-auxwindow Issues related to use of auxiliary ("floating") windows. label Nov 30, 2023
@bpasero
Copy link
Member

bpasero commented Nov 30, 2023

👍 , 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
Copy link
Collaborator Author

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
Copy link

zcbenz commented Nov 30, 2023

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
Copy link
Member

bpasero commented Nov 30, 2023

@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
Copy link

zcbenz commented Nov 30, 2023

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
Copy link

zcbenz commented Nov 30, 2023

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
Copy link
Member

bpasero commented Nov 30, 2023

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
Copy link

zcbenz commented Dec 1, 2023

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
Copy link
Member

bpasero commented Dec 1, 2023

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
Copy link

zcbenz commented Dec 1, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromium Issues and items related to Chromium macos Issues with VS Code on MAC/OS X upstream Issue identified as 'upstream' component related (exists outside of VS Code) workbench-auxwindow Issues related to use of auxiliary ("floating") windows. workbench-dnd Drag-and-drop issues
Projects
None yet
Development

No branches or pull requests

3 participants