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

Allow applying preload scripts to opened contexts #803

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Oct 25, 2024

This change allows specifying that a specific preload script should apply not only to the specified contexts and but also to all contexts opened by the specified contexts (recursively).

Issue #756


Preview | Diff

@OrKoN OrKoN force-pushed the orkon/preload-script-inheritance branch from 377aa58 to b13c1de Compare October 25, 2024 12:41
@OrKoN OrKoN marked this pull request as ready for review October 25, 2024 12:43
@OrKoN OrKoN added the needs-discussion Issues to be discussed by the working group label Oct 25, 2024
@whimboo
Copy link
Contributor

whimboo commented Oct 25, 2024

This needs new tests as well, right?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
1. If |preload script|'s <code>contexts</code> [=list/contains=] |navigable id|,
set |is script run allowed| to true and break.

1. If |preload script|'s <code>applyToOpeningContexts</code> is true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a small detail, I'd prefer this to be 1. If |inherit| is false then break. 1. Set |navigable to |navigable|'s original opener.

But I also feel like this doesn't quite capture the invariant that each preload script is defined exactly once so has a single setting for whether it's inherited. So conceptually if we're not inheriting we're compaing against a set containing a single value, whereas if we are inheriting we're comparing against a set that contains all the values from openers. I think making that structure explicit would make the spec easier to read.

As a different thought: if this applies to preload scripts it should also apply to event subscriptions in the same way, right? Even if we don't do it in the same PR, it could be worth checking if the same logic works in that case, or if we want to hook in to the context creation lifecycle and update the underlying datastructures to explicitly add the new context, so the getters are still doing the same lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to apply it to the event subscriptions and, based on the current feedback from the clients, to the viewport configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically anything that's understood as browser configuration rather than interacting with a specific document.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Allow applying preload scripts to opened contexts.

The full IRC log of that discussion <AutomatedTester> topic: Allow applying preload scripts to opened contexts
<AutomatedTester> github: https://github.com//pull/803
<AutomatedTester> orkon: this is one of the. topics that was brought up by the playwright folks
<jgraham> q+
<AutomatedTester> ... this is the PR that prepared to implements inheritance for preload scripts
<sadym> q+
<AutomatedTester> ... I wonder if there are concerns with this approach and would this have an issue with the events being sent out?
<AutomatedTester> ack next
<AutomatedTester> jgraham: I think we shouild do this
<AutomatedTester> ... we should be looking to add global configuration or top level traversibles be documented
<AutomatedTester> ... I agree we should be doing this but I think we need to be aware of all the different ways that we should bve doing this
<AutomatedTester> q?
<AutomatedTester> ack next
<AutomatedTester> sadym: this looks like a new mechanism. I wonder if this playwright appraoch could not be handled with using preload script globally
<AutomatedTester> jgraham: I think it could but that might be harder
<AutomatedTester> ... people would want it a per context and I think we want it inheritable. They are both required
<AutomatedTester> q?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-script Script module needs-discussion Issues to be discussed by the working group needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants