Exit DOM full-screen on windowed plugin focus

RESOLVED FIXED in mozilla11

Status

()

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

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10 wontfix)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #579610 - Flags: review?(bugs)

Comment 2

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

Comment 3

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

Comment 4

6 years ago
Created attachment 580805 [details] [diff] [review]
Patch v2

Now with denying requests from the non-focused tab.
Attachment #579610 - Attachment is obsolete: true
Attachment #580805 - Flags: review?(bugs)

Comment 5

6 years ago
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+
(Assignee)

Comment 6

6 years ago
(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! :)
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/80831e4a10de
status-firefox10: --- → wontfix
status-firefox11: --- → fixed
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
status-firefox11: fixed → ---
Target Milestone: mozilla11 → ---

Comment 9

6 years ago
Oops, I wasn't going to r+. Sorry. I want to see the new patch.
(How did I r+ o_O )
(Assignee)

Comment 10

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

Comment 11

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

Comment 13

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

Comment 15

6 years ago
Created attachment 582173 [details] [diff] [review]
Patch v4

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)

Updated

6 years ago
Attachment #582173 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/80831e4a10de
https://hg.mozilla.org/mozilla-central/rev/54b7ede5ff39
Keywords: dev-doc-needed
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e986b4770c8
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/9e986b4770c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Whiteboard: [inbound]

Updated

6 years ago
Depends on: 713632
(Assignee)

Updated

5 years ago
Depends on: 726474

Updated

4 years ago
Depends on: 898757
You need to log in before you can comment on or make changes to this bug.