Closed Bug 706672 Opened 8 years ago Closed 8 years ago

Exit DOM full-screen on windowed plugin focus

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox10 --- wontfix

People

(Reporter: cpearce, Assigned: cpearce)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

Instead of denying requests for DOM full-screen when windowed plugins are present, we could instead exit full-screen when a windowed plugin is focused (or if a windowed plugin is focused when the request is processed).
Attached patch Patch v1 (obsolete) — Splinter Review
Hook into the focus manager to detect when we're switching focus to a windowed plugin, and exit full-screen if we're in full-screen mode. We don't need to deny requests for full-screen in documents which contain windowed plugins with this approach.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #579610 - Flags: review?(bugs)
Comment on attachment 579610 [details] [diff] [review]
Patch v1

># HG changeset patch
># Parent d06f27b779c23d6f7b6acc22f3434916821773f1
>Bug 706672 - Revoke DOM full-screen when windowed plugin focused on non-MacOSX systems. r=?
>
>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>@@ -8943,19 +8943,25 @@ nsDocument::IsFullScreenEnabled(bool aCa
>     // nsRunnable!
>     return true;
>   }
> 
>   if (!nsContentUtils::IsFullScreenApiEnabled()) {
>     LogFullScreenDenied(aLogFailure, "FullScreenDeniedDisabled", this);
>     return false;
>   }
>-  if (nsContentUtils::HasPluginWithUncontrolledEventDispatch(this)) {
>-    LogFullScreenDenied(aLogFailure, "FullScreenDeniedPlugins", this);
>-    return false;
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+  if (focusedElement) {
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(focusedElement);
>+    if (nsContentUtils::HasPluginWithUncontrolledEventDispatch(content)) {
>+      LogFullScreenDenied(aLogFailure, "FullScreenDeniedFocusedPlugin", this);
>+      return false;
>+    }
>   }
What if focusedElement is in some other window?
Should we actually allow fullscreen only for the documents in the focused tab?
Or should fullscreen mode focus the document?
Attachment #579610 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #2)
> What if focusedElement is in some other window?
> Should we actually allow fullscreen only for the documents in the focused
> tab?
> Or should fullscreen mode focus the document?

Hmmm... We shouldn't be granting requests for full-screen from the non-focused tab (though currently we are, and the new full-screen document receives focus).

I'll change my patch to deny requests which don't come from the focused tab.
Attached patch Patch v2 (obsolete) — Splinter Review
Now with denying requests from the non-focused tab.
Attachment #579610 - Attachment is obsolete: true
Attachment #580805 - Flags: review?(bugs)
Comment on attachment 580805 [details] [diff] [review]
Patch v2

>+static bool
>+IsInFocusedTab(nsIDocument* aDoc)
>+{
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  if (!fm) {
>+    return false;
>+  }
>+
>+  // Is there a focused DOMWindow?
>+  nsCOMPtr<nsIDOMWindow> focusedWindow;
>+  fm->GetFocusedWindow(getter_AddRefs(focusedWindow));
>+  if (!focusedWindow) {
>+    return false;
>+  }
>+
>+  // Find the common ancestor of aDoc and the focused document.
>+  nsCOMPtr<nsIDOMDocument> domDocument;
>+  focusedWindow->GetDocument(getter_AddRefs(domDocument));
>+  nsCOMPtr<nsIDocument> focusedDoc = do_QueryInterface(domDocument);
>+  nsIDocument* commonAncestor = GetCommonAncestor(focusedDoc, aDoc);
>+  if (commonAncestor == focusedDoc || commonAncestor == aDoc) {
>+    // The focused document is either above or below aDoc in the
>+    // doctree. It must be in the same tab (doctree branch), or
>+    // the request is coming from the chrome document.
>+    return true;
>+  }
>+
>+  return false;
>+}
I don't understand how this works if chrome is focused and aDoc is a hidden tab.



> FullScreenDeniedNotInDocument=Request for full-screen was denied because requesting element is no longer in its document.
> FullScreenDeniedMovedDocument=Request for full-screen was denied because requesting element has moved document.
> FullScreenDeniedLostWindow=Request for full-screen was denied because we no longer have a window.
> FullScreenDeniedSubDocFullScreen=Request for full-screen was denied because a subdocument of the document requesting full-screen is already full-screen.
> FullScreenDeniedNotDescendant=Request for full-screen was denied because requesting element is not a descendant of the current full-screen element.
>+FullScreenDeniedNotFocusedTab=Request for full-screen was denied because requesting element is in the currently focused tab.
"is" -> "is not" ?
Attachment #580805 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> >+IsInFocusedTab(nsIDocument* aDoc)
> I don't understand how this works if chrome is focused and aDoc is a hidden
> tab.

Ah right, I missed that. I'd better add a special case so that we deny requests made while the chrome document is focused which don't come from the chrome document:

static bool
IsInFocusedTab(nsIDocument* aDoc)
{
  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
  if (!fm) {
    return false;
  }

  // Is there a focused DOMWindow?
  nsCOMPtr<nsIDOMWindow> focusedWindow;
  fm->GetFocusedWindow(getter_AddRefs(focusedWindow));
  if (!focusedWindow) {
    return false;
  }

  // Retreive the focused document.
  nsCOMPtr<nsIDOMDocument> domDocument;
  focusedWindow->GetDocument(getter_AddRefs(domDocument));
  nsCOMPtr<nsIDocument> focusedDoc = do_QueryInterface(domDocument);

  // When the chrome document is focused, only grant requests from the
  // chrome document.
  if (nsContentUtils::IsChromeDoc(focusedDoc)) {
    return aDoc == focusedDoc;
  }      

  // Find the common ancestor of aDoc and the focused document.
  nsIDocument* commonAncestor = GetCommonAncestor(focusedDoc, aDoc);
  if (commonAncestor == focusedDoc || commonAncestor == aDoc) {
    // The focused document is either above or below aDoc in the
    // doctree. It must be in the same tab (doctree branch).
    return true;
  }

  return false;
}

> >+FullScreenDeniedNotFocusedTab=Request for full-screen was denied because requesting element is in the currently focused tab.
> "is" -> "is not" ?

Thanks! :)
Backed out for build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=80831e4a10de
{
../../../dom/base/nsFocusManager.cpp: In member function 'void nsFocusManager::SetFocusInner(nsIContent*, PRInt32, bool, bool)':
../../../dom/base/nsFocusManager.cpp:1201:70: error: no matching function for call to 'nsContentUtils::ReportToConsole(nsContentUtils::PropertiesFile, const char [37], long int, int, long int, const nsAFlatString&, int, int, nsIScriptError::<anonymous enum>, const char [4], nsIDocument*)'
../../dist/include/nsContentUtils.h:773:19: note: candidate is: static nsresult nsContentUtils::ReportToConsole(PRUint32, const char*, nsIDocument*, nsContentUtils::PropertiesFile, const char*, const PRUnichar**, PRUint32, nsIURI*, const nsAFlatString&, PRUint32, PRUint32)
make[6]: *** [nsFocusManager.o] Error 1
}

Did this fail on try?

https://hg.mozilla.org/integration/mozilla-inbound/rev/54b7ede5ff39
Target Milestone: mozilla11 → ---
Oops, I wasn't going to r+. Sorry. I want to see the new patch.
(How did I r+ o_O )
(In reply to Ed Morley [:edmorley] from comment #8)
> Backed out for build failures:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=80831e4a10de
> {
> ../../../dom/base/nsFocusManager.cpp: In member function 'void
> nsFocusManager::SetFocusInner(nsIContent*, PRInt32, bool, bool)':
> ../../../dom/base/nsFocusManager.cpp:1201:70: error: no matching function
> for call to 'nsContentUtils::ReportToConsole(nsContentUtils::PropertiesFile,
> const char [37], long int, int, long int, const nsAFlatString&, int, int,
> nsIScriptError::<anonymous enum>, const char [4], nsIDocument*)'
> ../../dist/include/nsContentUtils.h:773:19: note: candidate is: static
> nsresult nsContentUtils::ReportToConsole(PRUint32, const char*,
> nsIDocument*, nsContentUtils::PropertiesFile, const char*, const
> PRUnichar**, PRUint32, nsIURI*, const nsAFlatString&, PRUint32, PRUint32)
> make[6]: *** [nsFocusManager.o] Error 1
> }
> 
> Did this fail on try?
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/54b7ede5ff39

Not when it was on Try. But nsContentUtils::ReportToConsole() changed signature after my Try push but before my m-i push so I didn't notice, and that's the failure we have here.
Attached patch Patch v3 (obsolete) — Splinter Review
Rebased on top of recent landings to mozilla-inbound, and when chrome document is focused only grant requests for full-screen from the chrome document.
Attachment #580805 - Attachment is obsolete: true
Attachment #582126 - Flags: review?(bugs)
In addition to the build failures, the original landing also had test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7964285&tree=Mozilla-Inbound
4358 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_dom_fullscreen_warning.xul | Must be in full-screen mode
4360 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_dom_fullscreen_warning.xul | Should remain in full-screen mode for script initiated key events for VK_CANCEL
4361 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_dom_fullscreen_warning.xul | Should remain in full-screen for VK_CANCEL press - got false, expected true
4362 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_dom_fullscreen_warning.xul | Should receive MozShowFullScreenWarning for VK_CANCEL press - got false, expected true
Yeah, I fixed that in my patch v3 as well.

Try push for patch v3:
https://tbpl.mozilla.org/?tree=Try&rev=edd9eaddd51e
Comment on attachment 582126 [details] [diff] [review]
Patch v3


>+  // Retreive the focused document.
Retrieve 


>+  nsCOMPtr<nsIDOMDocument> domDocument;
>+  focusedWindow->GetDocument(getter_AddRefs(domDocument));
>+  nsCOMPtr<nsIDocument> focusedDoc = do_QueryInterface(domDocument);
>+
>+  // When the chrome document is focused, only grant requests from the
>+  // chrome document.
>+  if (nsContentUtils::IsChromeDoc(focusedDoc)) {
>+    return aDoc == focusedDoc;
>+  }      
Some extra spaces after }

But actually, could this all happen a bit differently.


- If document is in chrome, always allow fullscreen (nsContentUtils::IsChromeDoc()). 

Then separate method IsInActiveTab(nsIDocument* d):
- if document->GetContainer()->QI(nsIDocShell)->isActive == false,
  document isn't in active tab, return false.
- if focusmanager->GetActiveWindow() == document->GetContainer()->QI(nsIDocShellTreeItem)->rootTreeItem->QI(nsIDOMWindow)
  document is in focused tab in the active window, return true.

This would allow fullscreen even when focus is in chrome (but document is in active tab). If we don't want to handle that case,
check if focused window is chrome and deny content requested fullscreen.
Attached patch Patch v4Splinter Review
With IsInActiveTab() instead of IsInFocusedTab().

Try Push:
https://tbpl.mozilla.org/?tree=Try&rev=f2714974a443
Attachment #582126 - Attachment is obsolete: true
Attachment #582126 - Flags: review?(bugs)
Attachment #582173 - Flags: review?(bugs)
Attachment #582173 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/9e986b4770c8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Whiteboard: [inbound]
Depends on: 713632
Depends on: 726474
Depends on: 898757
You need to log in before you can comment on or make changes to this bug.