The default bug view has changed. See this FAQ.

Implement Document.mozFullScreenEnabled

RESOLVED FIXED in mozilla10

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cpearce, Assigned: jaws)

Tracking

({dev-doc-complete})

Trunk
mozilla10
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
s/mozFullScreenAllowed/mozFullScreenEnabled/g
Summary: Implement Document.mozFullScreenAllowed → Implement Document.mozFullScreenEnabled

Updated

6 years ago
Keywords: dev-doc-needed
Blocks: 470628
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Created attachment 563942 [details] [diff] [review]
Patch for bug 689058
Attachment #563942 - Flags: review?(roc)
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.
Attachment #563942 - Flags: review?(roc) → feedback?(roc)
Right. What sort of feedback are you looking for other than that? :-)
(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 :-)
Created attachment 563951 [details] [diff] [review]
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?
Attachment #563942 - Attachment is obsolete: true
Attachment #563942 - Flags: feedback?(roc)
Attachment #563951 - Flags: feedback?(roc)
I don't think I've ever seen a stack-allocated nsContentList...
Stack allocating reference counted objects is bad times all around.

Comment 9

6 years ago
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)
Created attachment 563952 [details] [diff] [review]
Patch for bug 689058 v3

Fixed the nits that people have mentioned.
Attachment #563951 - Attachment is obsolete: true
Attachment #563951 - Flags: feedback?(roc)
Attachment #563952 - Flags: feedback?(roc)
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.
(Reporter)

Comment 12

6 years ago
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.
Created attachment 564011 [details] [diff] [review]
Patch for bug 689058 v4

Updated the code based on the feedback.
Attachment #563952 - Attachment is obsolete: true
Attachment #563952 - Flags: feedback?(roc)
Attachment #564011 - Flags: review?(chris)
(Reporter)

Comment 14

6 years ago
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.
Attachment #564011 - Flags: review?(chris) → review-
Created attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

Updated patch based on feedback from cpearce, as well as switched implementation to use bool instead of PRBool.
Attachment #564011 - Attachment is obsolete: true
Attachment #564232 - Flags: review?(roc)
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.
Attachment #564232 - Flags: review?(roc)
(Reporter)

Comment 17

6 years ago
(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.
(Reporter)

Updated

6 years ago
Attachment #564232 - Flags: superreview?(roc)
Attachment #564232 - Flags: review+
Comment on attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

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

Oops
Attachment #564232 - Flags: superreview?(roc) → superreview+
https://hg.mozilla.org/integration/fx-team/rev/da524e9568b6
Whiteboard: [fixed-in-fx-team]
Looks like you forgot to rev uuids for nsIDOM{HTML,SVG,XML}Document.
https://hg.mozilla.org/mozilla-central/rev/da524e9568b6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Created attachment 564606 [details] [diff] [review]
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.
Attachment #564606 - Flags: superreview?(roc)
Attachment #564606 - Flags: review?(chris)
Attachment #564606 - Flags: superreview?(roc)
Attachment #564606 - Flags: review?(chris)
Attachment #564606 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/8ed606bd0e5e
We generally don't mark these as FIXED until they land in mozilla-central.

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

Updated

5 years ago
Blocks: 694690

Updated

5 years ago
Blocks: 703881

Updated

5 years ago
No longer blocks: 703881
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.