Open Bug 1627134 Opened 2 years ago Updated 29 days ago

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

Categories

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

defect

Tracking

()

People

(Reporter: ntim, Unassigned, NeedInfo)

References

(Blocks 1 open bug, )

Details

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

See Also: → 1626419

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

Flags: needinfo?(amarchesini)
Priority: -- → P1

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

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.

Severity: normal → --
Flags: needinfo?(amarchesini) → needinfo?(htsai)
Priority: P1 → --

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

Flags: needinfo?(htsai) → needinfo?(pbz)

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.

Flags: needinfo?(pbz) → needinfo?(ckerschb)

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.

Severity: -- → S3
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.