-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add support for "userContexts" argument to "browsingContext.setViewport" command #851
Comments
In retrospect it's a bit unfortunate that we put this command in |
I think it might be better to add viewport as an attribute (defaultViewport) for the creation of user contexts unless there is a use case for changing viewport for multiple contexts at once (which I do no see). |
I think we need a coherent plan for how the group of related things (e.g. viewports, geolocation emulation, timezone overrides, preload scripts, event subscriptions) will work. My concern with having creating a user context allow specifying the viewport configuration is that if we apply the same pattern elsewhere we'll end up with duplication for all the things that we want to configure; one command for targeting a specific context or set of contexts, and one property on user context creation. Or alternatively we'll have a mixed model where some things like viewport overrides are specified when creating a user context, but other things that people want to apply to an entire user context like preload scripts, or event subscriptions, are separate commands. The pattern we've adopted so far is to avoid single "configure all the things" commands (except capabilities, which I think have turned out to be a generally problematic model beyond the use case of picking a browser instance), and had single commands per configuration option. So my prior is that's the model we should keep in the future. |
I generally I agree but I also think changing viewport in multiple existing browsing contexts might get somewhat complicated w.r.t. the background tab throttling (I could be overthinking). |
PR: #876 |
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<tidoust> Topic: Support for "userContexts" argument to "browsingContext.setViewport" command<tidoust> github: https://github.com//issues/851 <tidoust> Alexandra: Would be good to have pre-setup viewports for all browsing contexts. I sketched a proposal for this. There was some concerns on the design. I thought it would be good to bring it here for discussion. <orkon> q+ <tidoust> ack orkon <jgraham> q+ <tidoust> orkon: Still need to review the PR on our side. It affects open navigables. That's a side effect. I guess it's not a big concern but maybe we should clarify whether we should activate, etc. <tidoust> ... I don't think it's a big concern, it seems fine to proceed. <tidoust> ack jgraham <tidoust> jgraham: If it seems fine, let's do it. On the general conversation on attaching user contexts, you can already do that one context at a time. <tidoust> ... I don't think it's making anything new in that case, just making things easier when there are many browsing contexts. <sadym> q+ <tidoust> ... Useful for when running multiple tests with a given context, e.g., in mobile mode, etc. That helps set things up initially, and not having to worry about that at runtime. <tidoust> ack sadym <sasha> q+ <tidoust> sadym: I wonder how we will specify what happens when a new browser context is created. We'll have a viewport for it. Do we have a hook? <tidoust> ack sasha <sadym> s/browser/browsing <tidoust> sasha: Another good topic to discuss. Roughly the moment when we detect the round creation. Pretty early when we have the contextcreated event. <tidoust> ... Ideally, we would have a hook for that. <tidoust> ... Early enough, but not sure exactly when. <tidoust> ... If people have additional feedback on when that should happen, let me know. I'll draft something for further discussion. <tidoust> jgraham: It should probably be tied to document creation. <tidoust> ... I need to look into details. <tidoust> ... Otherwise we could actually update HTML. <tidoust> ... I suspect it's not observable. |
Similar to other commands (e.g. session.subscribe) we should add a possibility to set viewport settings to all existing and future browsing contexts belonging to certain
userContexts
.The text was updated successfully, but these errors were encountered: