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).

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 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?

(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.

Now with denying requests from the non-focused tab.

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" ?

(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

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.

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.

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.

With IsInActiveTab() instead of IsInFocusedTab(). Try Push: https://tbpl.mozilla.org/?tree=Try&rev=f2714974a443

https://hg.mozilla.org/mozilla-central/rev/80831e4a10de https://hg.mozilla.org/mozilla-central/rev/54b7ede5ff39