Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support user contexts #570
Support user contexts #570
Changes from 38 commits
5259fa5
8cee793
84db4ee
d8ffee7
44341cf
8ab3f0b
4a4a0ca
59e608e
5f98005
f7c612d
a7a71a2
1a0c963
aa2c0ec
59d2c51
32b8efd
4733bd5
ff8b3f1
45aa6db
fc94b9f
3e7a0ec
5816e9e
bc19673
4a4aaa6
ded90b6
2c0c421
a30c574
9342771
d4976cf
4f93268
492089e
4e1af81
49f7ba2
7f221d1
e384195
af0d0c2
8a2bb4b
2d1a835
93acadb
88196bd
3542e7d
1fcda96
0db4d0d
0336819
253d3eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Firefox, we can open have tabs from several user contexts (containers) next to each other in the same window. I imagine it's fine to enforce this limitation if other vendors don't have this flexibility.
Does that mean that in Chrome, if you have 1 window with 1 tab in the default context and you use
browsingContext.create
with type=tab, it will force creating a new window on the fly? In that case it's not compliant with the following step of the type=tab section:So I'm wondering if we really need to be strict and fail here. We could modify the text of the "if type is tab" section below to say we will "attempt" to reuse an existing window (containing the reference context, if relevant), but it's not guaranteed.
cc @jgraham
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is not supported with Chrome.
Yes, it will create a tab in a new window. I think "should" allows for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah maybe, I thought the "should" was just there to cover the case where there is no existing window ("if any"). But maybe this also covers the cases where existing windows are not compatible with the tab we are trying to add.
The spec about the reference context uses the same phrasing with "should":
So if the "window containing the reference context" is not compatible with the tab we are trying to add, then it should be fine to add the tab in another window?
Again, not sure we should throw here, if the spec is flexible enough to allow to pick another window if needed?
Since nothing in Firefox prevents from having tabs from several containers in the same window, it seems a bit arbitrary to throw. Imagine we have 1 window with 1 tab "tabA" owned by "contextA". If someone uses "browsingContext.create(type=tab, userContext=contextB)" with no reference context, Firefox will add this tab next to tabA. But if the user passes referenceContext=tabA, we would throw because contexts don't match?
If we really want to have a consistent behavior across all browsers, I think we need to enforce that all tabs in a window all have the same user context, but that seems difficult/impossible to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"should" in specs is defined as "unless there's a valid reason not to". To me the "should reuse the window containing the reference context" is fine; it more or less macro expands to "shall reuse the window containing the reference context, unless there's a valid reason not to", and "we don't support multiple user contexts in the same window" seems like potentially a valid reason.
I also agree that Firefox in general can't enforce the condition that each OS window only gets tabs with a single user context and therefore we can't in WebDriver. For example we might connect to a browser where the user already created multiple containers in the same window.
Given that implementation differences seem impossible to avoid here, I think it might be OK to just allow whatever behaviour the implementation does? In practice clients can always request a new window when creating a tab in a different context, so for test automation purposes it's possible to build a stable, interoperable, foundation despite the underlying differences.