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

Fix references to html after navigation and session history rewrite #580

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

antosart
Copy link
Member

@antosart antosart commented Nov 28, 2022

This PR fixes broken references to html, which were mostly a result of the navigation and session history rewrite.

In particular, this change:

  • replaces browsing contexts with navigables (fixes Should we replace browsing contexts with navigables? #579).
  • drops the sandbox initialization part for documents, since it is now included in html,
  • rewords the part on plugins, since the html no longer defines plugin documents, but only considers pdfs.

This fixes #576.

@@ -3467,35 +3474,32 @@ this algorithm returns normally if compilation is allowed, and throws a
</h5>

This directive's <a for="directive">initialization</a> algorithm is
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we actually get rid completely of the initialization hook (and move the worker part to html, too)?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that would make things simpler, but I'm totally willing to believe that it would? Put up a patch to talk through?

Copy link
Contributor

Choose a reason for hiding this comment

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

That does seem nicer!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I filed #581. I'll take a stab at it when I have time.

index.bs Outdated
former two (also including navigations). This is true even when the data is
semantically equivalent to content which would otherwise be restricted by
another directive, such as an <{object}> element with a `text/html` MIME
type.

Note: When a plugin resource is navigated to directly (that is, as a <a>plugin document</a> in the
<a>top-level browsing context</a> or a <a>nested browsing context</a>, and not as an embedded
Note: When a plugin resource is navigated to directly (that is, as a <a>plugin</a>
Copy link
Member Author

Choose a reason for hiding this comment

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

The part about plugins should probably be rewritten to take into account that we only have pdf left. I guess we can do it in a follow-up change.

Copy link
Member

Choose a reason for hiding this comment

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

This is generally true, but I think Chromium at least also has weird things NaCL modules in extensions (behind an enterprise policy).

@antosart antosart requested a review from mikewest November 28, 2022 08:19
@antosart
Copy link
Member Author

cc @domenic

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
text: content security policy state; url: attr-meta-http-equiv-content-security-policy
text: create and initialize a new document object; url: initialise-the-document-object
text: initializing a new Document object; url: initialise-the-document-object
text: prepare the script element; url: prepare-the-script-element
text: container document; for: navigable; url: nav-container-document
text: CSP-derived sandboxing flags; url: csp-derived-sandboxing-flags
Copy link
Member

Choose a reason for hiding this comment

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

These should probably be exported then?

Copy link
Member Author

Choose a reason for hiding this comment

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

The navigable container document makes sense (whatwg/html#8556). For the CSP-derived sandboxing flags I am not sure. It looks more like an internal html thing, and we reference it here only to recall that html takes care of that. Should the definition be exported in such a case?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not. Those cases are somewhat tricky. You could link those inline using an explicit spec=html attribute I think.

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me, but I'll defer to @domenic since he's the expert on the navigation work.

index.bs Outdated Show resolved Hide resolved
@@ -3467,35 +3474,32 @@ this algorithm returns normally if compilation is allowed, and throws a
</h5>

This directive's <a for="directive">initialization</a> algorithm is
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that would make things simpler, but I'm totally willing to believe that it would? Put up a patch to talk through?

index.bs Outdated
former two (also including navigations). This is true even when the data is
semantically equivalent to content which would otherwise be restricted by
another directive, such as an <{object}> element with a `text/html` MIME
type.

Note: When a plugin resource is navigated to directly (that is, as a <a>plugin document</a> in the
<a>top-level browsing context</a> or a <a>nested browsing context</a>, and not as an embedded
Note: When a plugin resource is navigated to directly (that is, as a <a>plugin</a>
Copy link
Member

Choose a reason for hiding this comment

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

This is generally true, but I think Chromium at least also has weird things NaCL modules in extensions (behind an enterprise policy).

@antosart antosart force-pushed the fix-html-refs branch 2 times, most recently from 3eeed87 to 1bc6ac4 Compare November 29, 2022 06:46
Copy link
Contributor

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM modulo the reference stuff being slightly nicer with for=html than via adding to the anchors block.

index.bs Outdated Show resolved Hide resolved
@antosart
Copy link
Member Author

antosart commented Dec 1, 2022

Thanks, suggestions implemented!

@antosart antosart merged commit 947d892 into w3c:main Dec 1, 2022
@antosart antosart deleted the fix-html-refs branch December 1, 2022 10:01
github-actions bot added a commit that referenced this pull request Dec 1, 2022
…580)

SHA: 947d892
Reason: push, by antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
domenic pushed a commit to whatwg/html that referenced this pull request Dec 1, 2022
pmeenan pushed a commit to pmeenan/html that referenced this pull request Dec 2, 2022
pmeenan pushed a commit to pmeenan/html that referenced this pull request Dec 2, 2022
noamr pushed a commit to noamr/html that referenced this pull request Jan 17, 2023
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.

Should we replace browsing contexts with navigables? Broken references in Content Security Policy Level 3
4 participants