Skip to content

Fix drag and dropping of editors for webviews#82813

Merged
mjbvz merged 4 commits intomicrosoft:masterfrom
mjbvz:webview-drag-drop
Oct 19, 2019
Merged

Fix drag and dropping of editors for webviews#82813
mjbvz merged 4 commits intomicrosoft:masterfrom
mjbvz:webview-drag-drop

Conversation

@mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Oct 18, 2019

Fixes #25854

This change does the following:

  • Have the webview editor create an EditorDropTarget. This is required because the webview itself is not part of the normal editor dom (it exists as a top level node in the workbench so that we never reparent it). This means that the standard EditorDropTarget does not work.

  • Make webviews set pointer-events: none while a drag is happening. When a drag happens on Electron's webviews or a normal iframe, no drag and drop events seem to get generated. The fix is to set pointer-events: none while the drag is happening. But it's also difficult to detect when the drag is happening because the EditorDropTargets class eats the drop event itself. The only reliable seeming way I could find to determine when a drag starts and ends is looking at global events on the window.

This workaround is pretty ugly. Unfortunately I haven't been able to find a better way to do this

@mjbvz mjbvz requested review from bpasero and deepak1556 October 18, 2019 04:27
@mjbvz mjbvz self-assigned this Oct 18, 2019
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjbvz even though maybe you need to use hacks, the result is sweet 👍

My only concern with the approach is how you depend on editor interna to get the drop target to show. Instead, I would make a method on IEditorGroupsService to draw this target if a component needs it. Since this is an internal workbench service anyways, I see no harm in having it.

@mjbvz mjbvz force-pushed the webview-drag-drop branch from c6dc3cc to 1602a94 Compare October 18, 2019 22:52
@mjbvz
Copy link
Collaborator Author

mjbvz commented Oct 18, 2019

@bpasero Thanks for the feedback and good suggestion. I had to add the new method on the EditorPart class instead of on on the IEditorGroupsService interface since we use the HTMLElement type and IEditorGroupsService is under common instead of browser

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjbvz after you addressed my minor feeback I think this can be merged

};
}

createDropTargets(container: HTMLElement, delegate: EditorDropTargetDelegate): IDisposable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjbvz I use folding in that file and would prefer if this new method could move up below private assertGroupView

};
}

createDropTargets(container: HTMLElement, delegate: EditorDropTargetDelegate): IDisposable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjbvz maybe a better name would be createEditorDropTarget

this._webviewVisibleDisposables.clear();

// Webviews are not part of the normal editor dom, so we have to register our own drag and drop handler on them.
this._webviewVisibleDisposables.add((this._editorGroupsService as EditorPart).createDropTargets(input.webview.container, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjbvz I think one step further is to make this new method available from IEditorGroupsService to prevent this ugly cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is possible. IEditorGroupsService is in common so it can't reference HTMLElement. I've replaced the cast with an instanceof check which should be safer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #82968 to follow up.

mjbvz added 4 commits October 19, 2019 09:39
Fixes microsoft#25854

This change does the following:

- Have the webview editor create an `EditorDropTarget`. This is required because the webview itself is not part of the normal editor dom (it exists as a top level node in the workbench so that we never reparent it). This means that the standard `EditorDropTarget` does not work.

- Make webviews set `pointer-events: none` while a drag is happening. When a drag happens on Electron's webviews or a normal iframe, no  drag and drop events seem to get generated. The fix is to set `pointer-events: none` while the drag is happening. But it's also difficult to detect when the drag is happening because the  `EditorDropTargets` class eats the drop event itself. The only reliable seeming way I could find to determine when a drag starts and ends is looking at global events on the window.

This workaround is pretty ugly. I'm not sure if there's some better approach that would work with webviews
Unfortunatly we can't add this method to IEditorGroupsService as it uses the `HTMLElement` type
@mjbvz mjbvz force-pushed the webview-drag-drop branch from 1602a94 to bc544a1 Compare October 19, 2019 16:39
@mjbvz mjbvz merged commit cbda1fd into microsoft:master Oct 19, 2019
@mjbvz mjbvz deleted the webview-drag-drop branch October 19, 2019 16:41
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't drag Markdown preview to start a new editor group

2 participants