w3c / webdriver-bidi

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

Update `browsingContext.create` to require the new context to be active (in the foreground) #475

Closed OrKoN closed 1 year ago

OrKoN commented 1 year ago

Currently, Firefox implementation creates contexts in the background and Chrome implementation in the foreground. From the user perspective, I believe it makes sense that the new window/tab is focused and in the foreground just like when the user opens a tab manually. Plus it will help avoid a bunch of surprises due to throttling. browsingContext.activate would help with that but strictly speaking is not related to this issue as the foreground creation should be the default.

Related bugs:

OrKoN commented 1 year ago

cc @jgraham @whimboo

jgraham commented 1 year ago

I suggest we make this an option (and maybe don't even have a default, just require it). Something like activate: bool in the definition of thebrowsingContext.create command.

jgraham commented 1 year ago

Although again, you need to think through what happens in the case that you create a context in a window that doesn't have system focus (e.g. one that's minimized). Are you expecting the window to be unminimized if you create an active context, or are you expecting that it will become the active tab if the window is separately unminimized?

OrKoN commented 1 year ago

I would expect this to match the user-observed behaviour by default: if the user clicks File > new Window, the window is maximized (if it was minimized) and brought to the front (I tested and that is the behaviour in Chrome and Firefox on MacOS.). As far as I know, the only case when the context is created in the background by the user is the Cmd + Click on links. Perhaps, if it is configurable in the browsers, it would make sense to use the default browser behaviour and allow changing it via capabilities/profile folders or the parameter on the create command.

What is the motivation for creating the contexts in background by default?

jgraham commented 1 year ago

"Matches the user-observed behaviour" is really hard to define. For example the default user-observed behaviour of "open link in new tab" on desktop and mobile Firefox is to open it in a background tab. On the other hand opening in a new window (on desktop) does focus the window in the default configuration, similar to opening a new blank tab.

But all of these things are subject to change, and WebDriver allows possibilities that aren't open to users (e.g. opening a new tab in a minimised window on non-mac platforms), plus, if the behaviour is observable, being consistent between browsers is more important than being like the UI.

WebDriver classic tried to avoid this problem by claiming that browsers under automation should act as if OS-level focus wasn't important. That doesn't quite work for BiDi, so instead we need to allow authors to get consistent, defined behaviour, whilst also not restricting what they can do. In that case having a flag in the API, and precisely defining what happens with each value, seems like the way forward.

thiagowfx commented 1 year ago

and maybe don't even have a default, just require it

This is a breaking change, absolutely everywhere. Most WPT and local tests will break, all at once. Let's start with adding a default and then we can bikeshed whether to make it explicitly set by the user or not at some other point.

jgraham commented 1 year ago

Fixing wpt tests would probably be a one line change, since they could provide some default. But sure, I'm also happy with a boolean flag activate whose default is false (since we decided that boolean flags should always default to false, and it makes sense that the default behaviour is to do the minimal thing, which would be to not adjust the focus).

OrKoN commented 1 year ago

"Matches the user-observed behaviour" is really hard to define. For example the default user-observed behaviour of "open link in new tab" on desktop and mobile Firefox is to open it in a background tab. On the other hand opening in a new window (on desktop) does focus the window in the default configuration, similar to opening a new blank tab.

I agree but also I want to point out that there might be (rare) test scenarios when the clients wants to test how the app works with such browser-specific behaviours (since there are differences in behaviour between background and foreground contexts). Perhaps, the default value should be "do whatever the browser normally does" (hard to define though) plus activate=true|false. Also, I don't view the "open link in new tab" as a complete equivalent to browsingContext.create (the browsingContext.create does not accept a URL, for example) but rather as an equivalent of the "new tab/window" action. Another alternative, perhaps, could be a parameter for browsingContext.create called background with the default value false.

Another aspect to discuss is whether creating a context with activate=true would be equivalent to creating the context in the background and calling browserContext.activate right after. If there is no practical difference and we don't want to change the default value, then we could just expect the client to activate the context.

jgraham commented 1 year ago

Unfortunately "do whatever the browser normally does" is an interop nightmare, and for WebDriver[-BiDi] in particular the likely consequence is that tests can't be ported easily between different browsers. So I think we need to specify a default here.

I believe that activate=true would most likely be the equivalent of just calling activate immediately after creating the tab. So yes, I think the minimal spec change here would be to enforce the behaviour that the tab is not activated when just calling browsingContext.create, and allow clients to call activate manually if that's what they want. This could be optimised in the future if we find that it's a common use case.

OrKoN commented 1 year ago

This could be optimised in the future if we find that it's a common use case.

Puppeteer creates all new contexts in the foreground by default so we will need to call it every time. Also, Puppeteer does not even provide an option to create contexts in the background and we have not received user requests about that so far. I wonder if other tools need the background creation.

css-meeting-bot commented 1 year ago

The Browser Testing and Tools Working Group just discussed Update browsingContext.create to require the new context to active (in the foreground).

The full IRC log of that discussion <jgraham_> Topic: Update browsingContext.create to require the new context to active (in the foreground)
<jgraham_> github: https://github.com/w3c/webdriver-bidi/issues/475
<jgraham_> ack orkon
<jgraham_> orkon: The default is to create new contexts in the background, which creates problems due to throttling. We could either change the default, or we could require clients to activate the context when they do something meaingful with it. Opinions on what should happen? Gecko focuses the new one and then the original one. Background by default seems harder.
<orkon> jgraham_ (IRC): perhaps we can parametrize it in the command, activate or background
<orkon> jgraham_ (IRC): with the original webdriver the assumption was that the background window should always work which may or may not be possible
<orkon> jgraham_ (IRC): the original point was that you need to be able to do keyboard input
<orkon> jgraham_ (IRC): preferred would be to have a parameter
<jgraham_> q?
<orkon> q+
<jgraham_> ack orkon
<jgraham_> orkon: My preference is that we have a `background` parameter, so that contexts by default open in the foreground. This matches the user expectation in more cases. If we wanted background to be default, we could require users to send a second command to activate.
<jgraham_> orkon: Browsers don't really work with all tabs having focus like classic webdriver requires. Chrome headless implemented this, but it doesn't work for the headful browser.
<JimEvans> q+
<orkon> jgraham_ (IRC): we had a discussion that default values should be always false. So I think background parameter seems fine if we think that the foreground by default is the right thing
<jgraham_> ack JimEvans
<orkon> Jim Evans: it is worth noting that for implementations that I looked at when you say new tab that this tab is brought up in the foreground. So I think that should inform what the default should be here.
<jgraham_> q?
<orkon> jgraham_ (IRC): depending on how you open the tab e.g., via open a link in an new tab it might not be true. So having a parameter does make sense. Default for the foregound is fine