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

fix: add cdp.resolveRealm command #1882

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Conversation

jrandolf-2
Copy link
Contributor

@jrandolf-2 jrandolf-2 commented Feb 22, 2024

cdp.resolveRealm resolves a realm ID to an executionContextId.

This is needed for Puppeteer to adopt nodes from ARIA queries.

This is a workaround for https://crbug.com/326258571

@jrandolf-2 jrandolf-2 changed the title chore: emit custom property for execution context ID fix: emit custom property for execution context ID Feb 22, 2024
@OrKoN
Copy link
Collaborator

OrKoN commented Feb 22, 2024

I wonder if instead we should accept realm in BiDi+ and automatically replace it with an execution context id. @sadym-chromium WDYT?

@jrandolf-2
Copy link
Contributor Author

I wonder if instead we should accept realm in BiDi+ and automatically replace it with an execution context id. @sadym-chromium WDYT?

I think that's pretty clever. +1

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 22, 2024

Please update title/description

@jrandolf-2 jrandolf-2 changed the title fix: emit custom property for execution context ID fix: add BiDi-only property to DOM.resolveNode CDP command Feb 22, 2024
@jrandolf-2 jrandolf-2 force-pushed the jrandolf/execution-context-id branch 3 times, most recently from 37bb9b7 to 0bb7241 Compare February 22, 2024 17:49
@sadym-chromium
Copy link
Collaborator

sadym-chromium commented Feb 23, 2024

@jrandolf can you please elaborate a bit the original issue? Why do you need DOM.resolveNode CDP command in BiDi mode? Maybe you have a Puppeteer github issue link?

@sadym-chromium
Copy link
Collaborator

sadym-chromium commented Feb 23, 2024

realm in BiDi+

If Puppeteer in BiDi needs to be aware of the CDP executionContextId, we can add a BiDi+ command cdp.resolveRealm:

CdpResolveRealmCommand = {
   method: "cdp.resolveRealm",
   params: CdpResolveRealmParameters,
}

CdpResolveRealmParameters = {
   bidiRealm: script.Realm,
}

CdpResolveRealmResult = {
   cdpExecutionContextId: text,
}

We already have CDP.getSession for the very same reason.

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 23, 2024

@sadym-chromium it’d be nice to avoid many round trips to support a11y queries over bidi+

@sadym-chromium
Copy link
Collaborator

sadym-chromium commented Feb 23, 2024

@sadym-chromium it’d be nice to avoid many round trips to support a11y queries over bidi+

In case of puppeteer + mapper, this will be an internal call, without a roundtrip to the browser. But I'm not sure what is the specific scenario this is required to

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 23, 2024

@sadym-chromium it would be more problematic when if we start supporting chromedriver. Also, it's not always desirable to make an async call (due to potential races). I don't have a strong preference for either way.

getSession was already a bit problematic since we need to have it at the time of the Frame/Page construction and that is synchronous. Currently, we trigger it in the constructor without awaiting so it could be prone to flakiness.

@jrandolf-2
Copy link
Contributor Author

@sadym-chromium it’d be nice to avoid many round trips to support a11y queries over bidi+

In case of puppeteer + mapper, this will be an internal call, without a roundtrip to the browser. But I'm not sure what is the specific scenario this is required to

When an A11y query is performed in Puppeteer, we get a backendNodeId. If the query is performed in a sandboxed realm, we need to DOM.resolveNode against the sandboxed realm instead of the default one.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/execution-context-id branch 2 times, most recently from 7a8a02f to e4c32bc Compare February 23, 2024 12:49
@jrandolf-2 jrandolf-2 changed the title fix: add BiDi-only property to DOM.resolveNode CDP command fix: add cdp.resolveNode command Feb 23, 2024
@jrandolf-2 jrandolf-2 changed the title fix: add cdp.resolveNode command fix: add cdp.resolveRealm command Feb 23, 2024
@jrandolf-2 jrandolf-2 added the puppeteer Run Puppeteer test when added to PR label Feb 23, 2024
Copy link
Collaborator

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

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

LGTM, but please:

  1. add an e2e test
  2. update readme with this command

@jrandolf-2 jrandolf-2 enabled auto-merge (squash) February 23, 2024 13:01
Copy link
Collaborator

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

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

Thanks!

@jrandolf-2 jrandolf-2 merged commit 08d3e45 into main Feb 23, 2024
34 checks passed
@jrandolf-2 jrandolf-2 deleted the jrandolf/execution-context-id branch February 23, 2024 13:13
jrandolf-2 pushed a commit that referenced this pull request Feb 23, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.5.11](chromium-bidi-v0.5.10...chromium-bidi-v0.5.11)
(2024-02-23)


### Bug Fixes

* add `cdp.resolveRealm` command
([#1882](#1882))
([08d3e45](08d3e45))
* use better error handling
([#1877](#1877))
([33536e0](33536e0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
puppeteer Run Puppeteer test when added to PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants