1495362 - FeaturePolicy: fullscreen

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Supporting for 'fullscreen' feature policy.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: 4_feature_fullscreen.patch

Comment 2

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

Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

"FeturePolicy"

Comment 3

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

Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

I tried and failed to find a patch which implements 
FeaturePolicyUtils::IsFeatureAllowed.

I assume pages by default allow fullscreen? And then something may disable it?
Would it make sense to have
FeaturePolicyUtils::IsFeatureDenied, if that is the special case?

But anyhow, I can't review this without knowing what 
FeaturePolicyUtils::IsFeatureAllowed does

Comment 4

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

(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #3)
> I tried and failed to find a patch which implements 
> FeaturePolicyUtils::IsFeatureAllowed.
> 
> I assume pages by default allow fullscreen? And then something may disable
> it?
> Would it make sense to have
> FeaturePolicyUtils::IsFeatureDenied, if that is the special case?
Or perhaps FeaturePolicyUtils::IsFeatureAllowed is fine, but it needs to be clear which features are enabled by default. Is that documented somewhere?

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

About the default value, yes I agree with you. But it's not a problem of this particular feature: we should discuss what the default policy value should for any existing feature.

Comment 6

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

Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

I don't see this patch testing the code path. We need to have tests which do X and keep doing X after this patch too. And then tests which ensure that feature policy can enabled or block fullscreen usage. You said there are wpts, but where exactly? I would expect something to be enabled in this bug.


s/FeturePolicy/FeaturePolicy/

Comment 7

[Andrea Marchesini [:baku]]

Attachment: 4_feature_fullscreen.patch

test included

Comment 8

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

Comment on attachment 9014083 [details] [diff] [review]
4_feature_fullscreen.patch

>+++ b/dom/html/HTMLIFrameElement.cpp
>@@ -322,14 +322,18 @@ HTMLIFrameElement::RefreshFeaturePolicy(
>   }
> 
>   mFeaturePolicy->InheritPolicy(OwnerDoc()->Policy());
> 
>   if (AllowPaymentRequest()) {
>     mFeaturePolicy->MaybeSetAllowedPolicy(NS_LITERAL_STRING("payment"));
>   }
> 
>+  if (AllowFullscreen()) {
>+    mFeaturePolicy->MaybeSetAllowedPolicy(NS_LITERAL_STRING("fullscreen"));
>+  }
>+
ok, so this was missing in the previous patch after all.


>+function doRequestFullscreen() {
>+  function handler(evt) {
>+    document.removeEventListener("fullscreenchange", handler);
>+    document.removeEventListener("fullscreenerror", handler);
>+    parent.continueTest(evt.type);
>+  }
Don't you want to call document.exitFullscreen() if fullscreenchange fired.


>+
>+  let test = tests.shift();
>+
>+  // Create an iframe with an allowfullscreen but with allow "fullscreen none".
This comment is wrong. You aren't creating "fullscreen none" but use the values from the tests array.

>+  // The request should be denied, and we should not receive a fullscreenchange
>+  // event in this document.
and then fix also this.


>+  var iframe = document.createElement("iframe");
>+  iframe.setAttribute("allowfullscreen", "true");
>+  iframe.setAttribute("allow", test[0]);
>+  iframe.src = INNER_FILE;
>+  setupForInnerTest("an iframe+allowfullscreen but denied by FeaturePolicy", event => {
And comment needs a fix too



> FullscreenDeniedSubDocFullscreen=Request for fullscreen was denied because a subdocument of the document requesting fullscreen is already fullscreen.
> FullscreenDeniedNotDescendant=Request for fullscreen was denied because requesting element is not a descendant of the current fullscreen element.
> FullscreenDeniedNotFocusedTab=Request for fullscreen was denied because requesting element is not in the currently focused tab.
>+FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because against FeturePolicy directives.
FeturePolicy. And there is something wrong with the sentence.
perhaps
...because of FeaturePolicy directives



Please make sure to run the test on try several times. fullscreen tests haven't been too stable in the past.


(https://fullscreen.spec.whatwg.org/#feature-policy-integration)

Comment 9

[Andrea Marchesini [:baku]]

> (https://fullscreen.spec.whatwg.org/#feature-policy-integration)

Thanks for the review. Do you want to file a spec bug for the default allowlist? Spec says:
"This specification defines a policy-controlled feature identified by the string "fullscreen". Its default allowlist is 'self'."

But we are implementing 'all' for backward compatibility.

Comment 10

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

But allowfullscreen makes the value all, right?

All I'm hoping is that pages which _don't_ use feature policy, don't break with all the feature policy stuff.

Comment 11

[Andrea Marchesini [:baku]]

> But allowfullscreen makes the value all, right?

if allowfullscreen is an attribute, it creates a FeaturePolicy 'fullscreen *' if we don't have any other 'fullscreen' declared directives. I'll add a test for this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cafd172dc48a
FeaturePolicy: fullscreen, r=smaug

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cafd172dc48a

Comment 14

[Francesco Lodolo [:flod]]

Comment on attachment 9014083 [details] [diff] [review]
4_feature_fullscreen.patch

Review of attachment 9014083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +69,5 @@
>  FullscreenDeniedLostWindow=Request for fullscreen was denied because we no longer have a window.
>  FullscreenDeniedSubDocFullscreen=Request for fullscreen was denied because a subdocument of the document requesting fullscreen is already fullscreen.
>  FullscreenDeniedNotDescendant=Request for fullscreen was denied because requesting element is not a descendant of the current fullscreen element.
>  FullscreenDeniedNotFocusedTab=Request for fullscreen was denied because requesting element is not in the currently focused tab.
> +FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because against FeturePolicy directives.

FeaturePolicy, I guess?

Comment 15

[Francesco Lodolo [:flod]]

BTW, why is the string in the patch different from what actually landed?
https://hg.mozilla.org/mozilla-central/rev/cafd172dc48a#l7.12

> +FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because of FeturePolicy directives.

P.S: no clue why opening a NI reset the QA Contact…

Comment 16

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

Hrm, I mentioned about FeturePolicy misspelling several times here. comment 2 and comment 6 and comment 8.

Comment 17

[Andrea Marchesini [:baku]]

I didn't find that misspelling in the patch. I thought it was in the code and not in the properties file. I was looking in the wrong place. I'm filing a bug to fix this typo.

Comment 18

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/de/docs/Web/HTTP/Headers/Feature-Policy
https://developer.mozilla.org/de/docs/Web/HTTP/Headers/Feature-Policy/fullscreen