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(codegen): basic drag'n drop support #27985

Closed
wants to merge 1 commit into from

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Nov 6, 2023

This comment has been minimized.

@@ -349,6 +372,8 @@ class RecordActionTool implements RecorderTool {
return true;
if (nodeName === 'INPUT' && ['date'].includes((target as HTMLInputElement).type))
return true;
if (target.draggable && nodeName !== 'A')
Copy link
Member Author

Choose a reason for hiding this comment

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

this check requires further thinking. It made these tests fail:

[chromium]  library/inspector/cli-codegen-1.spec.ts:653:7  cli codegen  should await popup ──
[chromium]  library/inspector/cli-codegen-2.spec.ts:200:7  cli codegen  should download files 

images/links with a src/href are draggable by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave it a try and cherry-picked your code on top of my PR #28767, and those tests pass. You can take a look at:

https://github.com/ruifigueira/playwright/tree/feat/record-drag-drop

Keep in mind that I changed the way recorder works: it no longer consumes events and performs actions via playwright while recording. This makes the recording experience more natural and, at least until now, I didn't notice any strange behaviour because of those changes (all tests are passing, as you can check in my PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Meanwhile I reverted most of the changes in the PR (it could be solved with a one-liner), but that branch still has them.

Copy link
Contributor

github-actions bot commented Nov 6, 2023

Test results for "tests 1"

15 flaky ⚠️ [chromium] › library/trace-viewer.spec.ts:1012:1 › should pick locator in iframe
⚠️ [firefox] › page/page-goto.spec.ts:81:3 › should work with Cross-Origin-Opener-Policy
⚠️ [chromium] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [chromium] › components/splitView.spec.tsx:65:5 › drag resize
⚠️ [webkit] › library/browsercontext-proxy.spec.ts:134:11 › should proxy local network requests › with other bypasses › link-local
⚠️ [webkit] › library/browsercontext-storage-state.spec.ts:51:3 › should set local storage
⚠️ [webkit] › library/browsercontext-storage-state.spec.ts:166:3 › should not restore localStorage twice
⚠️ [webkit] › library/download.spec.ts:342:5 › download event › should delete file
⚠️ [webkit] › library/har.spec.ts:595:3 › should have connection details
⚠️ [webkit] › page/page-goto.spec.ts:290:3 › should not throw if networkidle0 is passed as an option
⚠️ [webkit] › page/page-wait-for-navigation.spec.ts:85:3 › should work with clicking on links which do not commit navigation
⚠️ [playwright-test] › ui-mode-test-ct.spec.ts:55:5 › should run component tests after editing test
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots

26060 passed, 609 skipped
✔️✔️✔️

Merge workflow run.

@mxschmitt mxschmitt marked this pull request as draft November 13, 2023 10:21
@mxschmitt
Copy link
Member Author

Action items:

  • test on real world drag'n drop scenarios
  • Look into getting rid of the weird if

@AnshPatel15
Copy link

Could I ask how long it would take for this functionality to be added? I have asked before but I really need this to work on my application.

@chensce
Copy link

chensce commented Nov 15, 2023

We also really need this to work.

@chensce
Copy link

chensce commented Nov 24, 2023

@mxschmitt ,we can reference the rrweb(https://www.rrweb.io/) about drag'n drop recording

@mxschmitt
Copy link
Member Author

Closing for now, since this was an experiment and it has proven that it was not that trivial to implement. We might re-visit this feature request in the future and unfortunately can't commit on any eta.

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.

4 participants