Snapshot allowfullscreen

[zcorpan]

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.

---

@upsuper said:

If we decide to switch the behavior, I hope all browsers can coordinate to switch at roughly the same time, so that if any website is regressed, we can ask them to fix their website, rather than changing our behavior back and forth.

So, first, interest to do this at all, then y'all can figure out how to coordinate when to switch.

• Gecko: @upsuper @bzbarsky
• Chromium: @foolip ?
• WebKit: @cdumez ?
• Edge: @travisleithead ?

[foolip]

I would definitely like to see allowfullscreen eventually defined in terms of Feature Policy, so the more similar we can make it in the interim the better. @clelland @LoonyBean, do you know what the exact timing of when Feature Policy would snapshot its state is? I'd like to add a use counter to see how often this change would prevent fullscreen currently allowed.

[zcorpan]

Tests for allowfullscreen: web-platform-tests/wpt#4354

Tests for allowpaymentrequest: web-platform-tests/wpt#4369

[clelland]

@foolip - The plan is to have the embedding document's feature policy snapshotted at the point where the nested browsing context of the frame is navigated. (This is roughly the same behaviour as that of the iframe sandbox attribute.)

The full policy won't be set at that point; it will need to wait until the HTTP headers for the framed content have been received. The snapshotted policy from the embedder will be combined with the embedded document's headers at the point when the embedded Document is created, I believe.

[bzbarsky]

@clelland the iframe sandbox attribute is snapshotted, per spec, when the new document in the subframe is created. That's certainly what Firefox does. Is that not what Chrome does?

I expected feature policy to work exactly the same way, based on the explainer, which says "Define an inherited policy for feature for origin when document is initialized".

[clelland]

Thanks, @bzbarsky -- you're right there -- it's part of [Initializing a new Document object], and it looks like Chrome follows the spec. That means we definitely can resolve the policy inherited from the parent at the same time that we construct the policy for the child.

[foolip]

@clelland, as implemented in Blink, is it indistinguishable in timing from when the sandbox attribute is read, even if not close by in the code?

[zcorpan]

@clelland @bzbarsky I written a test for the timing aspect in web-platform-tests/wpt@c050814

[zcorpan]

I made a new pull request to only change allowpaymentrequest in #2195 so that is not held up by allowfullscreen.

[foolip]

Chromium: @foolip ?

I think we should do this, but there is a decent chance that it won't work out for the allowfullscreen case. However, as soon as there's one engine who wants to attempt the change, I think we should merge this. @upsuper, are you going to do it for Gecko? In Chromium, I wonder if it might be best to combine with @clelland's plans for defining allowfullscreen in terms of Feature Policy.

[zcorpan]

This PR is currently lagging behind #2195 so see that PR for now what the behavior should be. When that one is merged I will rebase and fixup this one for allowfullscreen/allowusermedia.

[upsuper]

@foolip, I'm not sure about Gecko's plan of Feature Policy. @bzbarsky may know more about that. I guess it might be better to combine them for us as well. But with the previous experience, I think adding (or blocking) this solely for allowfullscreen should be straightforward as well, so I'm open to applying it to allowfullscreen whenever we can coordinate.

[n054]

@foolip I run chrome on this device and would like to help test this pull request Would it be best to test it here on Github or should I pop over to the Google Git Chromium repo instead.

[zcorpan]

allowusermedia and allowpaymentrequest use the snapshot model in the current spec; so this pull request is about moving over allowfullscreen as well. I can brush up this PR when someone has tried to ship allowfullscreen with the snapshot model so we know what the web compat situation is like...

[zcorpan]

[blink-dev] Intent to Implement and Ship: Snapshot iframe allowfullscreen attribute
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UKChYGBYvSk

[foolip]

We do have implementer interest now, but do we want to leave this open until we see the compat fallout? One downside of this is that any tests can't be done inside of web-platform-tests up-front and we may forget to share them later, but I don't see any way around this and think it's the right trade-off here.

[zcorpan]

To avoid flip-flopping the standard (and web-platform-tests) I think we should leave this open until we have a clearer picture of what the web compat situation is like. The time to merge this (or not) will maybe function as a reminder to upstream the tests. WDYT?

[foolip]

I agree, let's go with that.

[annevk]

So it seems Chrome 62 shipped this change per @clelland in #3287. That PR will also obsolete this PR and I'll try to ensure that's noted in the eventual commit message. It seems there was already implementer interest so that's probably all okay.

[foolip]

Looks like, at least textually, #3287 doesn't include the same bits as this PR. Is this PR actually obsoleted, and if so should it be closed?

[annevk]

See #2187 (comment). I think we should close it once we land the other.

[foolip]

That is the comment I was reacting to, but OK, this isn't accidentally left open, which is what I suspected.

[domenic]

#3287 got merged, so we can close this. (Please reopen if I got that wrong...)