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

Specify navigating a browsing context #43

Closed
foolip opened this issue Jul 14, 2020 · 17 comments
Closed

Specify navigating a browsing context #43

foolip opened this issue Jul 14, 2020 · 17 comments
Assignees

Comments

@foolip
Copy link
Member

foolip commented Jul 14, 2020

One of the first things any automation script will do is to get a browsing context (#42) and navigate it. Navigating a browsing context is therefore something we'll want to work on fairly early, although it might not be the very first command.

At a high (enough) level it should be very similar to https://w3c.github.io/webdriver/#navigate-to, but there are plenty of considerations:

@jgraham
Copy link
Member

jgraham commented Jul 14, 2020

On waiting vs no waiting, what's does the precedent look like here in devtools/clients. I assume that the model is "no waiting, you have to listen for the events you want to use to determine the page is loaded enough", but maybe that's too low-level (it's certainly going to be at least one mode, since it basically corresponds to the pageLoadingStrategy of none).

@bwalderman
Copy link
Contributor

In CDP when you send a Page.navigate command, it responds as soon as the navigation has committed and the response includes a frameId you can use to keep track of events coming from that frame. We could do something similar by default (basically the "none" strategy) but also allow the client to pass in a page loading strategy of "eager" or "normal" if they want classic semantics.

@whimboo
Copy link
Contributor

whimboo commented Aug 27, 2020

As reference in Puppeteer the navigation immediately returns by default when the HTTP response has been received. If it should wait longer the waitUntil argument can be used to extend the wait for one or more lifecycle events like DOMContentLoaded, load, or even networkidle0 or networkidle2 (no more HTTP traffic).

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Navigation.

The full IRC log of that discussion <AutomatedTester> Topic: Navigation
<jgraham> GitHub Bot: https://github.com//issues/43
<AutomatedTester> Github topic: https://github.com//issues/43
<AutomatedTester> jgraham: the point of this topic, is how should navigation in the bidi topic
<AutomatedTester> ... [reads from the comment in the issue]
<AutomatedTester> q?
<simonstewart> q+
<AutomatedTester> ack simonstewart
<brwalder> q+
<AutomatedTester> simonstewart: From the comments in the issue, when the navigation starts it returns automatically and then people will need to listen for events
<jimevans> q+
<AutomatedTester> ... I think that it would be good to have webdriver http reformulated to work ontop of bidi here.
<AutomatedTester> ack brwalder
<AutomatedTester> brwalder: From reading the comments, I imagine this a command that doesnt return instantly and then listening for events
<AutomatedTester> ... so people want to use this then use the original page load strategies from webdriver http
<AutomatedTester> ... as doing it as instant return and listening for events is making things very complex for what should be thought of by users as simple
<jgraham> q+
<AutomatedTester> ... and I agree with simonstewart here
<AutomatedTester> simonstewart: I like the ability passing in the page load strategy
<mathiasbynens> +1 global state (setting a default page load strategy) is evil
<AutomatedTester> jgraham: global state is evil and we should avoid it
<AutomatedTester> brwalder: I wasn't thinkking of a global state but the client would be sending this per commande
<AutomatedTester> q?
<AutomatedTester> ack jimevans
<AutomatedTester> jimevans: is there mileage in leveraging the DOM events, like puppeteer?
<AutomatedTester> ... It would be relatively easy for a client library to descript it's page load strategy based on DOM Events
<AutomatedTester> ... WebDriver HTTP kinda does this for it's page load strategy
<simonstewart> q+
<AutomatedTester> ... would allow clients to become opinionated in this way
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart: brwalder highlighted that if you wanted to avoid the dance of setting things up correctly
<AutomatedTester> ... and that it would be a lot of work for clients
<simonstewart> q-
<AutomatedTester> jgraham: my initial take is page load strategy does things simply
<AutomatedTester> ... and as whimboo will attest, navigation is full of edge cases
<shengfa> q+
<AutomatedTester> ... [describes a use case]
<brrian> q+
<AutomatedTester> ack jgraham
<brwalder> q+
<AutomatedTester> ... and we need to make sure that the load event matches where you're expecting
<AutomatedTester> ... and what does navigate return?
<AutomatedTester> ... in CDP you get a loader event
<AutomatedTester> ... I dont know if this is described in the platform
<AutomatedTester> q?
<AutomatedTester> ack shengfa
<AutomatedTester> shengfa: I would like to comment on how CHromedriver does it
<AutomatedTester> ... there are loads of edge cases here
<AutomatedTester> ... we try keep track of all the frames
<AutomatedTester> ... and we look at frame start/stop loading events
<AutomatedTester> ... and checking readyState
<AutomatedTester> ... I don't know if we need to specify things better in bidi
<AutomatedTester> ... so do we want to expose the page loading strategy or all the events
<AutomatedTester> ... for exposing the events, we can specify the webdriver events
<jgraham> q+
<AutomatedTester> ... or expose the events we think are relevant to the client
<AutomatedTester> ... and regarding the workflow, the navigation events would just ack and then listening for events
<AutomatedTester> ... and the navigation is just a "subscribe to these events" type call
<AutomatedTester> q?
<AutomatedTester> ack brwalder
<AutomatedTester> ack brwalder
<AutomatedTester> ack brrian
<jgraham> q+ brwalder
<jgraham> q- later
<AutomatedTester> brrian: from safari there are lot of junk in this area
<AutomatedTester> ... and people do want events
<AutomatedTester> ... and the happy path should look nicely to them
<AutomatedTester> ... and I don't want to be exposing the loaders to the clients
<AutomatedTester> ... and we should make sure that we solve the correct use cases
<AutomatedTester> q?
<AutomatedTester> ack brwalder
<AutomatedTester> brwalder: It sounds like there are 2 legitimate paths for end users
<AutomatedTester> ... a happy path -> THe page just is loaded
<simonstewart> q+
<AutomatedTester> ... and the more exotic usecases and allow people to do more "advanced"
<AutomatedTester> ... and we can accept the page load strategy and we do "best case"
<AutomatedTester> ... and then provide an escape hatch and allow immediate return and listen for events
<AutomatedTester> ... and I think we should support both
<AutomatedTester> q?
<drousso> q+
<AutomatedTester> ack jgraham
<AutomatedTester> jgraham: there is definitely a concensus on making sure th
<AutomatedTester> ... what WEbdriver http already does and then for what ee
<AutomatedTester> events.
<AutomatedTester> ... [describes CDP implementation]
<AutomatedTester> ... we should look into the feasibility that if you subscribe events to a navigation so that we know that things are tied back to the initial command
<AutomatedTester> ... and we don't want people to have to guess that an event came from the command... hopefully
<AutomatedTester> q?
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart: one thing we forgot so far... if this coming from a Selenium cloud providers
<drousso> q-
<AutomatedTester> ... and it would be good to make sure that we dont send too much data back for those clients as we have 2 internets in the way
<AutomatedTester> q?
<AutomatedTester> Resolution: Specify navigation taking a page load strategy parameter to match WebDriver HTTP and investigate the page load life cycle events and exposing those

@foolip
Copy link
Member Author

foolip commented Nov 2, 2020

The above minutes are from https://www.w3.org/2020/10/27-webdriver-minutes.html#t02. I'd like to ask for clarification on the proposed model to make sure I understand it correctly.

My understanding is that we'd have a "context.navigate" command (assuming #62) with an optional parameter taking WebDriver Classic's values "none", "eager" and "normal". The command would be potentially long running, and the command response (containing the command ID) would only be sent once the page was loaded to the requested point.

Separately, there would be events corresponding to a change in current document readiness which one could enable to get events and precisely before (or after?) the "context.navigate" command response is sent.

Some of the details are from my assumptions, but does sound right?

If that is right, then I wonder if the protocol itself needs to provide two ways to achieve the same thing, as opposed to leaving ergonomic shorthands to client libraries? Or is there something that would be harder than it first appears if one had to rely on just events?

@jgraham
Copy link
Member

jgraham commented Nov 2, 2020

I think that's correct.

The concern about leaving this to libraries is that it can be very hard to get right. The typical edge case is that you request a load and before the command is fully processed you get an event from another pending load (which might even be e.g. an about:blank) and mistakenly assume that the load you scheduled is complete. So clients probably need to be careful to track the request id (CDP: loader id), but if they aren't it will mostly work just with occasional races (and maybe the opposite race is also possible where you get a load that you did initiate, but before you've registered the appropriate request id). Meanwhile the browser side needs to get this right anyway, so why not provide some slightly higher level features to make common cases easier whilst still providing the necessary primitives to do something more advanced.

Alternative framing: providing the commonly used options as explicit modes is favoured by the Priority of Constituencies since it means library authors don't all have to reimplement the same non-trivial logic.

On another note, I wonder how we specify a request id. I bet that's not exposed in the standards.

@whimboo
Copy link
Contributor

whimboo commented Nov 3, 2020

Given that the current navigation logic is not always enough to await a page to be loaded, I wonder if we should pick-up the networkIdle and networkAlmostIdle like lifecycle events from CDP. Those (or only the idle one) could be mapped as new page load strategies, or arguments.

If we would consider using these we might have to take care of observing the activity for network channels as currently used for the request via network events.

@foolip
Copy link
Member Author

foolip commented Nov 3, 2020

@jgraham do you think we could ensuring that the logic is trivial, by first specifying the corresponding events, and defining page load strategy in terms of the hooks for those events? The convenience wrapper, whether in implementations to support a page load strategy parameter, or in client libraries for more advanced cases, should ideally be:

  • subscribe to the appropriate events
  • initiate the navigation, get some ID from the response
  • wait for the appropriate event with that ID

If it's any more complicated than that, then I'd question if we've exposed the right events to allow for the more advanced cases.

@jgraham
Copy link
Member

jgraham commented Nov 3, 2020

That's how I'd expect that things work out. It may turn out that the page load strategy stuff isn't a huge value add, but I also think it's not going to be difficult to specify or implement and in the TPAC discussion there seemed to be a strong consensus that creating the higher-level abstraction here is a real benefit.

@whimboo : Do we know what heuritics those events are using? It seems like the kind of thing that could be an interop concern if browsers do slightly different things (e.g. a test could depend on a resource being loaded in browser A that has not in fact completed loading in B).

@whimboo
Copy link
Contributor

whimboo commented Nov 3, 2020

@whimboo : Do we know what heuritics those events are using? It seems like the kind of thing that could be an interop concern if browsers do slightly different things (e.g. a test could depend on a resource being loaded in browser A that has not in fact completed loading in B).

@mathiasbynens maybe you could given some feedback here regarding the networkIdle and networkAlmostIdle events? See also my last comment from above.

@jgraham
Copy link
Member

jgraham commented Dec 11, 2020

From #62:

CDP has a separate Page.frameNavigated event. Navigating a browsing context will indeed result in the tree of browsing contexts changing, but I agree it's not clear how best to represent that.

Maybe as a part of this work we need to decide if we want to have contextDestroyed events when contexts become inactive, even if they are alive in the bfcache. Or if we want another kind of event or just to have a navigation event and let clients figure out that contexts under the navigated context are inactive if not destroyed.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Navigation.

The full IRC log of that discussion <AutomatedTester> Topic: Navigation
<AutomatedTester> github: https://github.com//issues/43
<AutomatedTester> jgraham: This is the next feature that I want to work on
<AutomatedTester> ... we have previously discussed this at TPAC
<AutomatedTester> ... and I have documented it in the issue
<AutomatedTester> ... since this is async we can allow people to return when they see fit
<AutomatedTester> ... and we will likely need to define lifecycle steps for this
<foolip> q?
<simonstewart> q+
<AutomatedTester> ... and a question I have for Googlers. What is a loader id and what is it's lifecycle? THis is in CDP and blink and it's very different to requests
<AutomatedTester> ack simonstewart
<AutomatedTester> simonstewart: 2 things: We are wanting to do network interceptions
<AutomatedTester> ... 2nd thing: It would be good to reformul.ate the page loading strategies from Webdriver http in the bidi
<AutomatedTester> jgraham: THe way that I imagine this working is that we would have a way of setting these
<foolip> q+
<AutomatedTester> ... and people will say when the call returns and they can go from there
<AutomatedTester> ack foolip
<foolip> https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/loader/document_loader.h;l=245;drc=2ac64302ae161cd6b5e4b1254497bdf5fd6d3415
<AutomatedTester> foolip: I don't know what loader id is and found this...
<AutomatedTester> ... it seems to be attached to a browsing context
<AutomatedTester> ... and it looks to have been around for a while and could be in webkit for the inspector is
<AutomatedTester> ... and I don't know the smart questions to ask for will help ask questions to the relevant people
<AutomatedTester> q?
<jgraham> RRSAgent: make minutes v2
<RRSAgent> I have made the request to generate https://www.w3.org/2021/02/10-webdriver-minutes.html jgraham
<foolip> thanks AutomatedTester!
<jgraham> RRSAgent: bye
<RRSAgent> I see no action items
<jgraham> Zakim, bye
<Zakim> leaving. As of this point the attendees have been jgraham, AutomatedTester, sadym, drousso, brwalder, Honza, jdescottes, shengfa, whimboo, cb, foolip, simonstewart, jimevans
<AutomatedTester> how do we get github bot to close out

@jgraham
Copy link
Member

jgraham commented Mar 10, 2021

I've been making some progress on this locally. It looks like we're going to need quite a lot of integration points in the HTML spec; I've filed whatwg/html#6429 for that. The idea is that each async navigation (i.e. excluding fragment navigations or those that fail early) will get a navigation id, which I think is the analouge of the loader id in CDP.

API wise, I'm currently thinking:

  • A browsingContext.navigate command that takes a browsing context id, an optional waitUntil parameter which determines whether to return as soon as the HTML navigate algorithm starts running async, or to (asynchronously) wait until either DOMContentLoaded or load for the relevent context for the navigation id that's created (for fragment navigations we always return straight away; there are also various error conditions here that need to be handled and stuff like navigation ending in a download rather than a document load). In all async cases this returns the navigation id of the navigation that was started.
  • One outstanding question here is whether this ought to take a timeout parameter or if we're OK with putting that logic on clients.
  • A browsingContext.navigationStarted event that provides the context id, navigation id, and url for any non-fragment navigation (either client-initiated or browser initiated).
  • A browsingContext.fragmentNavigated event that provides the context id and url for any synchronous navigation e.g. a fragment navigation (could maybe rename this to match Page.navigatedWithinDocument from CDP and verify the semantics match for history API usage).

The remaining events are a bit uncertain. I think we need something close to Page.downloadWillBegin for navigations that end in a download. For observing the later stages of document-load type navigations, CDP has Page.lifeCycleEvent that also covers things like painting which are harder to specify cross-browser. AFAICT the special thing about this event is that it is part of the page domain but has to be explictly enabled. I assume the per-event subscription model in BiDi means we don't require this extra switch.

I assume that we're going to need at least:

  • A browsingContext.domContentLoaded event with the context id, navigation id, and url.
  • A browsingContext.load event with the context id, navigation id, and url.

I assume that the other lifecycle events are out of scope for this bug. The only concern is whether clients want to depend on network[Almost]Idle for waiting for a stable state after load, and how that's defined.

@whimboo
Copy link
Contributor

whimboo commented Mar 10, 2021

* A `browsingContext.navigate` command that takes a browsing context id, an optional `waitUntil` parameter which determines whether to return as soon as the HTML navigate algorithm starts running async, or to (asynchronously) wait until either `DOMContentLoaded` or `load` for the relevent context for the navigation id that's created (for fragment navigations we 

always return straight away; there are also various error conditions here that need to be handled and stuff like navigation ending in a download rather than a document load).

So what about the network idle specific values that CDP currently supports, and probably emits on its own after some time without network activity for the loaderId? It's more reliable to detect when a navigation has actually been done, and no more resources are loaded. I'm fairly sure this will be a request from a lot of users.

* One outstanding question here is whether this ought to take a timeout parameter or if we're OK with putting that logic on clients.

If the timeout is on the client side there must be a way to actually run a clean-up on the remote end. With the navigation various event handlers etc might have been set internally by the browser, which need to be removed again in case the page doesn't finish loading.

I assume that we're going to need at least:

* A `browsingContext.domContentLoaded` event with the context id, navigation id, and url.

* A `browsingContext.load` event with the context id, navigation id, and url.

As reference here a list of page.* events that CDP actually emits:

    "frameStartedLoading",
    "frameNavigated",
    "domContentEventFired",
    "loadEventFired",
    "navigatedWithinDocument",
    "frameStoppedLoading",

Some of them are actually marked as experimental. And as mentioned by @jgraham there are also the lifecycle events.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Navgation progress.

The full IRC log of that discussion <AutomatedTester> topic: Navgation progress
<AutomatedTester> github: https://github.com//issues/43
<AutomatedTester> jgraham: there a comment in https://github.com//issues/43#issuecomment-795507208
<AutomatedTester> ... and I am working on the assumption that we have a way to navigate
<AutomatedTester> ... and that we want a way for people to "wait" on certain events or DOM Ready
<AutomatedTester> ... there mig
<AutomatedTester> ... there might be work on fixing the html spec
<AutomatedTester> ... and this for navigations that happen in the client or events that hapepn on the web like links
<AutomatedTester> ... fragments are a special case as they are synchronous
<AutomatedTester> ... and comparing with CDP, that only shares the lifecycle events
<AutomatedTester> ... and we don't need the whole raft of events
<AutomatedTester> ... we are likely to know some of the end points of the navigation, like a download, would need to be shared
<AutomatedTester> q?
<foolip> q+
<AutomatedTester> ack foolip
<AutomatedTester> foolip: there are 2 things like network idle and network almost idle don't appear to be in any spec so we could have that as out of scope
<bj> I strongly prefer to mirror navigation timing and other existing specs.
<brwalder> q+
<AutomatedTester> ... we can have this mirror real world specs
<AutomatedTester> ... and I think that whimboo said that he exspects people to ask for this striaght away
<AutomatedTester> ... and I agree with bj on this that we should follow other specs
<AutomatedTester> ack brwalder
<AutomatedTester> brwalder: for clients that want to use network idle as the stopping point
<AutomatedTester> ... we would need to intro the concept of a netwrok requests
<AutomatedTester> ... which we currently dont have
<foolip> Per https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#pagegotourl-options it looks like "load" is the default of Puppeteer, which is a relief :)
<AutomatedTester> ... any work on network requests would need to be done before we move onto network idle and leave it out of scope for now
<AutomatedTester> q?
<jgraham> It looks like "networkidle2" is a possible value for waitUntil in puppeteer, so not the default but exposed

@jgraham
Copy link
Member

jgraham commented Mar 17, 2021

So what about the network idle specific values that CDP currently supports, and probably emits on its own after some time without network activity for the loaderId? It's more reliable to detect when a navigation has actually been done, and no more resources are loaded. I'm fairly sure this will be a request from a lot of users.

I think it's a reasonable feature request, but it's at least non-trivial to specify so I'm inclined to leave it until later (once we have some integrations into fetch, for example).

If the timeout is on the client side there must be a way to actually run a clean-up on the remote end. With the navigation various event handlers etc might have been set internally by the browser, which need to be removed again in case the page doesn't finish loading.

That problem doesn't seem related to timeouts as such; if the browser is stuck in a never-stops-loading state the fact that we've registered some event handlers isn't the biggest problem. If the navigation does complete eventually we'll go through the normal unregistering process once it does and emit a message that the client will proceed to ignore. But that doesn't seem like loads of potential overhead for what is already an error situation.

    "frameStartedLoading",
    "frameNavigated",
    "domContentEventFired",
    "loadEventFired",
    "navigatedWithinDocument",
    "frameStoppedLoading",

So we have analouges to most of these events, but we always provide the navigation id where appropriate whereas (afaict) with CDP you need the lifecycle events to get the navigation (loader) id. I'm not sure why they have different events for similar parts of the lifecycle with different properties, so if there's something I've missed in the design it would be good to know what it is :)

@foolip
Copy link
Member Author

foolip commented Jul 19, 2021

Fixed by #93.

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

No branches or pull requests

5 participants