1608358 - Problem with allowfullscreen and allow=fullscreen

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Anne (:annevk)]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Anne (:annevk)]

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

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1608358 - Fix fullscreen allowed contition check to contain feature policy

Comment 5

[Rob Wu [:robwu]]

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.

Comment 7

[Anne (:annevk)]

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1608358 - Fix allowfullscreen check to check for feature policy. r=baku

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fec7d23c5a02
Fix allowfullscreen check to check for feature policy. r=baku

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01bcf8fe712a
Backed out changeset fec7d23c5a02 for wpt failures on document-fullscreen-enabled-cross-origin.sub.html. CLOSED TREE

Comment 13

[Cosmin Sabou [:CosminS]]

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1384795e8af4
Fix allowfullscreen check to check for feature policy. r=baku

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24586 for changes under testing/web-platform/tests

Comment 19

[Alexandru Michis [:malexandru]]

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

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Chris Mills [:cmills]]

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!

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 23

[Chris Mills [:cmills]]

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

Comment 24

[Bogdan Maris, Desktop Test Engineering]

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

Comment 25

[Mathew Hodson]

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

Comment 26

[j.j.]

[Tracking Requested - why for this release]:

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

Comment 27

[Julien Cristau [:jcristau]]

ni?emilio for comment 26

Comment 28

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 29

[Julien Cristau [:jcristau]]

I don't know either. Maybe Anne does?

Comment 30

[Johann Hofmann [:johannh]]

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

Comment 31

[Julien Cristau [:jcristau]]

Can you clarify what triggered comment 26?

Comment 32

[Anne (:annevk)]

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

Comment 33

[Emilio Cobos Álvarez (:emilio)]

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

Comment 34

[j.j.]

(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 35

[Rob Wu [:robwu]]

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Attachment: ESR patch

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 38

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 78.6esr, thanks.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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