w3c / webdriver-bidi

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

`browsingContext.navigationStarted` for the initial navigation to `about:blank` #766

Open sadym-chromium opened 2 months ago

sadym-chromium commented 2 months ago

Analogous to the initial browsingContext.load event, it can be confusing, especially if the browsing context is created, navigated to about:blank and then navigated to some other URL.

From discussion:

Hmm, so about:blank is tricky, and gecko are trying to change their behaviour so that the load is synchronous like Blink and WebKit. Given the current HTML spec, it's not clear if you can expect to observe a load event for the initial about:blank, so I think it makes some amount of sense to not have our tests depend on this tricky area of interop. I think for now it would be OK to just ignore any load event if the URL is about:blank, which I think would avoid the possible race without adding in yet another load.

Do we need those events for the initial navigation? If not, should we specify an exception?

sadym-chromium commented 2 months ago

We can mark those tests as tentative for now: https://github.com/web-platform-tests/wpt/pull/47958

whimboo commented 2 months ago

We cannot simply ignore all the navigations to about:blank (as proposed above) given that such a navigation can happen at any time by a test as well, which then has nothing to do with the initial navigation when the tab gets created.

But yes, we should discuss and maybe you can add it to the next meeting which probably will be the TPAC one this month?

sadym-chromium commented 2 months ago

We cannot simply ignore all the navigations to about:blank (as proposed above)

Let me clarify: about:blank can be a valid navigation, and I don't propose ignoring all the about:blank navigations. This issue is regarding only the initial navigation when the browsing context is created.

jgraham commented 2 months ago

I think in principle I'm happy with the idea of trying to ignore the navigation to initial about:blank so that from the user point of view it's an atomic part of the navigable being created. So no events relating to the load of that document, maybe also delaying the context created event until after it's loaded, and preventing any commands running in a context while any initial about:blank has not completed loading.

However the technical here are tricky, and I haven't looked in great detail.

OrKoN commented 2 months ago

Does the spec actually go through the navigation started hook when creating a new browsing context? I do not seem to be able to confirm that.

whimboo commented 2 months ago

Note that this issue also affects the browsingContext.contextCreated event. If emitted too early, clients may assume the tab is ready for interaction and begin sending commands, leading to potential errors like this one.

Which events should we actually emit, and when, during the browsingContext.create command? Here's a proposal:

Is there anything else we should consider to ensure a smooth and error-free interaction? What are your thoughts on this approach?

css-meeting-bot commented 1 month ago

The Browser Testing and Tools Working Group just discussed Emitting Events During "browsingContext.create" when "about:blank" is loaded, and agreed to the following:

The full IRC log of that discussion <AutomatedTester> topic: Emitting Events During "browsingContext.create" when "about:blank" is loaded
<AutomatedTester> github: https://github.com/w3c/webdriver-bidi/issues/766
<AutomatedTester> scribe+
<AutomatedTester> whimboo: this issue was origianly raised by sadym
<AutomatedTester> ... this is about getting an event when the context being created and then when can we navigate
<orkon> q+
<AutomatedTester> ... so the question is what do we want to do with the creation of new tabs/documents?
<AutomatedTester> ... do we want to handle navigationStarted for about:black?
<AutomatedTester> ack orkon
<AutomatedTester> orkon: I checked the html spec and I couldn't see if there is a navigationStarted with about:blank
<AutomatedTester> ... perhaps we need to reverse the test so it doesn't have the event on about:blank
<AutomatedTester> q?
<whimboo> q+
<AutomatedTester> ack whimboo
<AutomatedTester> whimboo: we are happy to change this
<AutomatedTester> ... and this way we can improve the clients so they have a better idea when to work with the tab
<jdescottes> q+
<AutomatedTester> ... and I am not sure about the implementation status of these things in chrome and if/when it will be ready
<AutomatedTester> ack jdescottes
<AutomatedTester> jdescottes: when I was testing this in Firefox we weren't running events for navigation on about:blank
<orkon> q+
<AutomatedTester> ... if there are any scenario where we have any extra events in the browser?
<AutomatedTester> ack orkon
<orkon> https://github.com/web-platform-tests/wpt/pull/47958/files#diff-c773a239b93f48290e8f834f6fd9c337720fb92b206b65e03ce1c64633789032
<jdescottes> q+
<AutomatedTester> whimboo: we fire the created event early... this is a combined question of should we send anything out before the creation of the tab
<AutomatedTester> orkon: there are wpt tests that I have linked
<AutomatedTester> ... [describes test]. This test wasnt passing in chrome but in Firefox
<AutomatedTester> q?
<AutomatedTester> ack jdescottes
<AutomatedTester> jdescottes: thanks to the link. THe item about the creation event coming out too early... I think we should talk about this.
<gsnedders> q?
<sadym> This is the mentioned test: https://github.com/web-platform-tests/wpt/pull/47958
<AutomatedTester> ... in firefox we have changed the way that we emit events so that we do it after the creation of the context
<AutomatedTester> ack gsnedders
<orkon> q+
<AutomatedTester> gsnedders: Are you saying the load is async in firefox? I thought you could always access the document in the iframe in a sync way
<AutomatedTester> whimboo: this is for the new top level browser context
<AutomatedTester> gsnedders: ok, that wasn't clear in the issue
<AutomatedTester> ack orkon
<AutomatedTester> orkon: looks like we're talking about 2 different issues. This is about the browserContext creation with about:blank
<sadym> https://github.com/web-platform-tests/wpt/pull/47958/files#diff-c773a239b93f48290e8f834f6fd9c337720fb92b206b65e03ce1c64633789032R14-R29
<AutomatedTester> ... in firefox there are navigationStarted events being fired in about;blank
<AutomatedTester> sadym: I've linked to the the test
<AutomatedTester> q?
<AutomatedTester> jgraham: for the case of initial about:blank we should treat it like it is sync
<AutomatedTester> ... e.g. create a new tab with about:blank you can start interacting with it straight away
<jgraham> jgraham: and not emit a navigation started event
<AutomatedTester> gsnedders: is there a use case where that is not about:blank?
<AutomatedTester> [discussion about checking we are all started from about:blank and navigationStarted events]
<AutomatedTester> whimboo: firefox has 2 events emited for about:blank. a sync and an async event
<orkon> q+
<simonstewart> q?
<AutomatedTester> ... and we should treat new window creation special if there is a URL in there
<AutomatedTester> whimboo: how would window creation handle this?
<AutomatedTester> ack orkon
<AutomatedTester> orkon: the html spec says there are some special treatment for about blank. e.g. if it has params we need to update the history
<AutomatedTester> whimboo: this should be possible for us to do this and not emit the event
<AutomatedTester> q?
<AutomatedTester> ACTION: sadym to update test to hsow that events are not emited on about:blank
whimboo commented 1 month ago

Note that we are going to update the behavior of Firefox over on https://bugzilla.mozilla.org/show_bug.cgi?id=1922014.

whimboo commented 1 month ago

@sadym-chromium can you please update the related tests so that they actually behave as expected? Thanks.

sadym-chromium commented 1 month ago

@sadym-chromium can you please update the related tests so that they actually behave as expected? Thanks.

I started working on it in https://github.com/web-platform-tests/wpt/pull/48651