746885 - Make DOM fullscreen approval UI cooperate with pointer lock

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm doing work to make our keyboard access in fullscreen better (bug 716107). This requires adding UI to approve fullscreen, and enabling key input in fullscreen mode. See attachment 616431 [details] for a screencast of the current proposed UI.

The current proposed fullscreen approval UI requires a button/mouse click in chrome, but if pointer lock is requested, the pointer is hidden, thus buttons are unclickable. We need a way to make the pointer lock not block the fullscreen approval UI from working, but we still want to honour the pointer lock request once fullscreen is approved.

So I propose:
1. In nsDocument::RequestPointerLock(Element* aElement) after we've decided to grant pointer lock (when ShouldLockPointer() returns true, before calling SetPointerLock(aElement,aCursorStyle)) we check whether the document's nodeprincipal's URI has "fullscreen" permission. If so we proceed as usual. If not we add an observer to the nsIObserverService listening for "fullscreen-approved" and "fullscreen-denied" messages, save a (weak?) reference to aElement, and return.
2. When the approval UI gets an approve/deny user action/button press, it notifies the observers via the observer service with a "fullscreen-{approved,denied}" message. The document's observer receives this notification and if it's denied, sends a pointerlockerror event. If it's approved, it continues the rest of the pointer lock request, which may still fail or succeed.
3. If the approval UI is showing while document.mozCancelFullScreen() is called, it dispatched a "fullscreen-denied" notification, causing a pointerlockerror event to be dispatched as per step 2.

This proposal has the following drawback: script can infer that the request for fullscreen (with pointer lock) was denied. Arguably script can infer this anyway by listening to mousemove events and detecting the mouse tracking towards the fullscreen approval UI, followed by a mozfullscreenchange as the document leaves fullscreen when fullscreen is denied.

If this is an issue, instead while we're in unapproved fullscreen mode we could pretend that pointer lock succeeded but not hide the cursor, and while the fullscreen approval UI is showing we could put up a pointer-events:none box over chrome's <browser> element, so that no mouse events are dispatched to content. We'd otherwise pretend to be in pointer lock mode until fullscreen is approved, i.e. document.mozPointerLockElement returns the requesting element, the pointerlockchange event is dispatched, even though we're not actually pointer locked yet. Once fullscreen is approved, we proceed with the pointer lock request, hide the cursor etc. If the request fails at this stage, just dispatch pointerlockchange instead of pointerlockerror. This makes content unusable while the fullscreen approval UI is showing, which may actually be a good thing anyway.

What do you guys think?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I prefer the latter approach. Would be good to get some comments from UX devs.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

With the latter approach script can still infer when fullscreen is granted/denied; mouse events start happening again and we either remain in or leave fullscreen mode.

It also might be confusing to script to receive a pointerlockchange event before the page is actually ready to run pointer locked (because the approval UI is still showing).

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

A more refined proposal, taking into account post-security review fullscreen approval UI:

* Upon requesting pointer-lock (assuming we're in fullscreen), if context document's uri's permission for fullscreen is ALLOW, we grant the pointer lock request. If the permission was UNKNOWN, we add a "perm-changed" observer service notification observer and return. The pointer lock request is saved somewhere, and remains "pending". Note that the "fullscreen" permission shouldn't be able to be DENY while we're in fullscreen, but we should probably deny the pointer lock request in this case just in case. There should only be one pending pointer lock request at one time.
* Exiting fullscreen cancels any pending pointer lock requests, and removes the observer. Note when the user DENYs a fullscreen request with the fullscreen approval UI, we completely exit fullscreen.
* In the observer, upon notification we re-check the document's permissions again, and if permission is ALLOW we grant the pointer-lock request. Note there's no DENY handling; we cancel the pending pointer lock request upon exiting fullscreen.
* When we show the fullscreen approval UI and the user approves entering fullscreen, if "remember decision" is not checked, we add the perm with SESSION duration, and add the uri to a list.
* In browser.js when we completely exiting fullscreen, we iterate over the list of uris and remove the fullscreen perms of each uri. Thus when the user approves fullscreen but doesn't check "remember decision" in the approval UI, we still set the fullscreen permission in the permission manager to ALLOW, but only while we're in fullscreen mode.

Note this also means that we won't re-request approval when request fullscreen while we're already in fullscreen on a domain that has been approved for fullscreen, but not with "remember decision" checked. i.e.: request fullscren, approve but without "remember decision" checked, request fullscreen again for a child document in the same domain, approval UI will show again, even though fullscreen was already approved for that domain in this fullscreen session. With this propsoed change, the permission manager will store ALLOW permission for fullscreen on a domain for the lifetime of the fullscreen session, thus we won't prompt for fullscreen approval again in the above case.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v1: make pointer lock requests pending approval of fullscreen

Based on top of my work in bug 716107 to enable keys in fullscreen and require approval of fullscreen...

When we request pointer lock and the document isn't yet approved for fullscreen, we make the request pending, storing a weak ref to the requesting doc/element in static memory. We listen on the changes to the "fullscreen" permission for the document, and process the pointer lock request when fullscreen is approved by the user.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v1: Make fullscreen approval permission grants temporary unless "remember" checked

This is patch based on my patches in bug 716107, which enables keys in fullscreen mode and adds UI to approve entering fullscreen. I want to land the patches for this bug and bug 716107 at the same time, so that pointer lock isn't broken when bug 716107 lands.

This patch changes the fullscreen approval UI to temporarily add "fullscreen" permissions when fullscreen is approved without the "remember decision for $domain.com" checkbox checked. Gecko listens on "perm-changed" notifications and will activate pending pointer lock requests when it sees the domain being approved for fullscreen (since pointer-lock piggybacks on fullscreen's security model). We add a listener which detects when the chrome doc exits fullscreen and we then remove the permission, ensuring the permission grant was only temporary.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3 v1: Tests for approval UI, and ensure existing pointer lock tests work

Change the pointer lock mochitests so that the domain they run on is already approved for fullscreen, so they'll run as is.

Also add a test that explicitly tests that pending pointer lock requests are granted once the target domain is approved for fullscreen.

Greenish on Try:
https://tbpl.mozilla.org/?tree=Try&rev=e4f9f45dfb07

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 620588 [details] [diff] [review]
Patch 2 v1: Make fullscreen approval permission grants temporary unless "remember" checked

This looks like a rather gross hack. What if Firefox crashes in fullscreen mode?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Then because the permission is only added with session expiration, the perm isn't persistent.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

And if you can think of a better way, I'm all ears. ;)

Comment 10

[Dão Gottwald [:dao]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Then because the permission is only added with session expiration,

I missed that part. Seems fine then.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Dão, Olli, review ping? This is blocking the fullscreen key support, so I'd like to make progress on it if possible.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 620586 [details] [diff] [review]
Patch 1 v1: make pointer lock requests pending approval of fullscreen


>+class nsRequestPointerLock : public nsRunnable
>+{
>+public:
>+  nsRequestPointerLock(Element* aElement, nsIDocument* aDocument)
>+    : mElement(do_GetWeakReference(aElement)),
>+      mDocument(do_GetWeakReference(aDocument))
>+  {
>+  }
>+
>+  NS_IMETHOD Run()
>+  {
>+    nsCOMPtr<nsIDocument> doc(do_QueryReferent(mDocument));
>+    nsCOMPtr<Element> element(do_QueryReferent(mElement));
>+    if (!element || !doc) {
>+      return NS_OK;
>+    }
>+    doc->RequestPointerLock(element);
>+    return NS_OK;
>+  }
>+private:
>+  nsWeakPtr mElement;
>+  nsWeakPtr mDocument;
>+};
This class is only ever temporary in heap.
I don't see reason for weakptr.

>+/* static */
>+nsresult
>+nsDocument::SetPendingPointerLockRequest(Element* aElement)
>+{
>+  // If there's an existing pending pointer lock request, deny it.
>+  ClearPendingPointerLockRequest(true);
What if there is nsRequestPointerLock in the event loop waiting for Run to be called?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 620589 [details] [diff] [review]
Patch 3 v1: Tests for approval UI, and ensure existing pointer lock tests work


>+function setFullscreenAllowed() {
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
enablePrivilege in a normal mochitest? We're trying to get rid of
that stuff.

Could you add the helper functions to SpecialPowers

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 620589 [details] [diff] [review]
Patch 3 v1: Tests for approval UI, and ensure existing pointer lock tests work

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

::: dom/tests/mochitest/pointerlock/fullscreenApproval.js
@@ +1,4 @@
> +
> +function setFullscreenAllowed() {
> +  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +  const Cc = Components.classes;

You should be able to get away with SpecialPowers.wrap(Components) here and remove the need for the enablePrivilege call.

@@ +10,5 @@
> +}
> +
> +function removeFullscreenAllowed() {
> +  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +  const Cc = Components.classes;

Likewise here.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 620586 [details] [diff] [review]
> Patch 1 v1: make pointer lock requests pending approval of fullscreen
> 
> 
> >+class nsRequestPointerLock : public nsRunnable
> >+{
> >+public:
> >+  nsRequestPointerLock(Element* aElement, nsIDocument* aDocument)
> >+    : mElement(do_GetWeakReference(aElement)),
> >+      mDocument(do_GetWeakReference(aDocument))
> >+  {
> >+  }
> >+
> >+  NS_IMETHOD Run()
> >+  {
> >+    nsCOMPtr<nsIDocument> doc(do_QueryReferent(mDocument));
> >+    nsCOMPtr<Element> element(do_QueryReferent(mElement));
> >+    if (!element || !doc) {
> >+      return NS_OK;
> >+    }
> >+    doc->RequestPointerLock(element);
> >+    return NS_OK;
> >+  }
> >+private:
> >+  nsWeakPtr mElement;
> >+  nsWeakPtr mDocument;
> >+};
> This class is only ever temporary in heap.
> I don't see reason for weakptr.

What if the element or document are destroyed in between this event being dispatched and it being run? Do you mean these should be nsCOMPtrs instead? Or just raw pointers?

> What if there is nsRequestPointerLock in the event loop waiting for Run to
> be called?

Ah yes, I need to handle this case. Thanks!

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Never ever raw pointers unless you're 110% sure that they can't refer to deleted objects.
nsCOMPtr/nsRefPtr should be ok.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #14)
> Comment on attachment 620589 [details] [diff] [review]
> You should be able to get away with SpecialPowers.wrap(Components) here and
> remove the need for the enablePrivilege call.

I get permission denied when using SpecialPowers.wrap(Components) in a child window. Works fine in the mochitest main window though.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v2: make pointer lock requests pending approval of fullscreen

Review comments addressed.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3 v2: Tests for approval UI, and ensure existing pointer lock tests work

Added {set,remove}FullscreenAllowed to SpecialPowers.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 620588 [details] [diff] [review]
Patch 2 v1: Make fullscreen approval permission grants temporary unless "remember" checked

>+      var f = function(uri) {
>+        return (function(event) {
>+          if (event.target == document && document.mozFullScreenElement == null) {
>+            // The chrome document has left fullscreen. Remove the temporary permission grant.
>+            Services.perms.remove(uri.host, "fullscreen");
>+            document.removeEventListener("mozfullscreenchange", f, false);
>+          }
>+        })}(this.fullscreenDocUri);

The closure seems unnecessarily complicated. How about:

>      let host = this.fullscreenDocUri.host;
>      function onFullscreenchange(event) {
>        if (event.target == document && document.mozFullScreenElement == null) {
>          // The chrome document has left fullscreen. Remove the temporary permission grant.
>          Services.perms.remove(host, "fullscreen");
>          document.removeEventListener("mozfullscreenchange", onFullscreenchange);
>        }
>      }

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v2: Make fullscreen approval permission grants temporary unless "remember" checked

With the simplified closure. That's much nicer!

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 621920 [details] [diff] [review]
Patch 2 v2: Make fullscreen approval permission grants temporary unless "remember" checked

>+      document.addEventListener("mozfullscreenchange", onFullscreenchange, false);

nit: ", false" is redundant

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf4d89034490
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f2a0ff6945
https://hg.mozilla.org/integration/mozilla-inbound/rev/14018c3a1a23

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf4d89034490
https://hg.mozilla.org/mozilla-central/rev/b8f2a0ff6945
https://hg.mozilla.org/mozilla-central/rev/14018c3a1a23