-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Integrate Feature Policy concepts into HTML #3287
Conversation
This commit introduces the feature policy for Document objects, adds the 'allow' attribute to iframe elements, and reframes 'allowfullscreen', 'allowpaymentrequest' and 'allowusermedia' in terms of feature policy. Document allow* flags are removed, as they are no longer referenced. The 'allowed to use' algorithm is also updated to call into the feature policy 'Is feature enabled' algorithm, and rewritten to take a policy- controlled feature as an argument rather than an attribute, so that other specs can also use it to control other features.
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.
Thanks for the PR. Here is some early feedback. I hope that @zcorpan can also review as he worked on the predecessors of allow quite a bit. I'm a little concerned with the changes in semantics, but if those can somehow be proven to be compatible it would be a nice cleanup for sure.
source
Outdated
the <code>iframe</code> element's <span>browsing context</span> are to be allowed to use <code | ||
data-x="dom-element-requestFullscreen">requestFullscreen()</code> (if it's not blocked for other | ||
reasons, e.g. there is another ancestor <code>iframe</code> without this attribute set).</p> | ||
the <code>iframe</code> element's <span>browsing context</span> should be initialized with a |
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.
Since this is a non-normative description it should not use "should".
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 thought there was a distinction between 'should' and 'SHOULD', but if not, then this should change.
How about 'will', as in:
When specified, it indicates that
Document
objects in theiframe
element'sbrowsing context
will be initialized with a feature policy which allows the fullscreen feature to be used from any origin.
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.
Changed to will, thanks.
source
Outdated
@@ -29740,18 +29783,29 @@ interface <dfn>HTMLIFrameElement</dfn> : <span>HTMLElement</span> { | |||
<p>The <dfn><code data-x="attr-iframe-allowpaymentrequest">allowpaymentrequest</code></dfn> | |||
attribute is a <span>boolean attribute</span>. When specified, it indicates that | |||
<code>Document</code> objects in the <code>iframe</code> element's <span>browsing context</span> | |||
are to be allowed to use the <code>PaymentRequest</code> interface to make payment requests.</p> | |||
should be initialized with a <span data-x="concept-document-feature-policy">feature policy</span> |
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.
Same here.
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.
Done.
source
Outdated
the <code>iframe</code> element's <span>browsing context</span> are to be allowed to use <code | ||
data-x="dom-MediaDevices-getUserMedia">getUserMedia()</code> (if it's not blocked for other | ||
reasons, e.g. there is another ancestor <code>iframe</code> without this attribute set).</p> | ||
the <code>iframe</code> element's <span>browsing context</span> should be initialized with a <span |
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.
And here.
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.
Done.
source
Outdated
data-x="attr-iframe-allowusermedia">allowusermedia</code> attributes only take effect when the | ||
<span>nested browsing context</span> of the <code>iframe</code> is <span | ||
data-x="navigate">navigated</span>. Adding or removing them has no effect on an already-loaded | ||
page.</p> |
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.
This is an incompatible change per #2143 (comment). Is that what Chrome now implements? We used to take dynamic changes into account for at least allowfullscreen
.
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.
Chrome has been following this since M62 (Released in beta September 14, stable October 17). The original discussion to implement and ship was at https://groups.google.com/a/chromium.org/d/topic/blink-dev/UKChYGBYvSk/discussion (Referencing #2187 in this repo)
source
Outdated
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> | ||
<li><dfn data-x="concept-container-policy" data-x-href="https://wicg.github.io/feature-policy/#container-policy">Container Policy</dfn></li> | ||
<li><dfn data-x="concept-serialized-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#serialized-feature-policy">Serialized Feature Policy</dfn></li> | ||
<li>The <dfn data-x-href="https://wicg.github.io/feature-policy/#initialize-for-global">Initialize global’s Feature Policy from response</dfn> algorithm</li> |
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.
This should be renamed given that it applies to documents. That's not a global.
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.
Agreed. This is changed to #initialize-for-document
in w3c/webappsec-permissions-policy#104. I'll merge that one first once it matches the changes here, and update this PR accordingly.
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.
Done now.
source
Outdated
<li>The <dfn data-x="is-feature-enabled" data-x-href="https://wicg.github.io/feature-policy/#is-feature-enabled">Is feature enabled by policy for origin</dfn> algorithm</li> | ||
<li>The <dfn data-x="process-feature-policy-attributes" data-x-href="https://wicg.github.io/feature-policy/#process-feature-policy-attributes">Process feature policy attributes</dfn> algorithm</li> | ||
</ul> | ||
|
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.
This newline can go.
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.
Done
source
Outdated
|
||
<pre><strong><iframe src="https://maps.example.com/" allow="geolocation"></iframe></strong> | ||
</pre> | ||
|
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.
This newline can go.
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.
Done.
source
Outdated
</div> | ||
|
||
<p class="warning">Because they only influence the feature policy of the <span>nested browsing |
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 you mean nested browsing context's active document here? Also, link "feature policy".
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.
Yes; added links to "active document" and "feature policy".
source
Outdated
data-x="attr-iframe-allowusermedia">allowusermedia</code> attributes only take effect when the | ||
<span>nested browsing context</span> of the <code>iframe</code> is <span | ||
data-x="navigate">navigated</span>. Adding or removing them has no effect on an already-loaded | ||
page.</p> |
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.
page -> document
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.
Done (this was following the wording on the very similar warning for sandbox
, which should probably be changed too)
data-x="dom-iframe-sandbox">sandbox</code></dfn> must <span>reflect</span> the respective content | ||
data-x="dom-iframe-name">name</code></dfn>, <dfn><code | ||
data-x="dom-iframe-sandbox">sandbox</code></dfn>, and <dfn><code | ||
data-x="dom-iframe-allow">allow</code></dfn> must <span>reflect</span> the respective content |
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 think it would be nicer for feature testing if allow ended up reflecting the internal CSP-like data model. That would also give us an additional hook to make sure UAs actually implement it correctly.
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.
We're looking at adding a policy
member to iframe DOM elements (though not an html attribute) to be able to introspect the policy that is being applied to the frame; that's w3c/webappsec-permissions-policy#65, I believe.
That's slightly different than just reflecting the structure of allow
, since it also includes any other restrictions inherited from the document: If the current document does not have fullscreen
enabled, for instance, then looking at the allow
attribute on <iframe allow=fullscreen>
will indicate that fullscreen was requested, but the iframe.policy
member will show that the feature would not be allowed in that frame.
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.
Sure, but wouldn't reflecting allow
better still make sense?
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 think I see what you mean -- it was originally a DOMTokenList, but that wasn't flexible enough, so we made it a DOMString like csp rather than attempting to define a complete data structure to reflect.
It would be useful to reflect it in this way -- I presume that would mean including the IDL for that type here. would that also require updating the definition of reflect to declare what it means to "reflect a feature policy allowlist"?
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.
An alternative might be to not use reflect and write out what the requirements are.
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 think using reflect here is good and proper, as long as the idea is to be like sandbox/srcset/csp instead of something more complicated.
I'm not sure it's worth the extra trouble to make a custom class for this until people ask for it; given that they haven't done so for those other attributes in the past I'd much rather wait than put in the up-front effort now.
Please see https://infra.spec.whatwg.org/#conformance. Using "will" seems fine for a statement of fact.
Ah, I guess we still need to clean that up at some point.
Could you file a follow-up issue on that? |
(Aside: your Git email address does not match your GitHub email address. You might want to add your @google.com handle to your GitHub account so you get proper credit.) |
Note to self: commit message needs to reference everything regarding allowfullscreen that's now moot (and close as appropriate). |
Other things that need to be done before landing:
|
Also see tests for snapshotting allowfullscreen in web-platform-tests/wpt#4354 |
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've updated the PR in response to the comments left on it, and to remove the unneeded references to feature "names" (from w3c/webappsec-permissions-policy#125)
source
Outdated
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> | ||
<li><dfn data-x="concept-container-policy" data-x-href="https://wicg.github.io/feature-policy/#container-policy">Container Policy</dfn></li> | ||
<li><dfn data-x="concept-serialized-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#serialized-feature-policy">Serialized Feature Policy</dfn></li> | ||
<li>The <dfn data-x-href="https://wicg.github.io/feature-policy/#initialize-for-global">Initialize global’s Feature Policy from response</dfn> algorithm</li> |
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.
Done now.
source
Outdated
<li>The <dfn data-x="is-feature-enabled" data-x-href="https://wicg.github.io/feature-policy/#is-feature-enabled">Is feature enabled by policy for origin</dfn> algorithm</li> | ||
<li>The <dfn data-x="process-feature-policy-attributes" data-x-href="https://wicg.github.io/feature-policy/#process-feature-policy-attributes">Process feature policy attributes</dfn> algorithm</li> | ||
</ul> | ||
|
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.
Done
source
Outdated
spec="FEATUREPOLICY">. | ||
|
||
<div class="example"> | ||
|
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.
Done.
source
Outdated
Geolocation API within the nested context.</p> | ||
|
||
<pre><strong><iframe src="https://maps.example.com/" allow="geolocation"></iframe></strong> | ||
</pre> |
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.
Done.
source
Outdated
|
||
<pre><strong><iframe src="https://maps.example.com/" allow="geolocation"></iframe></strong> | ||
</pre> | ||
|
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.
Done.
source
Outdated
feature to be used from any <span>origin</span>. This is enforced by the <span | ||
data-x="process-feature-policy-attributes">Process feature policy attributes</span> algorithm | ||
<ref spec="FEATUREPOLICY">. Note that this will only allow use of the fullscreen feature if the | ||
<code>iframe</code> element's <span>node document</span> is also allowed to use fullscreen. |
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.
Done -- merged these together, in a note
div.
source
Outdated
@@ -29740,18 +29783,29 @@ interface <dfn>HTMLIFrameElement</dfn> : <span>HTMLElement</span> { | |||
<p>The <dfn><code data-x="attr-iframe-allowpaymentrequest">allowpaymentrequest</code></dfn> | |||
attribute is a <span>boolean attribute</span>. When specified, it indicates that | |||
<code>Document</code> objects in the <code>iframe</code> element's <span>browsing context</span> | |||
are to be allowed to use the <code>PaymentRequest</code> interface to make payment requests.</p> | |||
should be initialized with a <span data-x="concept-document-feature-policy">feature policy</span> |
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.
Done.
source
Outdated
the <code>iframe</code> element's <span>browsing context</span> are to be allowed to use <code | ||
data-x="dom-MediaDevices-getUserMedia">getUserMedia()</code> (if it's not blocked for other | ||
reasons, e.g. there is another ancestor <code>iframe</code> without this attribute set).</p> | ||
the <code>iframe</code> element's <span>browsing context</span> should be initialized with a <span |
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.
Done.
source
Outdated
</div> | ||
|
||
<p class="warning">Because they only influence the feature policy of the <span>nested browsing |
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.
Yes; added links to "active document" and "feature policy".
data-x="dom-iframe-sandbox">sandbox</code></dfn> must <span>reflect</span> the respective content | ||
data-x="dom-iframe-name">name</code></dfn>, <dfn><code | ||
data-x="dom-iframe-sandbox">sandbox</code></dfn>, and <dfn><code | ||
data-x="dom-iframe-allow">allow</code></dfn> must <span>reflect</span> the respective content |
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.
We're looking at adding a policy
member to iframe DOM elements (though not an html attribute) to be able to introspect the policy that is being applied to the frame; that's w3c/webappsec-permissions-policy#65, I believe.
That's slightly different than just reflecting the structure of allow
, since it also includes any other restrictions inherited from the document: If the current document does not have fullscreen
enabled, for instance, then looking at the allow
attribute on <iframe allow=fullscreen>
will indicate that fullscreen was requested, but the iframe.policy
member will show that the feature would not be allowed in that frame.
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.
Apart from the concerns expressed in the individual comments, I'm still a little concerned about the time it will take for implementations to catch up with all these changes (and about the compatibility impact for some of them).
Is Chrome already shipping this simplified version?
Tests are still very much needed as well, including adjusting the existing tests for allowfullscreen et al.
source
Outdated
<p>The following terms are defined in <cite>Feature Policy</cite>: <ref spec="FEATUREPOLICY"></p> | ||
|
||
<ul class="brief"> | ||
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> |
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.
Title case seems wrong?
source
Outdated
|
||
<ul class="brief"> | ||
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> | ||
<li><dfn data-x="concept-container-policy" data-x-href="https://wicg.github.io/feature-policy/#container-policy">Container Policy</dfn></li> |
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.
Title case seems wrong?
source
Outdated
<ul class="brief"> | ||
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> | ||
<li><dfn data-x="concept-container-policy" data-x-href="https://wicg.github.io/feature-policy/#container-policy">Container Policy</dfn></li> | ||
<li><dfn data-x="concept-serialized-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#serialized-feature-policy">Serialized Feature Policy</dfn></li> |
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.
This also does not seem to match what is referenced.
source
Outdated
<li><dfn data-x="concept-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#feature-policy">Feature Policy</dfn></li> | ||
<li><dfn data-x="concept-container-policy" data-x-href="https://wicg.github.io/feature-policy/#container-policy">Container Policy</dfn></li> | ||
<li><dfn data-x="concept-serialized-feature-policy" data-x-href="https://wicg.github.io/feature-policy/#serialized-feature-policy">Serialized Feature Policy</dfn></li> | ||
<li>The <dfn data-x-href="https://wicg.github.io/feature-policy/#initialize-from-response">Initialize document’s Feature Policy from response</dfn> algorithm</li> |
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 seems Feature Policy itself is inconsistent on the casing of "feature policy" (I recommend lowercase).
source
Outdated
@@ -9149,6 +9164,10 @@ partial interface <dfn id="document" data-lt="">Document</dfn> { | |||
containing all of the <span>Content Security Policy</span> objects active for the document. The | |||
list is empty unless otherwise specified.</p> | |||
|
|||
<p>The <code>Document</code> has a <dfn data-x="concept-document-feature-policy" data-export="" | |||
data-dfn-for="Document">feature policy</dfn>, which is a <span data-x="concept-feature-policy"> | |||
feature policy</span>, which is initially empty.</p> |
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.
This does not quite make sense since if document's feature policy is a feature policy, then feature policy has to be standalone. However, the Feature Policy specification suggests it's a slot on document, which makes this weird.
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've sent a PR to feature-policy (w3c/webappsec-permissions-policy#165) that I hope addresses this, by making feature policy standalone. Can you take a look and see if that makes sense?
source
Outdated
when the <span data-x="concept-document-feature-policy">feature policy</span> for the | ||
<code>iframe</code>'s <span>nested browsing context</span> is initialized. Its value must be a | ||
<span data-x="concept-serialized-feature-policy">serialized feature policy</span> <ref | ||
spec="FEATUREPOLICY">. |
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.
<ref>
goes after the dot.
data-x="dom-iframe-sandbox">sandbox</code></dfn> must <span>reflect</span> the respective content | ||
data-x="dom-iframe-name">name</code></dfn>, <dfn><code | ||
data-x="dom-iframe-sandbox">sandbox</code></dfn>, and <dfn><code | ||
data-x="dom-iframe-allow">allow</code></dfn> must <span>reflect</span> the respective content |
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.
Sure, but wouldn't reflecting allow
better still make sense?
source
Outdated
@@ -76964,7 +76922,9 @@ dictionary <dfn>DragEventInit</dfn> : <span>MouseEventInit</span> { | |||
|
|||
<li><p><span>Implement the sandboxing</span> for <var>document</var>.</p></li> | |||
|
|||
<li><p><span>Set the allow* flags</span> for <var>document</var>.</p></li> | |||
<li><p>Execute the <span>Initialize document’s Feature Policy from response</span> | |||
algorithm on the <code>Document</code> object and the <span data-x="concept-response">response |
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.
You need to use <var>document</var>
here.
source
Outdated
<li><p><span>Set the allow* flags</span> for the <code>Document</code>.</p></li> | ||
<li><p>Execute the <span>Initialize document’s Feature Policy from response</span> | ||
algorithm on the <code>Document</code> object and the <span | ||
data-x="concept-response">response</span> used to generate the document. <ref spec="FEATUREPOLICY"></p> |
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.
This goes beyond a 100 columns.
<script> | ||
new PaymentRequest(…); // Allowed to use | ||
</script></pre> | ||
</div> |
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 would really like to keep this example in some fashion, since it demonstrates why using same origin-domain can be more strict than using same-origin. It still seems relevant so I'm not sure why we'd remove it.
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've put that example back, though I ended up moving it to browsing-the-web.html#initialise-the-document-object, since that's where the processing steps are.
This adds a policy-controlled feature, named 'sync-xhr', which can be disabled in a document to turn off synchronous requests for that document (and documents in all descendant frames). Calling send() on a synchronous request in a document where "sync-xhr" is disabled will result in a "NetworkError" DOMException exception being thrown. Caveat: whatwg/html#3287 which redefines "allowed to use" in HTML to be more like https://wicg.github.io/feature-policy/#allowed-to-use has not yet landed. If that takes significant time we should add a note to its usage here. Tests: xhr/xmlhttprequest-sync-default-feature-policy.sub.html in web-platform-tests. Fixes #178.
I'm updating the wpt tests; there's an auto-generated PR at web-platform-tests/wpt#10966 |
FYI, if I go to https://github.com/whatwg/html/pull/3287/files there's still four comments from me that are not addressed. |
@annevk Ian and I spent some time yesterday going over this in person. The only outstanding issue we identified was the phrasing around document vs. browsing context, which Ian just fixed. IMO this is ready to merge; do you agree? (Assuming of course that Ian files browser bugs on the changes to the allow* attributes.) |
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 suspect this is mostly in order now. Thanks a lot for working through all the things. I have a couple nits and @domenic should do a more line-by-line editorial review.
I think we already have tests covering some of this? In any case, someone needs to ensure testing is adequate and link that from the final commit.
(I'm going on vacation, hence the summary review.)
</li> | ||
<li><p>If the result of running <span data-x="is-feature-enabled">Is feature enabled in document | ||
for origin</span> on <var>feature</var>, <var>document</var>, and <var>document</var>'s | ||
<span>origin</span> is "<code data-x="">Enabled</code>", then return true.</p></li> |
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.
'Enabled' per Feature Policy?
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.
Feature policy should be consistent, but isn't right now. This algorithm does use "Enabled
", but the one above used 'Enabled'
. I'll update that spec to use strings throughout, so this should be correct.
source
Outdated
@@ -9212,6 +9228,10 @@ partial interface <dfn id="document" data-lt="">Document</dfn> { | |||
containing all of the <span>Content Security Policy</span> objects active for the document. The | |||
list is empty unless otherwise specified.</p> | |||
|
|||
<p>The <code>Document</code> has a <dfn data-x="concept-document-feature-policy" data-export="" | |||
data-dfn-for="Document">feature policy</dfn>, which is a <span data-x="concept-feature-policy"> |
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.
Cannot have a newline after a start tag.
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.
Done
source
Outdated
@@ -77001,7 +76962,8 @@ dictionary <dfn>DragEventInit</dfn> : <span>MouseEventInit</span> { | |||
|
|||
<li><p><span>Implement the sandboxing</span> for <var>document</var>.</p></li> | |||
|
|||
<li><p><span>Set the allow* flags</span> for <var>document</var>.</p></li> | |||
<li><p>Execute the <span>Initialize document’s Feature Policy</span> algorithm on | |||
<var>document</var>. <ref spec="FEATUREPOLICY"></p> |
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.
Closing </li>
.
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.
Done
source
Outdated
checks are less permissive compared to doing a <span>same origin</span> check instead.</p> | ||
|
||
<div class="example"> | ||
|
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.
No additional newline here.
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.
Is there a more extensive styleguide somewhere? Not complaining at all, just trying to understand what's expected.
From a quick grep, I see 554 instances of "<div class="example">
" in the spec: 98 followed by a single newline; 438 followed by 2 newlines. (and 8 with the example starting on the same line as the opening <div>
)
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.
You'd be right to complain. We're slowly migrating to single newline after "block-level" start tags (and before the end tag), like div
, ol
, li
etc. if they contain multiple "block-level" children.
source
Outdated
</div> | ||
|
||
<div class="example"> | ||
|
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.
Same.
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.
Done.
Thanks, @annevk ! -- I'll take care of the nits and push again.
|
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.
LGTM! Found one typographical thing to fix and pushed it, as well as a couple things to note (below). Will merge now. Please file issues on the browsers, and leave pointers to them here.
reasons, e.g. there is another ancestor <code>iframe</code> without this attribute set).</p> | ||
the <code>iframe</code> element's <span>browsing context</span> will be initialized with a | ||
<span data-x="concept-document-feature-policy">feature policy</span> which allows the <code | ||
data-x="">fullscreen</code> feature to be used from any <span>origin</span>. This is enforced by |
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.
Are feature names fullscreen
, or "fullscreen"
? https://xhr.spec.whatwg.org/#sync-xhr uses the latter but https://wicg.github.io/feature-policy/#process-feature-policy-attributes and this patch uses the former.
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.
Good catch -- we originally had a different symbolic feature name and string identifier, but that turned out to be redundant, so we only use the string now, and the feature policy spec should just use those strings.
https://wicg.github.io/feature-policy/#policy-controlled-feature now says that features are identified by tokens, which are character strings.
context</span>'s <span>active document</span>, the <code data-x="attr-iframe-allow">allow</code>, | ||
<code data-x="attr-iframe-allowfullscreen">allowfullscreen</code>, <code | ||
data-x="attr-iframe-allowpaymentrequest">allowpaymentrequest</code> and <code | ||
data-x="attr-iframe-allowusermedia">allowusermedia</code> attributes only take effect when the |
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 wonder if we should tell authors not to use allowfullscreen/allowpaymentrequest/allowusermedia. Maybe only once all browsers support allow=""? This is probably worth filing a follow-up issue to track.
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 would be less confusing in the far future if we were to deprecate those; once there's wide support for allow
, we should think about doing that.
In the meantime, we should also make sure that there's guidance somewhere about not introducing new allowfoo
-style attributes on <iframe>
.
This was missed in #3287. Also changes references to feature policy names to be strings instead of just monospaced, per discussion in #3287 (comment).
This was missed in whatwg#3287. Also changes references to feature policy names to be strings instead of just monospaced, per discussion in whatwg#3287 (comment).
…ibute, a=testonly Automatic update from web-platform-tests Test snapshotting of allowfullscreen attribute (#4354) Spec: https://html.spec.whatwg.org/multipage/iframe-embed-object.html#allowed-to-use Test for whatwg/html#3287. -- wpt-commits: 7f6dfe9acab13bc9e86c0fc8c9bf24aee058671b wpt-pr: 4354
This commit introduces the feature policy for Document objects, adds the
'allow' attribute to iframe elements, and reframes 'allowfullscreen',
'allowpaymentrequest' and 'allowusermedia' in terms of feature policy.
Document allow* flags are removed, as they are no longer referenced.
The 'allowed to use' algorithm is also updated to call into the feature
policy 'Is feature enabled' algorithm, and rewritten to take a policy-
controlled feature as an argument rather than an attribute, so that
other specs can also use it to control other features.
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/iframe-embed-object.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )