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

Add drag-and-drop support. #7150

Merged
merged 13 commits into from Jun 4, 2021
Merged

Conversation

danpark-salesforce
Copy link
Contributor

This PR adds drag-and-drop support, leveraging new additions to the CDP Input domain (Input.setInterceptDrags, Input.dispatchDragEvent, and Input.dragIntercepted).

Fixes #1376

@google-cla google-cla bot added the cla: yes label Apr 24, 2021
@danpark-salesforce
Copy link
Contributor Author

Hello! I've been trying to test drag-and-drop at work using a variety of E2E tools like Cypress and Webdriver. Unfortunately like everyone else I kept running into a frustrating issue where the thing getting dragged would just stick to my mouse cursor instead of getting dropped at a given location.

Luckily, it turns out that Chromium recently added some new hooks for dragging and I'm happy to report they work great!

I know this PR is a bit light on the tests, but I wanted to make sure this was going in the right direction before I spent more time. Looking forward to your feedback, thanks!

@@ -0,0 +1,47 @@
/**
* Copyright 2018 Google Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright 2018 Google Inc. All rights reserved.
* Copyright 2021 Google Inc. All rights reserved.

@mathiasbynens
Copy link
Member

@jschfflr Could you PTAL (and consider the record/replay perspective)?

Copy link
Contributor

@jschfflr jschfflr left a comment

Choose a reason for hiding this comment

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

First of all, the general direction of this change looks good to me. Thanks for taking the time to create this patch!
I just have some small questions:
I'm not sure why you chose to create a new CDP Session for the drag events - could you elaborate on that or just use the already existing one?
Also, there is the optional modifiers flag - could you make sure that the current set of modifiers from the keyboard is respected?
For the return value of the drag: WDYT about returning the DragData directly (instead of the event)?

@danpark-salesforce
Copy link
Contributor Author

danpark-salesforce commented Apr 26, 2021

Thanks @jschfflr for taking a look! Updated the PR based on your feedback.

RE: I'm not sure why you chose to create a new CDP Session for the drag events - could you elaborate on that..?

This was for isolating the drag interception in case something else was also listening for drag events on the existing client. If this isn't a concern, I can use the existing client.

On a different note, the drop() method is currently dispatching three drag events in sequence (dragEnter, dragOver, and drop). I was thinking it might be better to move the dragEnter and dragOver into separate methods. What are your thoughts?

@jschfflr
Copy link
Contributor

Thanks for updating the patch!
I'd prefer to reuse the existing client but you have a good point about the isolation.
WDYT about approaching this in a similar way as the network interception by exposing a page.setDragInterception(true)?

If you feel like the drop events would also be useful on their own, feel free to also expose them.
But I'd prefer to have a single drop method because developers might not know about the specific event order for drag'n'drop events.
Also: WDYT about exposing a single dragAndDrop function that takes two selectors and drags one of them into the other?

@danpark-salesforce
Copy link
Contributor Author

danpark-salesforce commented Apr 28, 2021

Updated and moved the drag interception to the Page class; thanks for pointing me to that.

developers might not know about the specific event order for drag'n'drop events.

Makes total sense, will keep the drop method as is. I'll also add methods for the other drag events in case things like dragover effects need to be tested. (edit: I think adding a delay parameter to the drop() would be simpler).

Also: WDYT about exposing a single dragAndDrop function that takes two selectors and drags one of them into the other?

I think a dragAndDrop function is a good idea. One issue I do see with using selectors is it won't work with web components / shadow DOM. What are your thoughts on passing either selectors or element handles? Also, if we go down this route, we could add the dragAndDrop method directly to the Input as well.

@danpark-salesforce danpark-salesforce force-pushed the drag-and-drop branch 5 times, most recently from e452082 to b292ec0 Compare April 28, 2021 17:27
@jschfflr
Copy link
Contributor

Awesome, thanks!
I think, having individual methods for each event could still be valuable if a user only wants to test one of them.
But the delay parameter sounds super helpful too. Could you add both?

We do have the pierce/ query handler to deal with selectors that use the shadow dom. But I agree with you that using element handles is a better solution. Let's use them instead of the selectors.

@jschfflr
Copy link
Contributor

jschfflr commented May 7, 2021

@danparksf Hi, did you already have time to look into the changes we discussed?
I'd love to get this landed soon :)

@danpark-salesforce
Copy link
Contributor Author

Hi @jschfflr, thanks for the feedback! Been meaning to look into updating this PR and finally have some time to work on them. Should have an update later today, thanks!

@danpark-salesforce
Copy link
Contributor Author

@jschfflr haven't added the dragAndDrop() yet since I wanted to check something with you. In order to drag to a target element, we need to get the x, y coordinates of the target element. At the moment there is a method _clickablePoint() that can do that for us. Are you fine with having a public method to expose it?

@jschfflr
Copy link
Contributor

@danparksf Yes, feel free to mark this function as public!

@jschfflr
Copy link
Contributor

Maybe you can also change the return type and expose a public Point type similar to BoundingBox

@danpark-salesforce danpark-salesforce force-pushed the drag-and-drop branch 2 times, most recently from abe50de to 1385f56 Compare May 17, 2021 04:36
@danpark-salesforce
Copy link
Contributor Author

Thanks for the tips, PR updated!

@jschfflr
Copy link
Contributor

@danparksf Not sure if you are already aware of this, but you can use npm run lint locally to find all the lint errors that will also occur on the bots later.

@danpark-salesforce
Copy link
Contributor Author

Thanks, should be passing the lint check now.

@jschfflr
Copy link
Contributor

@danparksf Thanks! The linter is happy now. Could you also look into the failing unit tests? You can run them locally with npm run unit

This commit adds drag-and-drop support, leveraging new additions to the CDP
Input domain (Input.setInterceptDrags, Input.dispatchDragEvent, and
Input.dragIntercepted).
* Update date in test
* Pass modifiers when dispatching drag events
* Return DragData directly instead of event
* Moved drag interception code to Page
* Added tests for drag interception enablement
* Add separate methods for dragEnter and dragOver
* Fix lint errors
* Add some docs for ElementHandle.drag() and Mouse.drag()
@google-cla
Copy link

google-cla bot commented Jun 4, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 4, 2021
@jschfflr jschfflr enabled auto-merge (squash) June 4, 2021 09:57
@jschfflr
Copy link
Contributor

jschfflr commented Jun 4, 2021

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 4, 2021
@jschfflr
Copy link
Contributor

jschfflr commented Jun 4, 2021

@danparksf Awesome, let's get this landed then :)

@jschfflr jschfflr merged commit 48954c6 into puppeteer:main Jun 4, 2021
mathiasbynens pushed a commit that referenced this pull request Jun 7, 2021
This commit adds drag-and-drop support, leveraging new additions to the CDP Input domain (Input.setInterceptDrags, Input.dispatchDragEvent, and Input.dragIntercepted).
@jschfflr
Copy link
Contributor

jschfflr commented Jun 8, 2021

@danparksf Congrats on getting this landed!

@danpark-salesforce
Copy link
Contributor Author

Thanks @jschfflr for your help!

@danpark-salesforce danpark-salesforce deleted the drag-and-drop branch June 8, 2021 15:22
mathiasbynens added a commit that referenced this pull request Jul 13, 2021
Having it be a getter is surprising and inconsistent, since since the other `page.is*` APIs are just methods.

Issue: #7150
mathiasbynens added a commit that referenced this pull request Jul 13, 2021
Having it be a getter is surprising and inconsistent, since the other `page.is*` APIs are just methods.

Issue: #7150
mathiasbynens added a commit that referenced this pull request Jul 13, 2021
Having it be a getter is surprising and inconsistent, since the other `page.is*` APIs are just methods.

Issue: #7150
mathiasbynens added a commit that referenced this pull request Jul 13, 2021
Having it be a getter is surprising and inconsistent, since the other `page.is*` APIs are just methods.

Issue: #7150
jschfflr pushed a commit that referenced this pull request Sep 10, 2021
Having it be a getter is surprising and inconsistent, since the other `page.is*` APIs are just methods.

Issue: #7150
This was referenced May 30, 2022
This was referenced May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Drag-and-Drop
3 participants