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

"allowed to use" active document, browsing context container... #2143

Open
zcorpan opened this issue Dec 7, 2016 · 6 comments
Open

"allowed to use" active document, browsing context container... #2143

zcorpan opened this issue Dec 7, 2016 · 6 comments
Labels
needs tests Moving the issue forward requires someone to write tests topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@zcorpan
Copy link
Member

zcorpan commented Dec 7, 2016

In w3c/payment-request#361 (comment) @bzbarsky wrote:

What you may want to do is restrict this API to fully active documents only. But even then, walking up the creator document chain (but only until you get to a document in a toplevel browsing context) makes more sense than walking up the ancestor browsing context chain.

We don't check if document is the active document in "allowed to use".

https://html.spec.whatwg.org/#allowed-to-use currently says

  1. If document's browsing context has a browsing context container that is an iframe element with an allowattribute attribute specified, and whose node document is allowed to use the feature indicated by allowattribute, then return true.

https://html.spec.whatwg.org/#creator-browsing-context for iframe is the same as:
https://html.spec.whatwg.org/#parent-browsing-context which is defined in terms of:
https://html.spec.whatwg.org/#child-browsing-context

  • child is a nested browsing context of a browsing context container element
  • element is connected
  • element's shadow-including root's browsing context is parent

So... Shadow DOM should be considered here also. It is not clear to me yet if that is the only difference between parent browsing context and walking up the browsing context container.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 7, 2016

Here's the thing. The phrase "allowed to use" appears in HTML in three places: the definitions of the allowusermedia and allowfullscreen attrbibutes, and the definition of https://html.spec.whatwg.org/multipage/embedded-content.html#allowed-to-use

Looking at Gecko's implementation, we do have allowfullscreen that more or less follows the spec's definition (including walking up the browsing context chain, fwiw). We don't have allowusermedia.

But generally speaking, this algorithm as written means that whether the thing is allowed or not can change during the lifetime of the document, due to attribute changes on arbitrary ancestor <iframe> elements. It behaves differently from the feature policy proposals, which snapshot the parent information at document creation time...

It's possible that for allowfullscreen compat constraints mean the check has to be dynamic (though I doubt it), but for new things we should be following the feature policy approach of snapshotting the value at document creation time.

zcorpan added a commit that referenced this issue Dec 16, 2016
According to #2184 (comment)
the page that was previously broken by allowfullscreen not being
live has now changed, so it may be possible to move back to the
snapshot model used by <iframe sandbox> and Feature Policy.

Fixes #2143.

This partially reverts 9f6b91c.
@zcorpan zcorpan mentioned this issue Dec 16, 2016
4 tasks
zcorpan added a commit that referenced this issue Dec 20, 2016
According to #2184 (comment)
the page that was previously broken by allowfullscreen not being
live has now changed, so it may be possible to move back to the
snapshot model used by <iframe sandbox> and Feature Policy.

Fixes #2143.

This partially reverts 9f6b91c.
zcorpan added a commit that referenced this issue 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.
annevk pushed a commit that referenced this issue Jan 14, 2017
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: web-platform-tests/wpt#4369.

Part of #2143.
@zcorpan zcorpan added the topic: shadow Relates to shadow trees (as defined in DOM) label Sep 1, 2018
@zcorpan
Copy link
Member Author

zcorpan commented Sep 1, 2018

I think this issue can be closed now. @bzbarsky agree?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 1, 2018

Maybe we should make sure there are tests for this though.

@zcorpan zcorpan added the needs tests Moving the issue forward requires someone to write tests label Sep 1, 2018
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
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 whatwg#2187).

Test: web-platform-tests/wpt#4369.

Part of whatwg#2143.
@domenic
Copy link
Member

domenic commented Nov 18, 2020

@clelland do you know if there are tests for this? (The spec was fixed, but we left the issue open to track adding tests.)

@clelland
Copy link
Contributor

There are a few things in this issue, so I'm not sure specifically which one we were holding it open for:

  • For snapshotting the allowfullscreen (and allow) attributes, we have WPT here and here.
  • For checking whether the document is active, there are fullscreen tests here and here
  • There is one Shadow DOM test for fullscreen here, but I'm not sure if it's testing the concern mentioned in the initial comment. I don't think we have any tests for allow that include Shadow DOM.
  • (And allowusermedia has been removed since this issue was last updated)

@domenic
Copy link
Member

domenic commented Nov 19, 2020

Sorry for not being clear, and thanks for tracking those down!! The main change I was concerned about was the active document check.

Do you know about if there are tests that ensure that even if a document would be allowed to use a feature due to allow="", it cannot do so if it's non-active? It looks like the tests you linked cover allowfullscreen="", but some for allow="" would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging a pull request may close this issue.

4 participants