w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
336 stars 35 forks source link

Emit BrowsingContext (frame) `name` on `BrowsingContext.contextCreated` event #634

Closed Lightning00Blade closed 3 months ago

Lightning00Blade commented 6 months ago

In CDP when there is a new Frame created it's possible to collect the data about it's element attribute name or id. Ref: Page.Frame.name

The proposed solution would be add a property on the browsingContext.contextCreated return type called name (or identifier), that will be text / null of either the frame's element attribute id or name.

Example of such scenarios where this is useful:

  1. Open Top-level Browsing Context
  2. Top-level Browsing Context attaches 2 iframes simultaneously
    • Iframe of page A with name="A"
    • Iframe of page B with name="B"
  3. Try to get frame for A

This will expose only the initial name on creation as after that the attribute may change by scripts or script evaluation. For the up to date users use script evaluation.

whimboo commented 6 months ago

@Lightning00Blade if id and name are given which should be prioritized? What if both have different values? How does the client know if it's the id or name? Maybe both would be needed?

sadym-chromium commented 6 months ago

I can see a point of this not only when the browsing context is created, but when the getTree is called.

Lightning00Blade commented 6 months ago

To answers Henrik question, in CDP the name is always top priority, then it's the id. and

<iframe name="TheFrameName" id="TheFrameId"></iframe>
Frame.name => "TheFrameName"

CDP does not provide a way to differentiate if it's a name or id.

sadym-chromium commented 5 months ago

To answers Henrik question, in CDP the name is always top priority, then it's the id. and

<iframe name="TheFrameName" id="TheFrameId"></iframe>
Frame.name => "TheFrameName"

CDP does not provide a way to differentiate if it's a name or id.

Can we extend CDP with it? It seems meaningful.

css-meeting-bot commented 5 months ago

The Browser Testing and Tools Working Group just discussed Frame name.

The full IRC log of that discussion <jgraham> Topic: Frame name
<jgraham> github: https://github.com/w3c/webdriver-bidi/issues/634
<jgraham> lightning00blade: We want to extend browsingContext.created or BrowsingContextInfo with name and or id from the attributes of an iframe; this is used in Puppeteer.
<MaksimSadym> q+
<jgraham> jgraham: That sounds fine, I don't know any reason not to do that.
<jgraham> ack MaksimSadym
<jgraham> MaksimSadym: Only concern is what to do if they change, it's probably better to add them in the BrowsingContextInfo
<jgraham> lightning00blade: Puppeteer only updates the property once. You want to know which frame corresponds to which element created. Getting the newest version isn't supported.
<jgraham> MaksimSadym: Maybe then it makes sense to keep it only in the event, since the getTree data could be outdated.
<jgraham> lightning00blade: A user can get the latest values via script evaluation.
<jgraham> jgraham: To me it makes more sense to just put it in BrowsingContextInfo; if Puppeteer wants to ignore that data it's fine, but it means we're not special casing the specific use case.
<jgraham> q?
OrKoN commented 4 months ago

I have been thinking about this use case again and I wonder if Puppeteer's API actually makes sense here:

1) it only gives access to the name/ID set at the iframe creation time => so might not match the current DOM state. 2) it does not differentiate between ID and name. 3) it is also possible to achieve similar results using locators OR script evaluation. 4) it is only applicable to iframes and not top-level browsing contexts.

So if we include it in the WebDriver BiDi spec, we could solve the issue No 2 by listing the fields (ID, name) separately but the issue No 1, 3 and 4 remain since the browsingContextInfo is not emitted if DOM changes and for implementations (e.g., CDP) it might be not very straight-forward to implement the sync of the DOM attributes with the browsing context tree. I would suggest that we consider deprecating this API and instead directing users towards the locator/selector API in Puppeteer? cc @Lightning00Blade @sadym-chromium

whimboo commented 3 months ago

@Lightning00Blade and @sadym-chromium any feedback from you regarding the suggestions from @OrKoN? Quite a few tests are failing because of this issue. Thanks!

sadym-chromium commented 3 months ago

I filed a feature request in puppeteer to deprecate Frame.name(). @Lightning00Blade consider closing this issue.

whimboo commented 3 months ago

Given that https://github.com/puppeteer/puppeteer/pull/12084 is merged and already released I assume we can close this issue? @sadym-chromium @Lightning00Blade

sadym-chromium commented 3 months ago

Given that puppeteer/puppeteer#12084 is merged and already released I assume we can close this issue? @sadym-chromium @Lightning00Blade

sure!