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.

Sign up for GitHub

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

Jump to bottom

fix(chromium): drag and drop works in chromium #6207

Merged
merged 1 commit into from
Jun 7, 2021
Merged

fix(chromium): drag and drop works in chromium #6207

merged 1 commit into from
Jun 7, 2021

Conversation

JoelEinbinder
Copy link
Contributor

Waiting for #6203 to percolate to the cdn. But this all works locally.

Fixes #1094

dgozman
dgozman reviewed Apr 16, 2021
View reviewed changes
src/server/chromium/crDragDrop.ts Outdated Show resolved Hide resolved
src/server/chromium/crDragDrop.ts Outdated
return false;
await this._page._mainFrameSession._client.send('Input.dispatchDragEvent', {
type: 'dragCancel',
x: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we store last drag position to pass here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasonable! I was going based off of Firefox, which has (0,0) for the position on all dragend events. But Chromium does report the position of dragend.

src/server/chromium/crDragDrop.ts Outdated
});
}).toString(), true, undefined, 'utility');
// don't resolve on errors. This could be replaced by Promise.any after Node v15
dragSignalPromises.push(promise.catch(e => new Promise(() => {})));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not resolve on errors? What if we have just a single frame and is has just navigated under our feet? We'll stall forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't stall because these are used with Promise.race

src/server/chromium/crDragDrop.ts Outdated Show resolved Hide resolved
@JoelEinbinder JoelEinbinder force-pushed the unhack_drag_drop_2 branch 2 times, most recently from fe2b625 to 3a1bb81 Compare April 20, 2021 06:42
@JoelEinbinder JoelEinbinder marked this pull request as ready for review May 7, 2021 15:06
@JoelEinbinder
Copy link
Contributor Author

Ready for review!

dgozman
dgozman reviewed May 7, 2021
View reviewed changes
src/server/chromium/protocol.ts Outdated Show resolved Hide resolved
tests/page/page-drag.spec.ts Outdated
it.skip(({ isAndroid }) => isAndroid);
it.fixme(({ browserName }) => browserName === 'chromium');
it.skip(({isAndroid}) => isAndroid);
it.skip(({isChromium, browserMajorVersion}) => isChromium && browserMajorVersion < 91);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was under impression that we are waiting for the stable to pick up the protocol changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a try-catch in the code.

tests/page/page-drag.spec.ts
await page.goto(server.PREFIX + '/frames/one-frame.html');
await page.mouse.move(30, 30);
await page.mouse.down();
await page.mouse.move(60, 60);
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 do a mouseup just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added an event tracker as well.

src/server/chromium/crDragDrop.ts Outdated
dragSignalPromises.push(promise.catch(() => false));
}
// make sure that all previous evaluate expressions hit the renderer
await this._crPage._page.mainFrame().evaluateExpression('1', false, undefined, 'utility').catch(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to do this for all oopifs, I guess.

src/server/chromium/crDragDrop.ts Outdated

const dragSignalPromises: Promise<boolean>[] = [];
for (const frame of this._crPage._page.frames()) {
const promise = frame.evaluateExpression((function() {
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 you need something like nonStallingRawEvaluateInExistingMainContext her (there is such a function) to avoid stalling mouse move because some frame is perpetually navigating.

@JoelEinbinder JoelEinbinder force-pushed the unhack_drag_drop_2 branch 2 times, most recently from fa05b58 to a4f5a8f Compare May 7, 2021 15:47
@JoelEinbinder JoelEinbinder requested a review from yury-s June 1, 2021 17:09
@YusukeIwaki YusukeIwaki mentioned this pull request Jun 2, 2021
Merged
yury-s
yury-s reviewed Jun 3, 2021
View reviewed changes
tests/page/page-drag.spec.ts
it.skip(({ isAndroid }) => isAndroid);
it.fixme(({ browserName }) => browserName === 'chromium');
it.skip(({isAndroid}) => isAndroid);
it.skip(({browserName, browserMajorVersion}) => browserName === 'chromium' && browserMajorVersion < 91);
Copy link
Member

Choose a reason for hiding this comment

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

91 is current stable, we can drop the version check

src/server/chromium/crInput.ts
});
}

async down(x: number, y: number, button: types.MouseButton, buttons: Set<types.MouseButton>, modifiers: Set<types.KeyboardModifier>, clickCount: number): Promise<void> {
if (this._dragManager.isDragging())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be drop? If I do left button down, drag and then right button click (while holding left button) it will paste the selection.

src/server/chromium/crDragDrop.ts
return frame.evaluateExpression((() => window.__didStartDrag).toString(), true, undefined, 'utility').catch(() => false);
}))).some(x => x);
this._dragState = expectingDrag ? (await dragInterceptedPromise).data : null;
client.off('Input.dragIntercepted', onDragIntercepted!);
Copy link
Member

Choose a reason for hiding this comment

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

Consider removing 'mousemove' listeners too

dgozman
dgozman reviewed Jun 3, 2021
View reviewed changes
src/server/chromium/crDragDrop.ts Outdated
await client.send('Input.setInterceptDrags', {enabled: true});
} catch {
// If Input.setInterceptDrags is not supported, just do a regular move.
// This can be removed once Input.setInterceptDrags lands into Chromium Stable.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be needed for quite a while for Electron, right? Let's update the comment.

src/server/chromium/crDragDrop.ts Outdated
const dragInterceptedPromise = new Promise<Protocol.Input.dragInterceptedPayload>(x => onDragIntercepted = x);

await Promise.all(this._crPage._page.frames().map(async frame => {
await frame.evaluateExpression((function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that awaiting all frames when some oopif could be perpetually navigating is going to stall. Did we figure out the solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go add a test

@klhex klhex mentioned this pull request Jun 5, 2021
Closed
@aslushnikov aslushnikov merged commit 8960584 into microsoft:master Jun 7, 2021
@YusukeIwaki YusukeIwaki mentioned this pull request Jun 8, 2021
Merged
@YusukeIwaki YusukeIwaki mentioned this pull request Jun 16, 2021
Merged
@bsteffensTempus
Copy link

What is the correct way to implement this functionality from within a playwright test?

@Dospios
Copy link

Dospios commented Jul 11, 2021

How use that from tests?

@JoelEinbinder
Copy link
Contributor Author

Today you can just do page.mouse.down, page.mouse.move, and page.mouse.up. Follow #7562 for the high level api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgozman dgozman dgozman left review comments

@yury-s yury-s yury-s left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] drag and drop
6 participants
@JoelEinbinder @bsteffensTempus @Dospios @yury-s @dgozman @aslushnikov