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

Introduce navigable's config instance #80

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

domfarolino
Copy link
Collaborator

@domfarolino domfarolino commented Apr 25, 2023

I think this PR makes things better and more explicit, but I believe things are still broken even after this. A few questions/points:

  • In the Fence methods, should it ever be possible for instance to be null, as we check for in step 2? I think the instance that we check when retrieving the Fence object in the first place (https://wicg.github.io/fenced-frame/#dom-window-fence > Step 2) should be a sufficient gate, such that we don't have to check for instance anymore inside the methods, right? The way I envision the ideal world here, is that because we have to support URN iframes, we make the "instance" retriever in both the window.fence getter and the fence.* methods both traverse upwards until they find a navigable with a non-null instance and both reference that instance. Does that sound right?

Preview | Diff

@domfarolino domfarolino requested a review from gtanzer April 25, 2023 13:15
@gtanzer
Copy link
Collaborator

gtanzer commented Apr 25, 2023

@domfarolino

The way I envision the ideal world here, is that because we have to support URN iframes, we make the "instance" retriever in both the window.fence getter and the fence.* methods both traverse upwards until they find a navigable with a non-null instance and both reference that instance. Does that sound right?

Yes, that sounds right. There would be some algorithm "Get the current fenced frame config instance" which is implemented as that for now, and later becomes simpler.

@domfarolino
Copy link
Collaborator Author

OK, let's implement that in a PR just after this one to fix all of this mess. I'm just trying to land this for #74.

@gtanzer
Copy link
Collaborator

gtanzer commented Apr 25, 2023

In the Fence methods, should it ever be possible for instance to be null, as we check for in step 2? I think the instance that we check when retrieving the Fence object in the first place (https://wicg.github.io/fenced-frame/#dom-window-fence > Step 2) should be a sufficient gate, such that we don't have to check for instance anymore inside the methods, right?

And yes, this also sounds right.

@domfarolino domfarolino merged commit 4686c12 into master Apr 25, 2023
@domfarolino domfarolino deleted the domfarolino/navigable-config-instance branch April 25, 2023 14:06
github-actions bot added a commit that referenced this pull request Apr 25, 2023
SHA: 4686c12
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants