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

chore: identify largest gaps in Bidi API #32434

Merged
merged 6 commits into from
Sep 4, 2024
Merged

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Sep 3, 2024

This pull request introduces initial support for the WebDriver BiDi protocol in Playwright. The primary goal of this PR is not to fully implement BiDi but to experiment with the current state of the specification and its implementation. We aim to identify the biggest gaps and challenges that need to be addressed before considering BiDi as the main protocol for Playwright.

This comment has been minimized.

@yury-s yury-s marked this pull request as ready for review September 3, 2024 19:25

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@@ -0,0 +1,95 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to avoid duping this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to keep the config separate from other browsers as there are too many vailing tests in bidi and it's unclear when the spec/implementation will get to the state where we could make the tests pass.

packages/protocol/src/protocol.yml Outdated Show resolved Hide resolved
packages/playwright-core/src/server/registry/index.ts Outdated Show resolved Hide resolved
@@ -98,6 +98,8 @@ export interface PageDelegate {
resetForReuse(): Promise<void>;
// WebKit hack.
shouldToggleStyleSheetToSyncAnimations(): boolean;
// Bidi throws on attempt to document.open() in utility context.
useMainWorldForSetContent?(): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should make these shortcuts in shared classes. If you are looking for the ways to unblock the tests, maybe it makes sense to move this into the delegate? Can be a common base class or a utility for chromium/webkit/firefox.

packages/playwright-core/src/server/page.ts Show resolved Hide resolved
packages/playwright-core/src/server/bidi/bidiPage.ts Outdated Show resolved Hide resolved
packages/playwright-core/src/server/bidi/bidiPage.ts Outdated Show resolved Hide resolved
packages/playwright-core/src/server/bidi/bidiPage.ts Outdated Show resolved Hide resolved
await this._performActions([{ type: 'pointerUp', button: toBidiButton(button) }]);
}

async click(x: number, y: number, options: { delay?: number, button?: types.MouseButton, clickCount?: number } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Do the same to the other ports to have click everywhere?

tests/library/browser.spec.ts Outdated Show resolved Hide resolved
@@ -216,6 +217,8 @@ export class Mouse {
async click(x: number, y: number, options: { delay?: number, button?: types.MouseButton, clickCount?: number } = {}, metadata?: CallMetadata) {
if (metadata)
metadata.point = { x: this._x, y: this._y };
if (this._raw.click)
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 add a test that page.mouse.down + page.mouse.up generate click and disable it for bidi.

if (object.type === 'event') {
// Route page events to the right session.
let context;
if ('context' in object.params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having a single browser-wide session, and choose a page/context/frame in each event handler separately.

packages/playwright-core/src/server/bidi/bidiPage.ts Outdated Show resolved Hide resolved
this._page._frameManager.frameRequestedNavigation(frameId, params.navigation!);

const url = params.url.toLowerCase();
if (url.startsWith('file:') || url.startsWith('data:') || url === 'about:blank') {
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 add a test for a navigation to blob url.


async getContentFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
const executionContext = toBidiExecutionContext(handle._context);
const contentWindow = await executionContext.rawCallFunction('e => e.contentWindow', { handle: handle._objectId });
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 add a test that this works for cross-process iframes.

packages/playwright-core/src/server/bidi/bidiPage.ts Outdated Show resolved Hide resolved
if (!parent)
throw new Error('Frame has been detached.');
const parentContext = await parent._mainContext();
const list = await parentContext.evaluateHandle(() => { return [...document.querySelectorAll('iframe,frame')]; });
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 add a test for getFrameElement() where iframe is inside shadow dom.

This pull request introduces initial support for the WebDriver BiDi protocol in Playwright. The primary goal of this PR is not to fully implement BiDi but to experiment with the current state of the specification and its implementation. We aim to identify the biggest gaps and challenges that need to be addressed before considering BiDi as the main protocol for Playwright.
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Test results for "tests 1"

1 failed
❌ [playwright-test] › babel.spec.ts:135:5 › should not transform external

2 flaky ⚠️ [chromium-library] › library/trace-viewer.spec.ts:248:1 › should have network requests
⚠️ [playwright-test] › ui-mode-test-setup.spec.ts:22:5 › should run global setup and teardown

29536 passed, 637 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s merged commit 9a2c60a into microsoft:main Sep 4, 2024
29 of 30 checks passed
@yury-s yury-s deleted the chore-bidi branch September 4, 2024 18:36
yury-s added a commit to yury-s/playwright that referenced this pull request Sep 6, 2024
yury-s added a commit that referenced this pull request Sep 6, 2024
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.

3 participants