Last Comment Bug 706672 - Exit DOM full-screen on windowed plugin focus
: Exit DOM full-screen on windowed plugin focus
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
:
Mentors:
Depends on: 898757 713632 726474
Blocks: 545812
  Show dependency treegraph
 
Reported: 2011-11-30 14:38 PST by PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
Modified: 2013-07-27 12:40 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
Patch v1 (17.31 KB, patch)
2011-12-06 23:53 PST, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (25.56 KB, patch)
2011-12-11 17:49 PST, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch v3 (25.72 KB, patch)
2011-12-15 15:25 PST, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch v4 (26.90 KB, patch)
2011-12-15 20:32 PST, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review

Description PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-11-30 14:38:16 PST
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).
Comment 1 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-06 23:53:36 PST
Created attachment 579610 [details] [diff] [review]
Patch v1

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.
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-07 02:28:52 PST
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?
Comment 3 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-07 13:08:11 PST
(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.
Comment 4 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-11 17:49:48 PST
Created attachment 580805 [details] [diff] [review]
Patch v2

Now with denying requests from the non-focused tab.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-14 05:30:59 PST
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" ?
Comment 6 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-14 14:23:46 PST
(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! :)
Comment 7 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-15 13:46:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/80831e4a10de
Comment 8 Ed Morley [:emorley] 2011-12-15 14:17:18 PST
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
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-15 14:21:42 PST
Oops, I wasn't going to r+. Sorry. I want to see the new patch.
(How did I r+ o_O )
Comment 10 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-15 14:24:10 PST
(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.
Comment 11 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-15 15:25:23 PST
Created attachment 582126 [details] [diff] [review]
Patch v3

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.
Comment 12 Matt Brubeck (:mbrubeck) 2011-12-15 15:41:25 PST
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
Comment 13 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-15 16:36:26 PST
Yeah, I fixed that in my patch v3 as well.

Try push for patch v3:
https://tbpl.mozilla.org/?tree=Try&rev=edd9eaddd51e
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-15 16:47:49 PST
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.
Comment 15 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-15 20:32:46 PST
Created attachment 582173 [details] [diff] [review]
Patch v4

With IsInActiveTab() instead of IsInFocusedTab().

Try Push:
https://tbpl.mozilla.org/?tree=Try&rev=f2714974a443
Comment 17 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-18 11:46:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e986b4770c8
Comment 18 Marco Bonardo [::mak] 2011-12-19 05:31:02 PST
https://hg.mozilla.org/mozilla-central/rev/9e986b4770c8

Note You need to log in before you can comment on or make changes to this bug.