-
Notifications
You must be signed in to change notification settings - Fork 42
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
Define the browsingContext.create command #133
Conversation
index.bs
Outdated
1. Let |type| be the value of the <code>type</code> field of | ||
|command parameters| if present, or "<code>window</code>" otherwise. | ||
|
||
1. TODO: insert step 5 of https://w3c.github.io/webdriver/#new-window verbatim here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really the main question of this PR, everything I've done is mostly the boilerplate.
https://w3c.github.io/webdriver/#new-window involves the "current browsing context" which we probably want to avoid in BiDi. But without that, presumably we should pass in another browsing context which the new one should be opened after in the tab case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess it makes sense to have an optional context, although absent that there's probably a way to talk about using the browsing context which has, or most recently had, OS-level focus as the relevant context in the tab case.
It occurs to me this should be accompanied by a PR to provide a switch-to-context method c.f. https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-bringToFront
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling CDP's Target.createTarget is a lot like a user opening a new unrelated tab, in which case there's no need for an existing/current browsing context.
It almost feels like we should have two commands; One that invokes the window.open
steps as WebDriver/HTTP's New Window command does, and one that just creates a new tab/window without an opener relationship as Target.createTarget does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, so rather than a type
argument, two commands for these two operations? https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createTarget does have a newWindow
boolean though, so the precedent seems to be a single command, both in CDP and WebDriver. Am I misunderstanding the suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jgraham regarding Page.bringToFront
in CDP, do you think the equivalent in BiDi should be browsingContext.bringToFront
then?
Perhaps we need to make the concept of a group of browsing contexts (tabs) explicit? Playwright has a notion of BrowserContext
which seems to group together their Page
objects. The main reason for it seems to be "independent browser sessions" rather than representing a visible window, but there might be something to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that WebDriver is creating the window with noopener
, so it's really a new tab that's not connected to any other tabs. So I think one command that opens a new top level browsing context in either a tab or a window should be fine.
I think BrowserContext in Playwrihgt is trying to solve a different kind of problem; I think it's an abstraction over the idea of multiple containers (in gecko) or private windows (in Blink), which don't share state. It seems like a reasonable idea to surface that concept, but I don't think it's apping 1:1 with OS windows, and is probably a more advanced feature. It's also not a 1:1 mapping with OS windows, which I think is what you're looking for here.
For my money, a browsingContext.bringToFront
that would take the context parameter, get the associated top-level-browsing context, and do [something] makes sense. The question is whether [something] should require that it ends up with OS-level focus or just that it's selected context in whatever window it's in without requiring anything at the OS level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's put Playwright's BrowserContext
to the side for now.
For the purposes of this PR, my main question is if we want a browsingContext.create
command to work without any reference to which window it should open the new browsing context in. The slightly unsatisfying bit is going to be if we add a notion of windows later, there would have to be an implicit window here and we've tried to avoid things like WebDriver's "current top-level browsing context" in BiDi so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my proposal from #133 (comment) still makes sense: if you provide a context, the new context should be created as if the user was creating a new context starting at that one (that's hard to specify in detail because to some extent it's an implementation detail, but roughly it would mean a new tab would share an os window with the tab containing that context; I think the text will be necessarily handwavy here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left this as an inline issue in the spec now, reading:
HTML’s window open steps depend on the "entry global object" and WebDriver’s New Window command depends on the "current browsing context", but we have neither here. What OS window should new tabs be opened in?
I guess https://labs.w3.org/repo-manager/pr/id/w3c/webdriver-bidi/133 indicates that y’all should have @cwilso add you to the Browser Testing and Tools WG |
For my own review later, |
Looks like CDP has a |
https://github.com/puppeteer/puppeteer/blob/e94a6b73d3014487b48a018fdcfb5f9874c86bdf/src/common/Browser.ts#L431-L443 seems to be the only use of |
OK, so maybe we should start with about:blank for simplicity. Even so we'll need to decide whether |
Yes, I've seen some email about that :) |
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<AutomatedTester> Topic: browsing context create command<AutomatedTester> github: https://github.com//pull/133 <AutomatedTester> foolip: THis is a PR that I sent out today <AutomatedTester> ... it's basically boilerplate and some questions <jgraham> q+ <AutomatedTester> ... we should merge jgraham's PRs first and then have a look at this PR <AutomatedTester> ack jgraham <foolip> q+ <AutomatedTester> jgraham: There is an open question around creating a context and then do we get events? I think the spec already handles this case already <AutomatedTester> ack foolip <AutomatedTester> foolip: I think that's right, we will get the events, it's just around the order of the events. <AutomatedTester> q? |
As asked offline, here is the raw CDP log when new page with a given URL is open
So the |
78ae090
to
1f454a6
Compare
@whimboo do you want to take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read it right there is still a missing item to get fixed which was inappropriately marked as resoved.
Co-authored-by: Henrik Skupin <mail@hskupin.info>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @foolip! This looks fine now. I assume you will merge yourself.
SHA: f5ce465 Reason: push, by @sadym-chromium Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This is based on @jgraham's suggestion: #133 (comment)
This is based on @jgraham's suggestion: #133 (comment)
This is based on @jgraham's suggestion: #133 (comment) Co-authored-by: Julian Descottes <jdescottes@mozilla.com> Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
This is based on @jgraham's suggestion: #133 (comment) Co-authored-by: Julian Descottes <jdescottes@mozilla.com> Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
This is based on @jgraham's suggestion: #133 (comment)
This is based on @jgraham's suggestion: #133 (comment) Co-authored-by: Julian Descottes <jdescottes@mozilla.com> Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
This is based on @jgraham's suggestion: w3c#133 (comment) Co-authored-by: Julian Descottes <jdescottes@mozilla.com> Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Fixes #116.
Preview | Diff