Closed Bug 1608358 Opened 10 months ago Closed 4 months ago

Problem with allowfullscreen and allow=fullscreen

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla80
Tracking Status
firefox-esr78 ? ?
firefox80 --- verified

People

(Reporter: annevk, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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?

Flags: needinfo?(annevk)

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

Flags: needinfo?(annevk)
Assignee: nobody → tnguyen
Duplicate of this bug: 711276
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 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.

Flags: needinfo?(amarchesini)
Keywords: dev-doc-needed
Duplicate of this bug: 1588377

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

Flags: needinfo?(jhofmann)
Duplicate of this bug: 1651369

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 ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fec7d23c5a02
Fix allowfullscreen check to check for feature policy. r=baku
Backout by csabou@mozilla.com:
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

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 ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1384795e8af4
Fix allowfullscreen check to check for feature policy. r=baku
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24586 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 4 months 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: 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).

Status: RESOLVED → VERIFIED
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 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.

You need to log in before you can comment on or make changes to this bug.