1627134 - Hide fullscreen video control button when fullscreen is disabled via Feature Policy

Status: NEW

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Tim Nguyen :ntim]

data:text/html,<iframe src="http://www.capstone.cse.msu.edu/2019-01/projects/mozilla/project-video.mp4" allowfullscreen allow="fullscreen 'none'"></iframe>

STR:

• Visit the test URL

AR:
Video controls have a non-functional fullscreen button

ER:
The fullscreen button should be hidden (or functional, though it might make sense to hide it, from the main UI at least)

This logic is supposed to hide it:
https://searchfox.org/mozilla-central/rev/4ccefc3181f9d237ef4ca8bd17b4e7c101ddf7b5/toolkit/content/widgets/videocontrols.js#1555-1560

except document.fullscreenEnabled does not take in account the feature policy state. It would be quite easy to implement this on the platform side (it's just the matter of moving the feature policy check from Element::GetFullscreenError to Document::GetFullscreenError), but it sounds to me that this document.fullscreenEnabled behavior was intentional according to this test: https://searchfox.org/mozilla-central/rev/4ccefc3181f9d237ef4ca8bd17b4e7c101ddf7b5/dom/html/test/file_fullscreen-featurePolicy-inner.html#19-20

Comment 1

[:Gijs (he/him)]

:baku, can you confirm the tail-end of comment #0 about document.fullscreenEnabled ? And if that's right, what's the "right" way for video controls to check if full screen is allowed?

Aside: we'll probably also want to disable the context menu item.

Comment 2

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #1)

Aside: we'll probably also want to disable the context menu item.

The context menu has system privileges, and was fixed by bug 1626419.

Comment 3

[:Gijs (he/him)]

Hsin-Yi, who knows about feature policy and can answer the question in comment #1? It looks like the patch from bug 1626419 was C++ only, and IsFeatureAllowed is not exposed through webidl or similar, so frontend can't fix this bug without some help from the DOM team and/or changes to how we expose feature policy bits.

Comment 4

[Hsin-Yi Tsai (she/her)[:hsinyi]]

(In reply to :Gijs (he/him) from comment #3)

Hsin-Yi, who knows about feature policy and can answer the question in comment #1? It looks like the patch from bug 1626419 was C++ only, and IsFeatureAllowed is not exposed through webidl or similar, so frontend can't fix this bug without some help from the DOM team and/or changes to how we expose feature policy bits.

I am not sure who owns this part recently. I hope :pbz has thoughts.

Comment 5

[Emma Zühlcke [:emz]]

I don't think there is currently clear ownership of permissions (/feature) policy. Christoph, would your team be interested in this? Reading comment 3, the required changes are on the platform side. I'd be happy to help with front-end dependencies as they come up.

Comment 6

[Johann Hofmann [:johannh]]

The spec seems to be relatively clear about this:

https://www.w3.org/TR/permissions-policy-1/#iframe-allowfullscreen-attribute and https://fullscreen.spec.whatwg.org/#dom-document-fullscreenenabled

...which I would interpret as saying that allow="fullscreen" has the same effects as allowfullscreen including changing document.fullscreenEnabled, but with higher precedence. So allow="fullscreen none" should override document.fullscreenEnabled and the video controls.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Johann Hofmann [:johannh] from comment #6)

The spec seems to be relatively clear about this:

https://www.w3.org/TR/permissions-policy-1/#iframe-allowfullscreen-attribute and https://fullscreen.spec.whatwg.org/#dom-document-fullscreenenabled

...which I would interpret as saying that allow="fullscreen" has the same effects as allowfullscreen including changing document.fullscreenEnabled, but with higher precedence. So allow="fullscreen none" should override document.fullscreenEnabled and the video controls.

Thanks Johann for pointing out what would need to happen. Having flags set to P3, S3 seems about right to me.