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 closing a browsing context #170

Closed
whimboo opened this issue Jan 31, 2022 · 8 comments · Fixed by #172
Closed

Specify closing a browsing context #170

whimboo opened this issue Jan 31, 2022 · 8 comments · Fixed by #172
Labels
module-browsingContext Browsing Context module

Comments

@whimboo
Copy link
Contributor

whimboo commented Jan 31, 2022

Beside issue #116 (PR #133) we would also need a command to close a browsing context.

@whimboo whimboo added the module-browsingContext Browsing Context module label Jan 31, 2022
@whimboo
Copy link
Contributor Author

whimboo commented Jan 31, 2022

We should be clear about what should happen when the last browsing context gets closed. I assume it will be similar to the WebDriver HTTP specification, so that the browser quits and the session will be closed?

CC @jgraham

@jgraham
Copy link
Member

jgraham commented Jan 31, 2022

This also needs to fail if the context isn't a top level browsing context.

juliandescottes added a commit to juliandescottes/webdriver-bidi that referenced this issue Feb 4, 2022
juliandescottes added a commit to juliandescottes/webdriver-bidi that referenced this issue Feb 5, 2022
juliandescottes added a commit to juliandescottes/webdriver-bidi that referenced this issue Feb 7, 2022
@juliandescottes
Copy link
Contributor

PR #172 was open to fix this, but there are two items we should discuss:

  1. What should happen when we close the last tab/window:
    • explicitly close the browser and end the session (same as webdriver classic?)
    • leave the decision to the implementation
    • do nothing but expose another command to close the browser (Browser.close ?)
  2. What should be the return value of browsingContext.close
    • Nothing
    • List or count of remaining top level browsing contexts
    • List or count of remaining windows

@juliandescottes
Copy link
Contributor

juliandescottes commented Feb 9, 2022

Another thing to consider: Puppeteer currently supports a runBeforeUnload parameter for Page.close: https://pptr.dev/#?product=Puppeteer&version=v1.20.0&show=api-pagecloseoptions

Internally it will switch between CDP's Page.close and Target.closeTarget depending on the value of this parameter, but maybe this is something we should handle directly in the BrowsingContext.close command for BiDi?

See also w3c/webdriver#1294

@juliandescottes
Copy link
Contributor

Quick note about CDP: testing CDP's Page.close, at least on Mac calling Page.close on the last tab doesn't close the Chrome application itself, although the behavior is probably different on Linux/Windows.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Specify closing a browsing context.

The full IRC log of that discussion <jgraham> Topic: Specify closing a browsing context
<jgraham> github: https://github.com//issues/170
<jgraham> jdescottes: Want to be able to close a browsing context. We have a PR, but there are some open questions.
<jgraham> jdescottes: How to handle closing last window? Should that always close the browser even if it doesn't have to (e.g. mac)? Or should it be implementation defined?
<jgraham> jdescottes: What should the command return? Classic command returns a list of open windows, should we do that?
<simonstewart> q+
<brwalder> q+
<jgraham> jdescottes: Puppeteer has the ability to run some code right before closing the window, do we need that capability?
<jgraham> ack simonstewart
<jgraham> simonstewart: Closing windows is an interesting problem. We left it so that if you want to close a session you call "quit", but if you close the final window we try to quit. So behaviour is defined in original webdriver and I suggest we follow that.
<jgraham> simonstewart: Running something before close: maybe? Could be done by allowing JS to execute on page load and that could register an unload event handler.
<jgraham> q?
<jgraham> ack brwalder
<jgraham> brwalder: With puppeteer it's a common pattern to run in headless mode and reuse a "browser context" and open/close within that context. It might be useful to be able to keep the browser alive when the last browser "window" is closed.
<simonstewart> q+
<jgraham> ack
<jgraham> ack simonstewart
<jgraham> simonstewart: Depends on the browser/WM. Some will probably quit the browser when there are no windows, so we can't rely on this.
<jgraham> brwalder: Could we say it's implementation dependent? For e.g. Edge in headful mode you should expect the browser to close when the last window closes, but for e.g. Chrome in headless mode the browser might still be alive.
<jgraham> simonstewart: I am worried about people depending on specific behaviour and not being able to port tests to other browsers. Maybe this could be a capability?
<jgraham> q+
<jgraham> simonstewart: A capability would make this deterministic
<jgraham> brwalder: That makes sense
<jgraham> ack jgraham
<jgraham> simonstewart: Predicatbility and determinism are important. Easy to force quit.
<jdescottes> q+
<jgraham> jgrhaam: I'm worried about regressing the performance of tests that depend on the no-quit-on-last-window-close behaviour of e.g. headless Chrome in Puppeteer. We should check with those teams if it's an acceptable regression or if they could use a workaround like keeping a dummy tab around.
<jgraham> simonstewart: What's the use case for keeping the browser open when the last window is closed?
<jgraham> brwalder: e.g. Puppeteer users open the browser but create a Page per test without quitting the browser.
<jgraham> simonstewart: Does BiDi session map to the Page or the process?
<jgraham> brwalder: Session in the spec maps to a browser process. Puppeteer Page is more like a tab.
<jgraham> ack jdescottes
<jgraham> jdescottes: Would the solution with exposing a capability work in all cases?
<simonstewart> q+
<jgraham> jgraham: We could use a capability, but it's not clear that people will pay attention to the capability value if they wouldn't also pay attention to e.g. an event indicating that the browser's closing after the window closed.
<jgraham> jgraham: Could also have "browserAboutTOQuit" in the return value if we wanted.
<jgraham> ack simonstewart
<jgraham> simonstewart: Thinking about parallelism. We want every call to new session to be a new isolated browser session. Could make each a new browser, or could partition a single browser process into multiple sessions. So as long as closing final window in the session closes the session that would be reasonable. Then you could support different behaviours in the same way, even if in some cases there is a
<jgraham> process that outlives the session.
<jgraham> brwalder: I agree the session doesn't have to map specifically to a process. I'm not sure what the best alternative is at this point.
<jgraham> jgraham: Our session currently do map to processes, so I don't want to depend on that not being 1:1 to support important use cases.
<jgraham> simonstewart: I think the safest thing to do is close the session when the final window is closed. Then if the process is still running you could start a new session for subsequent tests.
<jgraham> jgraham: There is still some overhead there, and it's different to classic WebDriver. But it would mean that you throw away all session state between tests, which could be better.
<simonstewart> q+
<jgraham> brwalder: Puppeteers' browser context is close to session. Those contexts are always explcitly closed. So this wouldn't support Puppeteer's existing behaviour, which could be a problem for them.
<jgraham> ack simonstewart
<jgraham> simonstewart: One of the reasons that WebDriver has Quit and Close is we wanted to make the distinction between being totally done with all of a session, and maybe retaining some state. For puppeteer is each Page an isolated instance of the browser? So apart from performance is it indistinguishable from shutting down the browser except in performance?
<jgraham> brwalder: Not quite, BrowserContext provides the state isolation. Page is like a tab. BrowserContext has many Pages. Page doesn't provide any isolated state.
<jgraham> BrowserContext is like a state container, holds cookies, localstorage etc.
<jgraham> simonstewart: In Selenium we tell people to open a new window. You could implement the puppeteer behaviour by having a window that doesn't get closed.
<jgraham> brwalder: Placeholder window would still be visible in API.
<jgraham> simonstewart: Do we think this is behaviour we should support? Is it just a performance optimisation?
<jgraham> brwalder: It's considered the correct way to use Puppeteer.
<jgraham> simonstewart: In Selenium people want to use a single session for multiple tests to retain state (e.g. cookies) and avoid starting a new browser process. It comes at the cost of breaking test isolation. Do we want to provide an easy mechansim to break isolation? Is BiDi new ssession in Chrome actually an isolated session?
<jgraham> brwalder: We want to make it possible to break isolation e.g. for setting up cookies. I didn't mean to imply that in Chromium a new BiDi session should reuse state. Each new session should have new state. Puppeteer doesn't have anything quite like a WebDriver session. Nearest is BrowserContext. WebDriver session should have isolated state.

@juliandescottes
Copy link
Contributor

juliandescottes commented Feb 10, 2022

No strong opinions for the return value (but some options for closing the last tab/window might make use of the return value to communicate the behavior to the consumer).
Regarding onBeforeUnload, no strong opinions either, could be done differently if needed (re-reading the transcript, maybe it was not clear that the proposal was to have a flag to skip before unload promps, not to run code at beforeunload).

Most of the discussion revolved around the behavior when closing the last tab/window.

One should read the conversation above for more context, but a short summary is that we want to enforce consistent behavior as much as possible with WebDriver BiDi, and closing the browser is the only consistent behavior that could be enforced by WebDriver. This is what WebDriver HTTP does. It was also mentioned that closing the browser encourages proper test isolation. But tools such as Puppeteer currently close the last tab/window in headless mode and expect the browser to stay alive, so that they can open a new tab/window for the next task. So doing exactly the same as WebDriver HTTP would be a regression here, or would force the tools to keep a tab open in the background to keep the browser alive.

The suggested alternative was that closing the browser could be left unspecified, and be an implementation detail. Instead the spec could just expect that the session should be closed when the last tab/window is closed. Meaning that a tool such as puppeteer would not have to restart the browser between tests but would still have to create a new session for each test.

@mathiasbynens @jschfflr hi! Do you have any feedback about the proposal above? If closing the last tab/window would close the session (but not necessarily the browser), how inconvenient would that be for you? Do you foresee performance issues with this?

@juliandescottes
Copy link
Contributor

I want to note that we also discussed an option based on capabilities, which after thinking about it outside of the meeting feels compelling to me.

By default, closing the last tab/window would always close the browser.
The user can request a capability such as keepBrowserAfterAllWindowsClosed.
If the target browser does not support this behavior, the session will not be created.
If it does, the session should be created successfully and the browser should not close when the last tab/window is closed.

The downside of this is that tests written for a browser with keepBrowserAfterAllWindowsClosed will not be portable to a browser which doesn't support it. But at least the default behavior would be consistent, so that you have to opt-in to expose yourself to inconsistencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-browsingContext Browsing Context module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants