So looking at bug 1588377 the testcase there https://codepen.io/yukulele/pen/YzPeVZE?editors=1000 does not work in Firefox and that does not give us different markup (whereas Twitter seems to do so).

That uses <iframe allowfullscreen allow=things-but-not-fullscreen> -> <iframe allow=fullscreen> and that should probably work per Feature Policy, but does not in Firefox.

What does
<iframe allowfullscreen allow=things-but-not-fullscreen> -> <iframe allow=fullscreen>
mean?

Document A contains the first iframe which references document B containing the second iframe.

Baku, the patch on this bug is r+ by you, but the author has left. Can it be sent to Lando?

From a cursory look the patch appears to properly implement the allow="fullscreen" attribute on iframes.

I stumbled upon this bug because I was investigating why the following does not work, and believe that the patch will address the issue:

data:text/html,<iframe allow="fullscreen" srcdoc='<button onclick="document.documentElement.requestFullscreen()">Go Fullscreen</button>'></iframe>

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/fullscreen , allow="fullscreen" is supposed to be supported, but that is not the case. Firefox only supports fullscreen in frames if the allowfullscreen attribute is present.

Johann might be able to help with this. It seems this is the only remaining bug for our allow="" attribute implementation.

We can clean up this code once the feature policy pref goes away, but
meanwhile this should do the right thing.

Drive-by stealing as I saw this fly by and I've touched related code recently.

Backed out for wpt and mochitest fullscreen related failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=64d26ee7d9e7eb729b73b4564bbfff6b3f0d66d1&failure_classification_id=2&selectedTaskRun=Ly2L6ZIHQnWEQSGQxhiaBA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=309358901&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=309358965&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/01bcf8fe712a86759f4e112958652c8864584088

Hmmpf, try auto fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17ab2a3f1daeb46d315aa52028e99ea19a0862e2

Or "Emilio can't read try push results" fail, in fairness. There's one failure which I skipped over and that got me backed out.

The failures are actually progressions. Right now <iframe src="same-origin.html"></iframe> disallows fullscreen in Firefox, but it shouldn't per spec.

I tweaked the tests so that they still test what they intend to.

https://hg.mozilla.org/mozilla-central/rev/1384795e8af4

I think I've explained this OK in the docs:

• Updated the first note on https://wiki.developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/fullscreen
• Added a rel note to https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/80#HTTP

Can you let me know if you think I've understood this correctly? I think I might add a note to the browser compat data too, but want to get this checkout out first. Thanks!

Yeah, those notes sound alright! It is exactly as you describe it, in order for featurepolicy to work you also had to have allowfullscreen on the frame.

Great, thanks :emilio. I've added a note to the compat data to cover this too: https://github.com/mdn/browser-compat-data/pull/6493

Reproduced the initial issue using an old Nightly build from 2020-01-10. Verified that this issue is fixed using latest Firefox 80.0b5 across platforms (Windows 10 64bit, macOS 10.15 and Ubuntu 18.04 64bit).

Bug 1495362 was put in the wrong field. I think it was meant to be put in the Regressed By field.

[Tracking Requested - why for this release]:

So ESR78 fails on <iframe allowfullscreen> without allow=fullscreen?
If so, should this be fixed?

ni?emilio for comment 26

I don't have a sense of how big of a problem this is in practice, but if it applies cleanly I don't think it'd be risky.

However I think this also would need bug 1606660 etc. I can try to get a patch for the ESR tree and report back if you think it's going to be a problem and is worth uplifting.

I don't know either. Maybe Anne does?

Are there any known real world webcompat bugs from this? If yes then we should probably uplift, if not then probably not. It seems like an edge case to me...

Can you clarify what triggered comment 26?

Unless j.j. knows a site I similarly think it's not worth the hassle, especially given what Emilio points out in comment 28.

(In reply to j.j. from comment #26)

[Tracking Requested - why for this release]:

So ESR78 fails on <iframe allowfullscreen> without allow=fullscreen?
If so, should this be fixed?

Also this is backwards, esr should work with allowfullscreen, but will fail when only allow=fullscreen is specified.

(In reply to Julien Cristau [:jcristau] from comment #31)

Can you clarify what triggered comment 26?

This bug has never affected me.
Duplicate bug 1588377 was about Youtube and Twitter and I asked because the status was unclear to me.

comment 5 is a reduction from a broken site that was shared with me by a developer of https://github.com/webrecorder/replayweb.page .

As a work-around, I suggested to add the "allowfullscreen" attribute to their iframe, which was added in https://github.com/webrecorder/replayweb.page/blame/05adead57c2a8238827a32b67559fac8f44b8066/src/replay.js#L201

Developers can work around this, but since MDN has documented for so long that allow="fullscreen" works, I would recommend to uplift this if the patch is safe. The worst that could happen is that sites that use allow="fullscreen" aren't functioning in Firefox, and the easy solution for that is just to move to a different browser.

If this is causing issues with Youtube and Twitter, maybe we should consider an uplift patch. Emilio, can you look into what that would look like in practice?

This is effectively part 1 of bug 1606660 plus the patch on this bug. I think it should do.

It required some conflict-rebasing but I think it should be fine.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: comment 35 etc
User impact if declined: some videos / iframes can't go fullscreen.
Fix Landed on Version: 80
Risk to taking this patch (and alternatives if risky): not super-risky I'd say. If it's green on try it should be good to go, we have good tests for this.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 9187111 [details] [diff] [review]
ESR patch

Approved for 78.6esr, thanks.

https://hg.mozilla.org/releases/mozilla-esr78/rev/528bc3937f80