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

Integrate Feature Policy concepts into HTML #3287

Merged
merged 10 commits into from Jun 22, 2018

Conversation

clelland
Copy link
Contributor

@clelland clelland commented Dec 13, 2017

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 )

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.
Copy link
Member

@annevk annevk left a 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
Copy link
Member

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".

Copy link
Contributor Author

@clelland clelland Dec 19, 2017

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 the iframe element's browsing context will be initialized with a feature policy which allows the fullscreen feature to be used from any origin.

Copy link
Contributor Author

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>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

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

And here.

Copy link
Contributor Author

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>
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 an incompatible change per #2143 (comment). Is that what Chrome now implements? We used to take dynamic changes into account for at least allowfullscreen.

Copy link
Contributor Author

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>
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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>

Copy link
Member

Choose a reason for hiding this comment

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

This newline can go.

Copy link
Contributor Author

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>&lt;iframe src="https://maps.example.com/" allow="geolocation">&lt;/iframe></strong>
</pre>

Copy link
Member

Choose a reason for hiding this comment

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

This newline can go.

Copy link
Contributor Author

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
Copy link
Member

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".

Copy link
Contributor Author

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>
Copy link
Member

Choose a reason for hiding this comment

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

page -> document

Copy link
Contributor Author

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
Copy link
Member

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.

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'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.

Copy link
Member

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?

Copy link
Contributor Author

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"?

Copy link
Member

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.

Copy link
Member

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.

@annevk annevk mentioned this pull request Dec 20, 2017
4 tasks
@annevk
Copy link
Member

annevk commented Dec 20, 2017

I thought there was a distinction between 'should' and 'SHOULD'

Please see https://infra.spec.whatwg.org/#conformance. Using "will" seems fine for a statement of fact.

Done -- origin, even when it is "document's origin"

Ah, I guess we still need to clean that up at some point.

(this was following the wording on the very similar warning for sandbox, which should probably be changed too)

Could you file a follow-up issue on that?

@annevk
Copy link
Member

annevk commented Dec 20, 2017

(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.)

@annevk
Copy link
Member

annevk commented Dec 20, 2017

Note to self: commit message needs to reference everything regarding allowfullscreen that's now moot (and close as appropriate).

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Dec 20, 2017
@annevk
Copy link
Member

annevk commented Dec 20, 2017

Other things that need to be done before landing:

  • web-platform-tests PR (also needs to be referenced from the commit message; we can take care of the referencing)
  • implementation bugs (except for Chrome I suppose, though might be good to reference in case of coordination needs)

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Dec 20, 2017
@zcorpan
Copy link
Member

zcorpan commented Dec 24, 2017

Also see tests for snapshotting allowfullscreen in web-platform-tests/wpt#4354

Copy link
Contributor Author

@clelland clelland left a 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>
Copy link
Contributor Author

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>

Copy link
Contributor Author

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">

Copy link
Contributor Author

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>&lt;iframe src="https://maps.example.com/" allow="geolocation">&lt;/iframe></strong>
</pre>
Copy link
Contributor Author

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>&lt;iframe src="https://maps.example.com/" allow="geolocation">&lt;/iframe></strong>
</pre>

Copy link
Contributor Author

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.
Copy link
Contributor Author

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>
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
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'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.

Copy link
Member

@annevk annevk left a 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>
Copy link
Member

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>
Copy link
Member

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>
Copy link
Member

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>
Copy link
Member

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>
Copy link
Member

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.

Copy link
Contributor Author

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">.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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>
Copy link
Member

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.

&lt;script>
new PaymentRequest(&hellip;); // Allowed to use
&lt;/script></pre>
</div>
Copy link
Member

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.

Copy link
Contributor Author

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.

annevk pushed a commit to whatwg/xhr that referenced this pull request Mar 2, 2018
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.
@loonybear loonybear mentioned this pull request Mar 23, 2018
@clelland
Copy link
Contributor Author

I'm updating the wpt tests; there's an auto-generated PR at web-platform-tests/wpt#10966

@zcorpan zcorpan removed the needs tests Moving the issue forward requires someone to write tests label May 13, 2018
@annevk
Copy link
Member

annevk commented May 14, 2018

FYI, if I go to https://github.com/whatwg/html/pull/3287/files there's still four comments from me that are not addressed.

@domenic
Copy link
Member

domenic commented Jun 21, 2018

@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.)

Copy link
Member

@annevk annevk left a 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>
Copy link
Member

Choose a reason for hiding this comment

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

'Enabled' per Feature Policy?

Copy link
Contributor Author

@clelland clelland Jun 22, 2018

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">
Copy link
Member

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.

Copy link
Contributor Author

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>
Copy link
Member

Choose a reason for hiding this comment

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

Closing </li>.

Copy link
Contributor Author

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">

Copy link
Member

Choose a reason for hiding this comment

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

No additional newline here.

Copy link
Contributor Author

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>)

Copy link
Member

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">

Copy link
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@clelland
Copy link
Contributor Author

Thanks, @annevk ! -- I'll take care of the nits and push again.
For testing, we have WPT for:

@domenic domenic added addition/proposal New features or enhancements and removed do not merge yet Pull request must not be merged per rationale in comment labels Jun 22, 2018
Copy link
Member

@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! 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
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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>.

@domenic domenic merged commit 78741b7 into whatwg:master Jun 22, 2018
@clelland clelland deleted the integrate-feature-policy branch June 26, 2018 15:10
domenic pushed a commit that referenced this pull request Jun 28, 2018
This was missed in #3287. Also changes references to feature policy
names to be strings instead of just monospaced, per discussion in
#3287 (comment).
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
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).
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 8, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

None yet

4 participants