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

Snapshot the allowpaymentrequest attribute #2195

Merged
merged 4 commits into from
Jan 14, 2017

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 20, 2016

New allow* attributes should use the snapshot model used by <iframe sandbox>
and Feature Policy. The allowfullscreen and allowusermedia attributes are
left with their current live behavior in this change, because it is not yet
clear if it is Web-compatible to change them (see #2187).

Part of #2143.

New allow* attributes should use the snapshot model used by <iframe sandbox>
and Feature Policy. The allowfullscreen and allowusermedia attributes are
left with their current live behavior in this change, because it is not yet
clear if it is Web-compatible to change them (see #2187).

Part of #2143.
@zcorpan zcorpan added the needs tests Moving the issue forward requires someone to write tests label Dec 20, 2016
@zcorpan zcorpan mentioned this pull request Dec 20, 2016
4 tasks
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Dec 20, 2016
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Dec 20, 2016
@zcorpan
Copy link
Member Author

zcorpan commented Dec 20, 2016

Tests: web-platform-tests/wpt#4369

@zcorpan zcorpan removed the needs tests Moving the issue forward requires someone to write tests label Dec 20, 2016
@zcorpan
Copy link
Member Author

zcorpan commented Dec 20, 2016

cc @rsolomakhin

<p>Otherwise, follow these substeps:</p>

<ol>
<li><p>If <var>document</var> has the <var><var>allowattribute</var> flag</var> set, and
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 nested var here is maybe a bit unclear? Any ideas? Adding "«" and "»" before/after with CSS helps I think...

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't it <var>allow<var>attribute</var> flag</var> elsewhere? That was reasonbly clear I thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I was misremembering. Some CSS solution is probably good; not sure I'd use those quotes though since we use them to denote Infra lists.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -28485,7 +28485,9 @@ interface <dfn>HTMLIFrameElement</dfn> : <span>HTMLElement</span> {
has a <span>nested browsing context</span>, the user agent must <span data-x="parse a sandboxing
directive">parse the sandboxing directive</span> using the attribute's value as the
<var>input</var> and the <code>iframe</code> element's <span>nested browsing context</span>'s
<span><code>iframe</code> sandboxing flag set</span> as the output.</p>
<span><code>iframe</code> sandboxing flag set</span> as the output. If the <code>iframe</code> has
an <code data-x="attr-iframe-allowpaymentrequest">allowpaymentrequest</code> attribute, then set
Copy link
Member Author

@zcorpan zcorpan Dec 23, 2016

Choose a reason for hiding this comment

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

Hmm, I put this here because it was here in 9f6b91c but this doesn't make sense. It needs to be in https://html.spec.whatwg.org/#initialise-the-document-object I think.

edit: and https://html.spec.whatwg.org/#creating-a-new-browsing-context

@annevk
Copy link
Member

annevk commented Jan 12, 2017

I saw someone mention that allowusermedia isn't implemented yet, so should we move that at the same time?

@zcorpan
Copy link
Member Author

zcorpan commented Jan 12, 2017

OK, done.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

LGTM assuming that @domenic is happy with the nested var.

@annevk
Copy link
Member

annevk commented Jan 13, 2017

I was about to land this using

Snapshot allowpaymentrequest and allowusermedia attributes

New allow* attributes should use the snapshot model used by <iframe sandbox> and Feature Policy. The allowfullscreen attributes is left with its current live behavior in this change, because it is not yet clear if it is web-compatible to change it (see #2187).

Test: https://github.com/w3c/web-platform-tests/pull/4369.

Part of #2143.

as commit message however I noticed that the commit is no longer accurate.

possible this behavior will go away for the <code
data-x="attr-iframe-allowfullscreen">allowfullscreen</code> and <code
data-x="attr-iframe-allowusermedia">allowusermedia</code> attributes as well, see <a
href="https://github.com/whatwg/html/issues/2143#issuecomment-265514585">issue 2143</a>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

We should remove allowusermedia from this message. I'll add a commit.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 14, 2017

Thanks Anne. This is good to go now with your added commit?

@annevk
Copy link
Member

annevk commented Jan 14, 2017

Yeah, I didn't land it since I figured someone should review my change.

@annevk annevk merged commit b3224a8 into master Jan 14, 2017
@annevk annevk deleted the zcorpan/only-snapshot-allowpaymentrequest branch January 14, 2017 16:55
zcorpan added a commit to whatwg/resources.whatwg.org that referenced this pull request Jan 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants