-
Notifications
You must be signed in to change notification settings - Fork 32
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
Spec: Wait for network revocation in nested fenced frames before disabling network. #176
Open
blu25
wants to merge
37
commits into
master
Choose a base branch
from
liam-nested-revocation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
c9cbe3f
disableUntrustedNetwork skeleton
39d31fb
fill out more
7eaf2b1
more
c4705a6
address comments
ab9785d
skeleton
d75f324
content done % links
294e5fd
Merge remote-tracking branch 'origin/revoke-network' into liam-nested…
blu25 7c89c4e
Update spec.bs
blu25 0ce475f
remove 'defaults to'
blu25 2fc1484
Merge branch 'master' into liam-nested-revocation
blu25 c92fa38
address comments for code that's new in this PR
blu25 d94859f
Update spec.bs
blu25 9b6e158
attempt to fix validation errors
blu25 5d982b3
attempt to fix validation errors
blu25 f760c12
Merge branch 'master' into liam-nested-revocation
blu25 44435e0
test if removing the note fixes the build
blu25 78d1673
attempt to fix validation errors
blu25 77e3bd4
Update spec.bs
blu25 f5b1255
remove "can disable untrusted network"
blu25 a51ffdd
convert same-origin check to assert
blu25 5316240
update credentialless issue link
blu25 a24cd13
Merge branch 'master' into revoke-network
blu25 62f3b43
add changes from other review
blu25 49fe6a2
address comments
blu25 eb6bae4
Merge branch 'revoke-network' into liam-nested-revocation
blu25 9de235e
Update spec.bs
blu25 aabfad3
Merge branch 'master' into liam-nested-revocation
blu25 2c067b0
address review comments
blu25 19b33b4
Merge branch 'master' into liam-nested-revocation
blu25 95cc597
clean up merge issues
blu25 38d1e66
clean up and move note to definition
blu25 7a0e92a
Merge branch 'master' into liam-nested-revocation
blu25 f30fc25
address comments
blu25 eaf68b0
call out what is being stored in navigablesWithNetworkChildren
blu25 f476341
address review comments
blu25 adfb311
address review comments
blu25 589ca57
add iframe call and run in parallel
blu25 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Do we also have to do this during iframe teardown? I don't think so but I want to be sure.
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.
Only if the iframe teardown results in a child fenced frame also being torn down, but that will get caught by this anyway, so we don't need to add anything for iframes.
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.
Wait how will it get caught by this? Check out the iframe destruction/removal path — I don't think it results in fenced frames being "removed" from the DOM, so I don't think we'd go through this path for fenced frames if their iframe parent is removed.
Also, note that the recalculate algorithm requires being in parallel, but the removal steps are not called from in parallel. That's a bug.
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.
I guess I had assumed it was a cascading operation, but it looks like it's not. I'll add a separate
removed from a document
algorithm for iframes. I didn't see any place where this was being called for iframes to begin with, so it should be okay to add a new algorithm.I'm not sure if there's some common path that would be better to put this in. I considered the node removal algorithm, but that lives in the DOM spec, and I don't think that's supposed to get access to the concepts of traversables and node navigables. Let me know if there's a better spot you think this could live in.
I also updated the calls to be run in parallel.
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.
It's already being called here: https://html.spec.whatwg.org/C#the-iframe-element:html-element-removing-steps. It's using the more appropriate removing steps hook, instead of the removed from document hook, which I have a goal of removing from the HTML spec anyways, so I don't think we should add more instances of it sadly.
So I guess we should just monkeypatch the existing iframe algorithm, and as a follow-up, some day we can probably modify our spec to not use the removed from document hook.