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

feat(chromium): basic drag and drop support #4519

Closed
wants to merge 1 commit into from

Conversation

JoelEinbinder
Copy link
Contributor

This does drag and drop for chromium in a very hacky way all in the client. It covers the fast majority of use cases. We can remove the majority of the code by adding a CDP method to attack drag data to the mouse and another to intercept the dragstart. The chromium patches involve a lot of tricky IPC races/security concerns, and so this hacky patch lets us take our time over there.

#1094

closes #4508

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Could you please split this into 3 PRs: internal binding, new tests and implementation?

await Promise.all(this._page.frames().map(frame => frame._evaluateExpression(binding.source, false, {}).catch(e => {})));
}

async exposeInternalBinding(name: string, callback: frames.FunctionWithSource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this to the common Page class.

@@ -0,0 +1,115 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a part of InjectedScript, but install the dragstart handler lazily as you do now.

@@ -127,8 +127,8 @@ describe('Drag and drop', test => {
'mousedown',
isFirefox ? 'dragstart' : 'mousemove',
isFirefox ? 'mousemove' : 'dragstart',
'dragenter',
'dragover',
// 'dragenter',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's up with these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants