689058 - Implement Document.mozFullScreenEnabled

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need a way to convey to authors that the DOM full-screen API is enabled and requests for full-screen are likely to be granted in the current document. This is so authors know whether they can display a "full-screen" button in their UI. 

Add a readonly boolean attribute to Document, mozFullScreenAllowed. This returns true when the full-screen API is enabled, no windowed plugins are present, and all ancestor documents have the mozallowfullscreen attribute set.

Note this attribute doesn't imply requests will be granted - requests not run in a short-running user-generated event handler will still be denied.

Additionally, we may want to fire events when Document.mozFullScreenAllowed changes, but we're not sure if we actually need this yet or not.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

s/mozFullScreenAllowed/mozFullScreenEnabled/g

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 563942 [details] [diff] [review]
Patch for bug 689058

I noticed that I hadn't implemented the two other checks for windowed plugins and ancestor documents with mozallowfullscreen attribute.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. What sort of feedback are you looking for other than that? :-)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Right. What sort of feedback are you looking for other than that? :-)

Hehe, just wanted a gutcheck to make sure I wasn't headed down the completely wrong path :-)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058 v2

I used cpearce's implementation of searching ancestor iframes for mozallowfullscreen. Should I move the bit of duplicated code to nsContentUtils?

I tried to implement the windowed plugin check, but my nsContentList is length zero when tested on http://pearce.org.nz/full-screen/ with the windowed plugin button. Can you help me out here with what I might be doing wrong?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't think I've ever seen a stack-allocated nsContentList...

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Stack allocating reference counted objects is bad times all around.

Comment 9

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

Comment on attachment 563951 [details] [diff] [review]
Patch for bug 689058 v2

(Few random comments)

>+nsDocument::GetMozFullScreenEnabled(PRBool *aFullScreen)
>+{
>+  NS_ENSURE_ARG_POINTER(aFullScreen);
>+  *aFullScreen = false;
>+  if (!nsContentUtils::IsFullScreenApiEnabled())
>+    return NS_OK;
if (expr) {
  stmt;
}

>+
>+  nsIDocument* rootDocument = GetRootDocument(this);
>+  nsContentList objectContentList(rootDocument, kNameSpaceID_XHTML, nsGkAtoms::object, nsGkAtoms::object);
Please don't use refcounted objects as stack variables. We've had security bugs when such
objects have "leaked" to outside the scope.


>+  for (PRUint32 i = 0; i < objectContentList.Length(false); ++i) {
>+    nsIContent* objectContent = objectContentList.Item(i, false);
>+    NS_ENSURE_TRUE(objectContent, NS_ERROR_UNEXPECTED);
>+    nsIFrame* objectFrame = objectContent->GetPrimaryFrame();
>+    NS_ENSURE_TRUE(objectFrame, NS_ERROR_UNEXPECTED);
>+    nsIWidget* objectWidget = objectFrame->GetNearestWidget();
>+    NS_ENSURE_TRUE(objectWidget, NS_ERROR_UNEXPECTED);
I'm pretty sure we don't want to throw ever here, so replace
NS_ERROR_* with NS_OK or something (I'm not sure what this code is trying to do)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058 v3

Fixed the nits that people have mentioned.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 563952 [details] [diff] [review]
Patch for bug 689058 v3

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

We really need to share code between this attribute and mozRequestFullScreen.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 563952 [details] [diff] [review]
Patch for bug 689058 v3

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

::: content/base/src/nsDocument.cpp
@@ +8671,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsDocument::GetMozFullScreenEnabled(PRBool *aFullScreen)

To share code, can we have nsGenericHTMLElement::MozRequestFullScreen() call this before it proceeds with the request? Then we only have one copy of the mozallowfullscreen check loop.

@@ +8679,5 @@
> +  if (!nsContentUtils::IsFullScreenApiEnabled()) {
> +    return NS_OK;
> +  }
> +
> +  // Ensure that there are no windowed plugins are present.

I wouldn't bother with this check yet. The code to test for windowed plugins hasn't landed yet (Bug 684618), as it depends on bug 90268 which may take a while to debug and land. The patch in bug 684618 adds nsContentUtils::HasPluginWithUncontrolledEventDispatch(nsIDocument*), which we can just use here instead of this loop. We can easily add this check when bug 684618 lands.

::: content/html/content/test/test_fullscreen-api.html
@@ +43,2 @@
>    SpecialPowers.setBoolPref("full-screen-api.enabled", true);
> +  is(document.mozFullScreenEnabled, true, "document.mozFullScreenEnabled should be true if full-screen-api.enabled is true");

The mochitest's iframe does not have a mozallowfullscreen attribute, so mozFullScreenEnabled  will return false when it runs on tinderbox. Do these tests in file_fullscreen-api.html instead.

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +400,5 @@
>     */
>    readonly attribute boolean mozFullScreen;  
>  
>    /**
> +   * Denotes whether the full-screen api is enabled, per the about:config 

Comment is now out of date.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058 v4

Updated the code based on the feedback.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 564011 [details] [diff] [review]
Patch for bug 689058 v4

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

Only one change needed. :) I'm not a content-peer, so we'll also need an additional review from Olli or perhaps Roc once we get this in shape.

::: content/base/src/nsDocument.cpp
@@ +8682,5 @@
> +  // This stops the full-screen from being abused similar to the popups of old,
> +  // and it also makes it harder for bad guys' script to go full-screen and
> +  // spoof the browser chrome/window and phish logins etc.
> +  if (!nsContentUtils::IsFullScreenApiEnabled() ||
> +      !nsContentUtils::IsRequestFullScreenAllowed()) {

Do the nsContentUtils::IsRequestFullScreenAllowed() check in nsGenericHTMLElement::MozRequestFullScreen(), otherwise we can't use mozFullScreenEnabled outside user-generated event handlers, which is what we want it for.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +3399,5 @@
>  }
>  
>  nsresult nsGenericHTMLElement::MozRequestFullScreen()
>  {
> +  nsIDocument* doc = GetOwnerDoc();

if (!nsContentUtils::IsRequestFullScreenAllowed()) {
  return NS_OK;
}

::: content/html/content/test/file_fullscreen-api.html
@@ +144,5 @@
> +
> +      SpecialPowers.setBoolPref("full-screen-api.enabled", false);
> +      is(document.mozFullScreenEnabled, false, "document.mozFullScreenEnabled should be false if full-screen-api.enabled is false");
> +      document.body.mozRequestFullScreen();
> +      ok(!document.mozFullScreen, "Should still be in normal mode, because pref has is not enabled.");

We should really be verifying that requesting full-screen inside an iframe without mozallowfullscreen is denied. Eventually we'll also need to verify that a windowed plugin blocks mozFullScreenEnabled on non-MacOSX platforms. Both of these will be easier once we implement the "mozfullscreendenied" event. We can hold off testing that until "mozfullscreendenied" is implemented.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058 v5

Updated patch based on feedback from cpearce, as well as switched implementation to use bool instead of PRBool.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

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

Get review from Chris first, then ask me.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ -3436,5 @@
> -      // This request is not authorized by the parent document.
> -      return NS_OK;
> -    }
> -    node = nsContentUtils::GetCrossDocParentNode(node);
> -  } while (node);

Who's doing this check now? Seems like you moved it to GetMozFullScreenEnabled, but you still need to check this in MozRequestFullScreen.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Comment on attachment 564232 [details] [diff] [review] [diff] [details] [review]
> [...]
> Who's doing this check now? Seems like you moved it to
> GetMozFullScreenEnabled, but you still need to check this in
> MozRequestFullScreen.

MozRequestFullScreen calls GetMozFullScreenEnabled to ensure the request should be granted before calling nsIDocument::RequestFullScreen() to change to full-screen mode.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

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

Oops

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/da524e9568b6

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like you forgot to rev uuids for nsIDOM{HTML,SVG,XML}Document.

Comment 21

[Rob Campbell [:rc] (:robcee)]

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

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 689058 p2 (updating UUIDs)

Ms2ger pointed out that I forgot to update UUIDs for interfaces that inherit nsIDOMDocument.

This patch simply updates the UUIDs for the other interfaces.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/8ed606bd0e5e

Comment 24

[Rob Campbell [:rc] (:robcee)]

We generally don't mark these as FIXED until they land in mozilla-central.

https://hg.mozilla.org/mozilla-central/rev/8ed606bd0e5e

Comment 25

[Eric Shepherd [:sheppy]]

This was already documented but was flagged as being in Firefox 9, not 10. Updated:

https://developer.mozilla.org/en/DOM/Using_full-screen_mode
https://developer.mozilla.org/en/DOM/document.mozFullScreenEnabled

And mentioned on Firefox 10 for developers.