Closed Bug 1608358 Opened 5 years ago Closed 4 years ago

Problem with allowfullscreen and allow=fullscreen


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




Tracking Status
firefox-esr78 84+ fixed
firefox80 --- verified


(Reporter: annevk, Assigned: emilio)


(Blocks 1 open bug)


(Keywords: dev-doc-complete)


(3 files)

So looking at bug 1588377 the testcase there 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>

Flags: needinfo?(annevk)

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

Flags: needinfo?(annevk)
Assignee: nobody → tnguyen
Blocks: 1572461
Regressions: 1495362
Priority: -- → P2

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

Flags: needinfo?(amarchesini)
Keywords: dev-doc-needed

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

Flags: needinfo?(jhofmann)

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.

Assignee: tnguyen → emilio
Flags: needinfo?(jhofmann)
Flags: needinfo?(amarchesini)
Pushed by
Fix allowfullscreen check to check for feature policy. r=baku
Backout by
Backed out changeset fec7d23c5a02 for wpt failures on document-fullscreen-enabled-cross-origin.sub.html. CLOSED TREE

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.

Flags: needinfo?(emilio)
Pushed by
Fix allowfullscreen check to check for feature policy. r=baku
Created web-platform-tests PR for changes under testing/web-platform/tests
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Upstream PR merged by moz-wptsync-bot
Flags: qe-verify+

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

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!

Flags: needinfo?(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.

Flags: needinfo?(emilio)

Great, thanks :emilio. I've added a note to the compat data to cover this too:

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

Flags: qe-verify+

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

Flags: needinfo?(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.

Flags: needinfo?(emilio) → needinfo?(jcristau)

I don't know either. Maybe Anne does?

Flags: needinfo?(jcristau) → needinfo?(annevk)

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?

Flags: needinfo?(moz)

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

Flags: needinfo?(annevk)

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

Flags: needinfo?(moz)

comment 5 is a reduction from a broken site that was shared with me by a developer of .

As a work-around, I suggested to add the "allowfullscreen" attribute to their iframe, which was added in

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?

Flags: needinfo?(emilio)
Attached patch ESR patchSplinter Review

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 for more info.

Flags: needinfo?(emilio)
Attachment #9187111 - Flags: approval-mozilla-esr78?

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

Approved for 78.6esr, thanks.

Attachment #9187111 - Attachment is patch: true
Attachment #9187111 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
You need to log in before you can comment on or make changes to this bug.