-
Notifications
You must be signed in to change notification settings - Fork 39
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 commands and events related to navigation #93
Conversation
This is intended to allow awaiting given hooks to be invoked from other specs in async algorithms, and resuming when the first hook is invoked. Data may be passed back when resuming. This is conceptually similar to something like: let data = await Promise.race([event1, event2, ...])
This adds a navigate command to the browsingContext module that initiates a navigation of the given context to the given URL. The command also accepts a wait parameter which affects whether it returns immediately after the navigation is started or waits until the document that's loaded is eventually parsed. In the case of early return the navigation id allows later events to be connected to this specific navigation. In addition we also add a set of events relating to the navigation lifecycle, corresponding to the navigation starting, failing, reaching domContentLoaded, and reaching load. There is also an event for synchronous same-page navigations. There are quite a few open questions here about how we want to expose error conditions. Currently we emit errors from the command, but these don't have a lot of structured information and might end up with local ends trying to parse additional information out of the message part.
index.bs
Outdated
1. Let |url| be the value of the <code>url</code> field of |command | ||
parameters|. | ||
|
||
1. TODO: validate |url|. Should we allow relative URLs or only absolute URLs? |
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.
CDP's Page.navigate requires an absolute URL at the moment, so this would require some protocol changes.
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.
So for now I've allowed relative URLs because this seems to reduce the burden on clients. But if that's going to be an implementation problem then we can certainly disallow them.
index.bs
Outdated
1. If |wait condition| is interactive, let |event name| be | ||
"<code>DOMContentLoaded</code>", otherwise let |event name| be "<code>load</code>". | ||
|
||
1. Let (|event received|, |status|) be [=await=] [(|event name|, |navigation |
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 places 3 pairs in the wait queue, but at most 1 of them will be resumed which leaves 2 in the queue indefinitely. Should we define an "await any" primitive in this spec similar in purpose to Promise.race() that lets us express this step more cleanly? Might come in handy as we add more functionality to the spec.
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.
For now I've changed things so there's a single id for all the cases, so we can make the spec structure a map from id to events. I don't know if that will always work, or if there will be cases where we might get a different id for different events we await (also, it doesn't really matter if we happen to leak in the spec; obviously it's a problem in a real implementation, but hopefully it's not too difficult to infer that the intent here is "wait for whichever of these things happens first" and it's obvious that should translate into Promise.race
or implementation equivalent.
Co-authored-by: Brandon Walderman <bwalderman@outlook.com>
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<AutomatedTester> Topic: Navigation<AutomatedTester> github: https://github.com//pull/93 <AutomatedTester> jgraham: This is mostly a status update topic <AutomatedTester> ... the Navigation part of the spec is there that uses Navigable and the events <drousso> whoops just remembered <drousso> :P <AutomatedTester> ... so could people please read the PR <simonstewart> q+ <AutomatedTester> q? <AutomatedTester> ... it does things that WebDriver and CDP doesnt do things which I think is nicer for end users <AutomatedTester> ack simonstewart <foolip> I will review, jgraham. Also, I can only join half of the call today, and will leave now. <jgraham> s/does things/resolves relative urls/ <AutomatedTester> simonstewart: the reason it's not done in navigate is that I am not clever enough because it's non-trivial <AutomatedTester> ... most users just want to do one navigation and then user interactions to move around the rest of the site <AutomatedTester> ... cb may have more anecdotal data on this <AutomatedTester> q? |
I've reviewed whatwg/html#6496 and I think the rough shape of the integration between HTML and BiDi is clear now. Whether through extensible hooks or direct calls, HTML will call us with a navigation status containing a navigation ID, a status (maybe nullable) and the URL for the cases @jgraham listed:
I'll review this PR assuming that will stand. |
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 a lot of work, and wasn't very fast to review, but I'm sorry it took me this long. Thanks @jgraham!
index.bs
Outdated
|
||
1. Remove |id| from [=wait queue=]. | ||
|
||
1. Resume running the steps in |algorithm| from the point at which they were |
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.
Is the intention that the caller of resume actually keep running algorithm while still on the call stack, or should this be more like a signal to keep doing work at the next spin of the event loop, microtask checkpoint, or similar?
The synchronous behavior would mean there's more to think about possibly going wrong here, if it were actually implemented that way.
The precise timing of this is probably quite important, because it determines if any BiDi messages can be sent between the event that causes the navigation command to complete, and the response for the navigation command. I think it would be very good indeed if we can ensure that these are always back-to-back, never interleaved in some way given lots of navigation commands happening in parallel.
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.
The intention is that in reality this would look like:
[steps up to the await]
let event1 = new Promise(resolve => addEventListener("event1", resolve));
let event2 = new Promise(resolve => addEventListener("event2", resolve));
await Promise.race([event1, event2]);
[steps after the await]
So I think that would be "the algorithm resumes at the next microtask checkpoint"? Do you have suggested wording to make this explicit?
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 see, so in other words it should not keep running while the caller of resume stick on the stack, which I agree is the right choice! I think the way to say this is "queue a microtask to resume running ..."
@@ -130,6 +131,41 @@ This specification depends on the Infra Standard. [[!INFRA]] | |||
|
|||
Network protocol messages are defined using CDDL. [[!RFC8610]] | |||
|
|||
This specification defines a <dfn>wait queue</dfn> which is a map. | |||
|
|||
Issue: Surely there's a better mechanism for doing this "wait for an event" thing. |
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.
Right now there's just a single place that awaits, and a bunch of places that call resume with various values. It kinda works, doesn't it?
Another way to structure it would be to have a global async generator of sorts where every navigation event is yielded, and the navigate algorithm has a async for loop waiting for the expected event. I'm not sure it'd be less pseudo-code overall, but it would put most of the complexity in the navigate command.
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 suspect we're going to have more commands that end up async waiting for one or more events on the browser side, so punting the complexity into navigate doesn't obviously seem like a win. I also suspect this part will end up changing significantly in response to further use cases.
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, let's leave this alone and let the inline issue be a reminder that it's not set in a particularly hard type of stone.
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<AutomatedTester> topic: Navigation commands<AutomatedTester> github: https://github.com//pull/93 <AutomatedTester> jgraham: There is a PR up and a PR for the integration to HTML. brwalder and foolip have done a first pass <AutomatedTester> ... this is nearly ready to land <AutomatedTester> ...I have summarised the first first "issues" related to this <AutomatedTester> ... WHen we navigate and it's initiated from webdriver we have the response from the command and the events that are returned <AutomatedTester> ... foolip had the question that events happen and then we return the navigate request response <AutomatedTester> ... and does this make sense or should we respond for the command and then do the events <AutomatedTester> ... [describes how the promises can be responded to and the order] <AutomatedTester> ... the 2nd question is do we handle relative urls <AutomatedTester> ... and the 3rd thing is the proposal <AutomatedTester> ... has wait for 1 event or do we want to be like puppeteer and wait for a group of events <AutomatedTester> ... and since in html the event order is always guaranteed <AutomatedTester> ... but should we care about the network idle <AutomatedTester> ... so I could do with help understanding the use case <AutomatedTester> q? <AutomatedTester> ... if we are going to support an array we need to do it from the start and not patch it in later <AutomatedTester> ... and the last topic is around page reloading <AutomatedTester> ... I don't think this will be controversial and we can copy parts of puppeteer here <jgraham> s/puppeteer/CDP/ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Quoting/paraphrasing some bits from the logs:
Was there any discussion on this point? Searching GitHub for networkidle0 and networkidle2 turns up enough matches that I think we might eventually need to add something similar. I think the use case is that the page under test itself listens for "DOMContentLoaded" or "load" to load more stuff and make the page usable, but when it's no longer using the network it's probably finished. I assume "networkidle2" as added for pages doing long-polling or something. However, event adding these events, the order could be guaranteed: "DOMContentLoaded", "load", "networkidle2", "networkidle0". It's only if we have events where the order isn't guaranteed, like "fonts are loaded" + "video has preloaded" (very hypothetical) that waiting for both is strictly necessary. @mathiasbynens @sadym-chromium do you know what use cases people have for something like Since CDP doesn't have this convenience at all, I'm leaning in the direction of simplicity, just one event and betting that the set of events we want will always have a strict ordering. |
Any discussion on this? To summarize the issue again, it's that there's a both response acknowledging the success of the navigate command itself, and an event that signified the same thing. The order is important, both that it's well defined for interop, but also because of client ergonomics. |
@foolip , I'd prefer to have the navigate command respond only after the requested event (events??) are fired. it seems odd for a client to initiate a navigation via webdriver, choose a |
@bwalderman it would be guaranteed in any case that the event has already happened, and we should guarantee that the command completion and event are back-to-back in the WebSocket channel, it's only a matter of the order. That being said, having the events come first seems sensible when waiting for the "DOMContentLoaded" or "load" events. The trouble I see is with "synchronous" (from the HTML navigate algorithm PoV) success or failure to navigate, if we send the corresponding BiDi events first, the client can't know if that's because of its own attempt to navigate, or something else that was happening in the page. Maybe that's acceptable, or maybe we should change the order of event and command response here? |
So re: event order, it seems like CDP has the command return first and then emits an event later, even in the synchronous case. However it doesn't really help with the problem as the command doesn't return a navigation id, so you can't guarantee that the event you see is actually caused by the command you sent; all you can tell is that it happened in the same frame. We could have a mechanism to suppress emitting a fragmentNavigated event in the case that we've initiated a navigation, and some way to re-send that event once the command completes. We don't currently have that infrastructure, but it's not impossible. I think if we want that we should make sure that all events get a non-null navigation id, so that it's always possible to get the command return value knowing that no events could possibly have been emitted and use the navigation id field from that return value to determine which events are caused by that navigation. That probably still wouldn't be the easy way to use the command (since it requires returning as soon as the navigation id is known, and writing all the wait logic in the client). |
Searching for these options revealed the following documentation in Puppeteer: But there is still no mentioning of actual use-cases. Digging further gave me this discussion:
|
About which events do we have concerns here? When speaking about navigation via CDP we refer to Page.navigate. This method always returns immediately when the network response has been received. It doesn't wait for any page load event ( The only events that are sent out by CDP before the Otherwise the command returns a
What about defaulting to a wait logic that includes But that opens the question what we want to do for the |
I'm much more inclined to make the page load strategy a feature of the HTTP protocol and make the BiDi protocol specify the required behaviour for each navigation. Clients of course can have an API like
|
In such a case we should clearly articulate that in the BiDi spec. Especially because both protocols refer to the same capabilities.
Yes, that would be great and would allow to change the behavior without actually having to re-create the session to only adjust the strategy. But maybe it could be combined? If no |
Right. So with CDP, the following happens:
With BiDi, as the spec is at the moment:
My hope was that all the session-global state from WebDriver-classic would be represented by specific per-command fields in BiDI and we wouldn't use any of the session globals. Others might have a different opinion on this though. |
OK, I updated this to emit the fragementNavigated event after the command returns. |
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<AutomatedTester> topic: context.navigate<AutomatedTester> github: https://github.com//pull/93 <AutomatedTester> jgraham (IRC): the next three items are are status updates and requests for feedback <AutomatedTester> ... foolip (IRC) has reviewed the html parts and hope to get that landed soon <AutomatedTester> ... and this is the last call for the bidi to get this landed soon <AutomatedTester> ... and there has been discussion about how the event ordering should be compared to the command returning <AutomatedTester> ...[gives example around load event] <AutomatedTester> ... this is important to know when a navigation has happened by the test or by something else <AutomatedTester> ... fragments are a special case as they are synchronous <AutomatedTester> ...[describes how things could return] <AutomatedTester> q? <AutomatedTester> github-bot (IRC): end topic |
whatwg/html#6496 is in its near-final form now and I've double checked that the terms linked by HTML are all defined, for example https://pr-preview.s3.amazonaws.com/w3c/webdriver-bidi/pull/93.html#webdriver-bidi-load-complete. Once this PR is approved, we should first merge this, make sure it deploys and https://w3c.github.io/webdriver-bidi/#webdriver-bidi-load-complete exists, then merge the HTML side. |
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 all seems pretty sensible to me now. I'm sure if I went over it with a comb again there's something worth fixing, but I'm gonna say it's time to land this and iterate.
This is different to a failure in edge cases e.g. where window.stop is called, and we probably want to just treat the navigation as a success in this case.
Move the navigation status struct into WebDriver, and don't use the return value of navigate, but instead assume that a hook will be called before the navigation enters the async section.
The HTML side changed, so this now needs further review. Sorry. |
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 reviewed the two latest commits and think I reviewed everything before that.
This change is motivated by WebDriver BiDi needing to observe the progress of navigations, in order to support commands and events related to navigation: w3c/webdriver-bidi#93 In HTML, this adds a single new concept; a navigation id which is a unique id generated for each navigation. This enables WebDriver BiDi to tell when events are from the same navigation. In WebDriver BiDi, a navigation status struct is defined, with an id, a URL and a status. This is used as a uniform interface for communicating the navigation progress through all the integration points, even though not all fields are useful in all cases. The actual integration points added are: * Navigation started * Navigation aborted * Navigation failed * Download started * Fragment navigated * DOM load complete * Load complete
I have just merged whatwg/html#6496, now merging this. |
This change is motivated by WebDriver BiDi needing to observe the progress of navigations, in order to support commands and events related to navigation: w3c/webdriver-bidi#93 In HTML, this adds a single new concept; a navigation id which is a unique id generated for each navigation. This enables WebDriver BiDi to tell when events are from the same navigation. In WebDriver BiDi, a navigation status struct is defined, with an id, a URL and a status. This is used as a uniform interface for communicating the navigation progress through all the integration points, even though not all fields are useful in all cases. The actual integration points added are: * Navigation started * Navigation aborted * Navigation failed * Download started * Fragment navigated * DOM load complete * Load complete
Add command and events related to browsing context navigation.
This adds a navigate command to the browsingContext module that
initiates a navigation of the given context to the given URL. The
command also accepts a wait parameter which affects whether it returns
immediately after the navigation is started or waits until the
document that's loaded is eventually parsed. In the case of early
return the navigation id allows later events to be connected to this
specific navigation.
In addition we also add a set of events relating to the navigation
lifecycle, corresponding to the navigation starting, failing, reaching
DOMContentLoaded, and reaching load. There is also an event for
synchronous same-page navigations.
There are quite a few open questions here about how we want to expose
error conditions. Currently we emit errors from the command, but these
don't have a lot of structured information and might end up with local
ends trying to parse additional information out of the message part.
The PR depends on changes in HTML to define the necessary integration points. I'll update with a link to that PR once it's ready.
Preview | Diff