w3c / webdriver-bidi

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

The `BrowsingContext.reload` command needs to return a NavigateResult #527

Closed whimboo closed 11 months ago

whimboo commented 11 months ago

Currently the browsingContext.reload command returns an EmptyResult and not a NavigationResult. This is problematic because clients will require the navigation id to check for the navigation completed (in combination with the wait argument for none and interactive) or even for specific network events.

jgraham commented 11 months ago

Note that the spec here is just broken: https://w3c.github.io/webdriver-bidi/#command-browsingContext-reload has unconditional return statements in both steps 9 and 10, and the one in step 9 will return a browsingContext.NavigateResult, contrary to the CDDL which claims EmptyResult.

It appears that #382 broke this as it didn't realise we were already returning a browsingContext.NavigateResult and so assumed the return type was empty.

jgraham commented 11 months ago

https://github.com/w3c/webdriver-bidi/pull/528

whimboo commented 11 months ago

The PR has been merged.

thiagowfx commented 11 months ago

The wait=None case cannot guarantee an updated navigation id, as there's no await.

Should we document that?

thiagowfx commented 11 months ago

Chromium mapper PR, with tests: https://github.com/GoogleChromeLabs/chromium-bidi/pull/1255

wait=None cannot have such tests, as the new ID is typically the same as the old ID (as we do not await for page lifecycle events).

thiagowfx commented 11 months ago

cc @OrKoN @Lightning00Blade

jgraham commented 11 months ago

I think per spec the reload will cancel the existing navigation and should get a new ID. That seems pretty much like what a user would expect, so I'm not sure what you want us to document, or why this issue was reopened.

thiagowfx commented 11 months ago

This is specifically about wait=None. Because we return NavigationResult very quickly, before the navigation (reload) has finished, the navigation id will be the same as the previous instance.

My question is whether we should return EmptyResult for wait=None, or document that the return ID may not necessarily be the new ID, as there's no "await" (unlike "interactive" and "complete").

juliandescottes commented 11 months ago

Reading the BiDi navigate spec, I think it's implied that the navigation id is generated by WebDriver BiDi and used for the HTML navigate:

Navigate navigable with resource request, and using context’s active document as the source Document, with navigation id navigation id,

However the HTML spec for navigate does not seem to accept a navigation id as optional parameter for navigate, and instead always generates one at step 6:

Let navigationId be the result of generating a random UUID. [WEBCRYPTO]

Do we need an update in the HTML spec to accept a navigation id from WebDriver ?

whimboo commented 11 months ago

This is specifically about wait=None. Because we return NavigationResult very quickly, before the navigation (reload) has finished, the navigation id will be the same as the previous instance.

My question is whether we should return EmptyResult for wait=None, or document that the return ID may not necessarily be the new ID, as there's no "await" (unlike "interactive" and "complete").

We have issues as well for that in Firefox and return with about:blank in such a case. Nevertheless this is a specific issue which we might be able to handle in a follow-up issue? Would you mind to file one? I would like to update the tests to what has been landed here. @thiagowfx

thiagowfx commented 11 months ago

I'll file a separate issue.

You can update the tests for wait=complete and wait=interactive, but at this point since wait=none is not agreed upon, it may be worth to use a _tentative test file for it, as we do elsewhere, until this is settled.

Thanks!

thiagowfx commented 11 months ago

Filed https://github.com/w3c/webdriver-bidi/issues/532.

whimboo commented 11 months ago

You can update the tests for wait=complete and wait=interactive, but at this point since wait=none is not agreed upon, it may be worth to use a _tentative test file for it, as we do elsewhere, until this is settled.

Thanks! I'll actually leave none out for now given that this test most likely might become racy due to different expectations in browsers. Instead I'll add a TODO item.