Last Comment Bug 737100 - Extend Pointer Lock (Mouse Lock) for non-fullscreen elements
: Extend Pointer Lock (Mouse Lock) for non-fullscreen elements
Status: RESOLVED FIXED
[games:p?]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
:
Mentors:
http://dvcs.w3.org/hg/pointerlock/raw...
: 796992 838138 853171 (view as bug list)
Depends on: 737097 884629 947091 633602 822654 854035 854583 892110 1165306 1165308
Blocks: gecko-games 853160 858082 853171
  Show dependency treegraph
 
Reported: 2012-03-19 11:01 PDT by David Humphrey (:humph)
Modified: 2015-05-15 06:25 PDT (History)
24 users (show)
curtisk: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
verified
22+


Attachments
WIP, without permission handling (13.41 KB, patch)
2013-03-15 15:55 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
patch, v1 (41.58 KB, patch)
2013-03-16 16:41 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
patch (41.66 KB, patch)
2013-03-16 17:28 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
patch, v3 (42.64 KB, patch)
2013-03-17 00:36 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v4 (52.19 KB, patch)
2013-03-17 11:40 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v5 (52.57 KB, patch)
2013-03-17 16:00 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
cpearce: review-
Details | Diff | Splinter Review
v7 (55.15 KB, patch)
2013-03-20 06:46 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v8 (55.15 KB, patch)
2013-03-20 09:33 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
david.humphrey: feedback+
Details | Diff | Splinter Review
v9 (55.17 KB, patch)
2013-03-21 03:13 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v9 with some tweaks (1.27 KB, patch)
2013-03-21 09:49 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
v9 with some tweaks (55.10 KB, patch)
2013-03-21 09:51 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
cpearce: review+
Details | Diff | Splinter Review
Patch v.10 (92.41 KB, patch)
2013-03-21 18:59 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
v11 (93.35 KB, patch)
2013-03-22 04:00 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v10 -> v11 interdiff (10.60 KB, patch)
2013-03-22 04:03 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
dolske: ui‑review-
Details | Diff | Splinter Review
Patch v.12 (75.90 KB, patch)
2013-03-22 17:09 PDT, Justin Dolske [:Dolske]
bugs: review-
Details | Diff | Splinter Review
Screenshot (148.53 KB, image/png)
2013-03-22 17:11 PDT, Justin Dolske [:Dolske]
cpearce: feedback-
Details
Patch v.13 (77.87 KB, patch)
2013-03-23 19:29 PDT, Justin Dolske [:Dolske]
bugs: review+
Details | Diff | Splinter Review
patch (2.68 KB, patch)
2013-03-24 06:02 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review

Description David Humphrey (:humph) 2012-03-19 11:01:36 PDT
The first iteration of Pointer Lock (Mouse Lock) is requiring fullscreen elements in order to acquire a lock.  Google has done the same, but the API is meant to be used in non-fullscreen mode as well.  There are good reasons why one would want to not go fullscreen, among them performance with large canvases.

The code in bug 633602 overcomes many issues by assuming fullscreen, which will have to be untangled once we allow non-fullscreen elements.

I'm filing this so we don't lose track of it.  I am not clear when we want to do this.
Comment 1 Chris Pearce (:cpearce) 2012-04-22 19:52:57 PDT
We probably need some kind of UI to request permission, similar to geolocation's UI.
Comment 2 David Humphrey (:humph) 2012-05-28 11:08:34 PDT
It looks like this might have landed in Chrome already - https://twitter.com/Tojiro/status/207167169291751424
Comment 3 David Humphrey (:humph) 2012-05-28 11:09:50 PDT
Confirmed: https://twitter.com/Vincent_Scheib/status/207171218414125056
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-05-28 11:16:40 PDT
We should check what their permission model is and whether it is strong enough.
Comment 5 Vincent Scheib (scheib@chromium.org) 2012-05-29 09:40:43 PDT
The user should not be harassed by misbehaving sites. W.r.t. Pointer Lock that means a site that rapidly requests lock (or requests unlock & lock).

To protect against that, Chrome's implementation will only permit mouse lock if a user gesture (mouse click, or keyboard press...) was used to enter the mode. If a user has exited mouse lock, the site can not reaquire it without the user clicking on site content. The user may still view the site, and navigate away using the browser's controls.

A special consideration is made with fullscreen, because it also requires a user gesture and is exited by the user in the same way (pressing ESC key). Chrome does not require a 2nd user gesture for mouse lock once fullscreen is already enabled - mouse lock is permitted at any time.

An exception is also made when the application has initiated a mouse unlock. In that case, Chrome's implementation will (in the future, not landed yet, crbug.com/113460) allow a subsequent lock of the mouse without a user gesture and without producing exit instructions. This removes any nuisance for applications that frequently switch  in and out of mouse lock, such as a game with an inventory menu that desires to use the system cursor.
Comment 6 Chris Pearce (:cpearce) 2012-06-12 20:09:47 PDT
Seems like this is the sort of thing we'd like for Kilimanjaro.
Comment 7 JP Rosevear [:jpr] 2012-06-19 13:21:50 PDT
Not blocking b2g right now.
Comment 8 Sheila Mooney 2012-06-20 13:06:53 PDT
Need to check with bus dev to see if our partners require this and reopen if so.
Comment 9 Florian Bösch 2012-10-02 11:28:31 PDT
Current implementation in FF 15 is in non conformance to the specification: http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html

Test page there: http://codeflow.org/issues/pointerlock-test.html
Comment 10 Chris Pearce (:cpearce) 2012-10-02 13:28:52 PDT
How does our implementation not conform to the specification? Please be more specific.
Comment 11 Florian Bösch 2012-10-02 13:47:06 PDT
If you click on the buttons of the linked test page for "pointerlock" or "fullscreen and pointerlock" nothing happens in FF, but it works in chrome. In determining which behavior is correct you can consult the specification.

The specification states the conditions under which requestPointerLock has to succeed (http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html#methods), the conditions are:

1) Sandboxed pointerlock flag is not set
2) AND the window is in focus
3) AND the user agent is the active application
4) AND the target is in the DOM tree
5) AND no other document has acquired pointer lock

FF15 does not succeed under these conditions. Therefore the implementation is not in conformance with the specification. In particular the specification does not define the failure criteria currently used by firefox (calling requestPointerLock in the fullscreen change event).

Since that is so, Chromes behavior is correct, and Firefoxes is not, making this a bug.
Comment 12 Chris Pearce (:cpearce) 2012-10-02 13:57:20 PDT
It seems to me that you're trying to say that Firefox should grant pointer lock requests only not in fullscreen mode? That's what this bug is tracking.
Comment 13 Florian Bösch 2012-10-02 14:00:30 PDT
I've filed another bug https://bugzilla.mozilla.org/show_bug.cgi?id=796992 that details my objection precisely. The failure mode for the requestPointerLock in Firefox (being required to be called from the fullscreen change event) is not in the specification. So Firefox exceeds the specification in an unspecified way, which I consider a bug.
Comment 14 Chris Pearce (:cpearce) 2012-10-02 14:08:22 PDT
*** Bug 796992 has been marked as a duplicate of this bug. ***
Comment 15 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-10-02 14:58:20 PDT
Note, Chrome has its own restrictions too, which don't exactly comply with the spec.
(assuming comment 5 is still valid). Chrome requires user input before  pointer can be locked.
Comment 16 Al Billings [:abillings] 2012-10-03 14:09:36 PDT
Assign sec review to Curtis to set up full security team review.
Comment 17 Martin Best (:mbest) 2012-10-18 15:42:54 PDT
Just wanted to add that this has come up in conversations with game devs recently.  Which is why I found this bug and I am commenting.  I feel this is rising in importance now that people are seeing the potential for products that can take advantage of PointerLock.
Comment 18 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-12-18 06:39:32 PST
Filled bug 822654 to get this scheduled
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-02-05 05:52:18 PST
*** Bug 838138 has been marked as a duplicate of this bug. ***
Comment 20 Chris Pearce (:cpearce) 2013-02-25 18:32:55 PST
Security review completed today in bug 822654. The design we discussed was:
* Keep existing behaviour of pointer lock in fullscreen, i.e. assume that pointer lock is approved in fullscreen mode.
* In windowed mode, requests for pointer lock are automatically failed if they're not in user generated event handlers.
* In windowed mode, when a pointer lock request is made we drop down a permission dialog in a door hanger off the chrome (similar to the geoloation permission dialog) which asks permission before entering pointer lock. I think we should fail the pointerlock request if the user dismisses the door hanger.
* We should provide UI for remembering the pointerlock permission grant, and warn when entering pointerlock under a previously granted permission.
* Granting permission to enter PL but not opting to remember the grant still remembers the grant for the lifetime of the current document.
* Activating the chrome via touch controls (say in Metrofox or some other touch UI) should temporarily suspend PL, but not cause it to exit.
Comment 21 Vincent Scheib (scheib@chromium.org) 2013-03-04 10:01:45 PST
(In reply to Chris Pearce (:cpearce) from comment #20)
> Security review completed today in bug 822654. The design we discussed was:
> * Keep existing behaviour of pointer lock in fullscreen, i.e. assume that
> pointer lock is approved in fullscreen mode.
> * In windowed mode, requests for pointer lock are automatically failed if
> they're not in user generated event handlers.
> * In windowed mode, when a pointer lock request is made we drop down a
> permission dialog in a door hanger off the chrome (similar to the geoloation
> permission dialog) which asks permission before entering pointer lock. I
> think we should fail the pointerlock request if the user dismisses the door
> hanger.
> * We should provide UI for remembering the pointerlock permission grant, and
> warn when entering pointerlock under a previously granted permission.
> * Granting permission to enter PL but not opting to remember the grant still
> remembers the grant for the lifetime of the current document.
> * Activating the chrome via touch controls (say in Metrofox or some other
> touch UI) should temporarily suspend PL, but not cause it to exit.

Did you consider, and reject, this block from comment 5:
"
An exception is also made when the application has initiated a mouse unlock. In that case, Chrome's implementation will (in the future, not landed yet, crbug.com/113460) allow a subsequent lock of the mouse without a user gesture and without producing exit instructions. This removes any nuisance for applications that frequently switch  in and out of mouse lock, such as a game with an inventory menu that desires to use the system cursor.
"

Or, was it overlooked? This was implemented after applications such as http://www.cloudparty.com/ (and others) noted the user nuisance of a dialog shown as the app exits and enters pointer locks several times when switching between modes using the mouse cursor and using pointer lock.
Comment 22 Jesse Ruderman 2013-03-04 10:23:37 PST
We did not discuss that.

It seems safe to allow reactivating pointer-lock iff nothing has happened that would have caused pointer-lock to be deactivated.  For example, if the content area loses focus or the user presses Esc, the page would lose the ability to silently reactivate pointer-lock.

That policy would have the odd side-effect that closing your game inventory *might* show the warning again, depending on what you did while the inventory was up.  But a more relaxed policy would probably allow web sites to do sneaky things to maintain pointer-lock when they shouldn't be able to.

What does Chrome do?
Comment 23 Vincent Scheib (scheib@chromium.org) 2013-03-04 12:11:42 PST
In Chrome, if lock is being permitted and the application had previously voluntarily released lock, it will not re-show exit instructions. There is no permission logic difference, only a difference in if the exit instructions are reshown.

Code:
https://code.google.com/p/chromium/source/search?q=RequestToLockMouse+last_unlocked_by_target+file%3Afullscreen_controller.cc&origq=RequestToLockMouse+last_unlocked_by_target+file%3Afullscreen_controller.cc&btnG=Search+Trunk
Comment 24 Vincent Scheib (scheib@chromium.org) 2013-03-04 12:14:10 PST
(In reply to Vincent Scheib (scheib@chromium.org) from comment #23)
> In Chrome, if lock is being permitted and the application had previously
> voluntarily released lock, it will not re-show exit instructions. There is
> no permission logic difference, only a difference in if the exit
> instructions are reshown.

Whoops, misspoke after too long away from code! There is a logic change, in that it doesn't require a user gesture to relock if the application unlocked.

> 
> Code:
> https://code.google.com/p/chromium/source/
> search?q=RequestToLockMouse+last_unlocked_by_target+file%3Afullscreen_control
> ler.
> cc&origq=RequestToLockMouse+last_unlocked_by_target+file%3Afullscreen_control
> ler.cc&btnG=Search+Trunk
Comment 25 Florian Bösch 2013-03-04 12:25:05 PST
I have usecases (in productivity software) where temporary lock is beneficial (such as modifying viewports/dragging value entrie fields/color pickers etc). It has previously been argued that in such situations developers should provide their own pointer. However this suggestion is only valid for exclusive/forefront applications (like games) and it is very inconvenient for others (productivity applications). I term this concept "pointer-lock-drag" because it's acquiring lock on mousedown, records dragging and releases lock on mouse up.
Comment 26 Sam Thompson 2013-03-04 13:06:09 PST
In my opinion the most interesting use cases for WebGL are where HTML UI elements and 3D graphics intermix (as opposed to just putting a full screen 3d game into a browser). In this case, you want to be able to drag around in the 3D environment and then quickly transition to clicking on HTML elements in the page. We use this pattern extensively in Cloud Party (www.cloudparty.com), and it's very powerful. We achieve the temporary pointer lock in Chrome by locking on mouse down and releasing on mouse up, so the permission window only asks you the first time, which is workable, but can be confusing that first time when a user is most uneasy with the UI. Still, this works well enough, and we would be very happy if Firefox supported a similar concept.

Now, if there was an inherent mode like the suggested "pointer-lock-drag", where while you were dragging the pointer was locked, but was necessarily released when you let go, that would be ideal assuming that it wouldn't require the permission window (since it would only act when click-dragging on the element). This would work well in use cases such as an embedded 3D model viewer, any kind of productivity software like a 3D editor, and really anywhere where you want to blend HTML UI and a 3D scene, such as a Real Time Strategy game.
Comment 27 Florian Bösch 2013-03-04 13:15:42 PST
(In reply to Sam Thompson from comment #26)
> Now, if there was an inherent mode like the suggested "pointer-lock-drag",
> where while you were dragging the pointer was locked, but was necessarily
> released when you let go, that would be ideal assuming that it wouldn't
> require the permission window (since it would only act when click-dragging
> on the element). This would work well in use cases such as an embedded 3D
> model viewer, any kind of productivity software like a 3D editor, and really
> anywhere where you want to blend HTML UI and a 3D scene, such as a Real Time
> Strategy game.

It is possible to synthesize your own mouse events during pointer lock and dispatch them as far as I know to support "virtual" cursors. However the quick enter&exit pattern (or pointer-lock-drag as I call it) does solve a different problem. If you synthesize your own pointer during pointer locked, then switching to another tab/application etc. requires exiting pointerlock, which necessitates re-entering to return, which becomes quite tedious indeed quite quickly.
Comment 28 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-05 12:50:07 PST
I'll try to look at this soon.
Comment 29 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-15 15:55:27 PDT
Created attachment 725643 [details] [diff] [review]
WIP, without permission handling

Looks like we don't have a sane way to add simple doorhanger for this.
So need to do something insane.
Comment 30 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-16 16:41:12 PDT
Created attachment 725811 [details] [diff] [review]
patch, v1

Spec is certainly not clear in various edge cases.
The spec got fixed a bit yesterday, but needs still improvements.
I'll file (spec) bugs.

https://tbpl.mozilla.org/?tree=Try&rev=0e063d8ae3e8
Passed the existing pointerlock tests on linux.
(I temporarily enabled them since they are disabled by default on linux because of tbpl random orange.)
Need to write still tests, but even before that need to get feedback whether
the behavior is ok. Note, I've tested this only on linux so far.
Comment 31 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-16 17:24:01 PDT
Comment on attachment 725811 [details] [diff] [review]
patch, v1

Bah, try doesn't like this. new patch coming.
Comment 32 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-16 17:28:55 PDT
Created attachment 725816 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=949fdd19db6f
Comment 33 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-17 00:36:42 PDT
Created attachment 725848 [details] [diff] [review]
patch, v3

https://tbpl.mozilla.org/?tree=Try&rev=a96101a21990
Comment 34 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-17 07:56:58 PDT
Comment on attachment 725848 [details] [diff] [review]
patch, v3

And better patch coming later today.
(Need to handle the case when there is a permanent permission better.)
Comment 35 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-17 11:40:11 PDT
Created attachment 725909 [details] [diff] [review]
v4

https://tbpl.mozilla.org/?tree=Try&rev=0e3beb46d4c5
Comment 36 Vladimir Vukicevic [:vlad] [:vladv] 2013-03-17 14:53:52 PDT
Just tested on windows; works great!  Only issue that I saw was the permanent permission problem that you mentioned already (it just doesn't seem to remember).
Comment 37 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-17 16:00:40 PDT
Created attachment 725941 [details] [diff] [review]
v5

Yet some changes. Managed to clean up some code, but nsDocument.cpp is getting
complicated. There are just is too many options to start
fullscreen+pointerlock mode, yet support starting just pointerlock...

https://tbpl.mozilla.org/?tree=Try&rev=26b1da8f9d17

Chris, feel free to look at this.
(I think dougt could look at the browser/toolkit/permission handling parts)

One question: what kind of behavior do we want for 
"Always allow pointer locking". Right now after ESC if pointer lock is activated
you do see a warning. Should we not show the warning if pointer lock is activated using user input?
Or should "Always allow pointer locking" only apply to the case when there is user input. In other cases
the normal permission dialog should be shown? Or should we always require user input for pointer lock, and never
show a warning if "Always allow pointer locking" has been selected in the permission dialog?
(I think the last option sounds the best.)
Comment 38 Vladimir Vukicevic [:vlad] [:vladv] 2013-03-17 16:36:12 PDT
Ah -- smaug, I was confused.  I was indeed getting a warning, but I was confused since it still had a dropdown with Yes as the default.  I didn't read the text :)

I think the last option sounds best as well.  We can tweak UI in the future along the way to release; for example, it might be nice to display something somewhere visible (maybe the location bar?) if the pointer is currently locked in non-fullscreen.
Comment 39 :Ehsan Akhgari 2013-03-17 19:31:21 PDT
Comment on attachment 725941 [details] [diff] [review]
v5

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +278,5 @@
> +pointerLock.neverAllowPointerLock=Never allow pointer locking
> +pointerLock.neverAllowPointerLock.accesskey=N
> +pointerLock.title=Would you like to allow the pointer to be locked on %S?
> +pointerLock.autoLockIitle=%S will lock the pointer.
> +pointerLock.cancel=Press ESC at any time to cancel pointer lock.

Nit: it would be a good idea to add a LOCALIZATION NOTE here explaining what "pointer locking" means.
Comment 40 Chris Pearce (:cpearce) 2013-03-17 21:28:21 PDT
(In reply to Olli Pettay [:smaug] from comment #37)
> Created attachment 725941 [details] [diff] [review]
> v5
> 
> Yet some changes. Managed to clean up some code, but nsDocument.cpp is
> getting
> complicated. There are just is too many options to start
> fullscreen+pointerlock mode, yet support starting just pointerlock...

Yeah, I've been thinking we need to refactor the fullscreen and pointerlock bits out into some kind of manager object.

> 
> https://tbpl.mozilla.org/?tree=Try&rev=26b1da8f9d17
> 
> Chris, feel free to look at this.
> (I think dougt could look at the browser/toolkit/permission handling parts)
> 
> One question: what kind of behavior do we want for 
> "Always allow pointer locking". Right now after ESC if pointer lock is
> activated
> you do see a warning. Should we not show the warning if pointer lock is
> activated using user input?
> Or should "Always allow pointer locking" only apply to the case when there
> is user input. In other cases
> the normal permission dialog should be shown? Or should we always require
> user input for pointer lock, and never
> show a warning if "Always allow pointer locking" has been selected in the
> permission dialog?
> (I think the last option sounds the best.)

I think we should require user input and show the warning dialog (not an approval dialog) if either always allow or allow have previously been selected for a page *and* the user previously exited pointerlock by pressing ESC, with the exception that we'll allow requests without user input if script voluntarily gave up pointer lock (as per comments 20-24).

The security team approved of the idea of requiring user input in the security review.

Other behaviour issues (I'm testing on Win8):
* After a while playing around with pointer lock on my page at http://pearce.org.nz/fullscreen/pointerlock-position.html, I started to see an error logged to console when I clicked the "Yes" approve pointer lock on the doorhanger: "JavaScript error: resource://gre/modules/PopupNotifications.jsm, line 727: TypeError: notification.mainAction is null" . The approval doorhanger didn't hide, but bluring and then re-focusing the window caused the door hanger to hide, and pointer lock then entered. I don't know how to reliably reproduce this though...
* Setting the permission in Page Info for "lock the pointer" from "Allow" to "Always ask" doesn't take effect until you close the Page Info window, whereas changing the permission for "Enter fullscreen" takes effect immediately. Actually sometimes changing the permission for "lock the pointer" doesn't work at all in Page Info.
* Request pointer lock, select "Always allow", press ESC, request again, the approval doorhanger shows, and click close on the approval dialog or select "Not now" in the combobox, the doorhanger hides, and pointer lock is entered. I'd expect us not to show a warning and not the approval dialog again, but if you're going to show the approval dialog, then the close/not now buttons shouldn't assume that pointer lock is entered.
* If a page has an "always allow" permission grant the "max 3 cancellations" logic doesn't seem to kick in and block future requests after 3 canceled requests.
Comment 41 Chris Pearce (:cpearce) 2013-03-17 21:32:06 PDT
Comment on attachment 725941 [details] [diff] [review]
v5

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

r- because we should only approve pointerlock requests in user generated event handlers, with the exception I mentioned above. And the other issues I pointed out above need to be addressed too, though many of them are on the xul side.

::: content/base/src/nsDocument.cpp
@@ +9858,5 @@
>    }
>  
>    NS_IMETHOD Run()
>    {
> +    static_cast<nsDocument*>(mDoc.get())->

Why do you reset this in the document? You should say why you need to in a comment here.

@@ +10579,5 @@
> +  }
> +
> +  // Note, we must bypass focus change, so pass true as the last parameter!
> +  if (!static_cast<nsDocument*>(d.get())->
> +        ShouldLockPointer(e, pointerLockedElement, true)) {

The incoming convention for boolean parameters seems to be to add a comment with the parameter's name, i.e.:

ShouldLockPointer(e, pointerLockedElement, /* aNoFocusCheck */ true)

@@ +10683,5 @@
>      return false;
>    }
>  
> +  if (static_cast<nsDocument*>(aElement->OwnerDoc())->
> +       mCancelledPointerLockRequests > 3) {

This "3" should be a suitably named #define or a pref.

I couldn't actually figure out how to hit this limit in practice...

::: content/base/src/nsDocument.h
@@ +1280,5 @@
> +  bool mAllowRelocking:1;
> +
> +  bool mAsyncFullscreenPending:1;
> +
> +  uint32_t mCancelledPointerLockRequests;

I think you should have comments on these explaining what they are and why they exist.
Comment 42 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-18 01:12:41 PDT
(In reply to Chris Pearce (:cpearce) from comment #40)
> * Setting the permission in Page Info for "lock the pointer" from "Allow" to
> "Always ask" doesn't take effect until you close the Page Info window,
> whereas changing the permission for "Enter fullscreen" takes effect
> immediately. Actually sometimes changing the permission for "lock the
> pointer" doesn't work at all in Page Info.
Hmm, I'm doing whatever geolocation does.


> * Request pointer lock, select "Always allow", press ESC, request again, the
> approval doorhanger shows
Crap, when did that regress. it was working :)
Comment 43 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-18 03:29:00 PDT
(In reply to Chris Pearce (:cpearce) from comment #41)
> @@ +10683,5 @@
> >      return false;
> >    }
> >  
> > +  if (static_cast<nsDocument*>(aElement->OwnerDoc())->
> > +       mCancelledPointerLockRequests > 3) {
> 
> This "3" should be a suitably named #define or a pref.
> 
> I couldn't actually figure out how to hit this limit in practice...
If the page tries to lock the pointer several times and user
Comment 44 Chris Pearce (:cpearce) 2013-03-18 14:00:42 PDT
(In reply to Olli Pettay [:smaug] from comment #43)
> (In reply to Chris Pearce (:cpearce) from comment #41)
> > @@ +10683,5 @@
> > >      return false;
> > >    }
> > >  
> > > +  if (static_cast<nsDocument*>(aElement->OwnerDoc())->
> > > +       mCancelledPointerLockRequests > 3) {
> > 
> > This "3" should be a suitably named #define or a pref.
> > 
> > I couldn't actually figure out how to hit this limit in practice...
> If the page tries to lock the pointer several times and user

Yeah, I was able to hit this limit when testing another time after I wrote this comment. I think I'd previously set an "always allow" approval when I was trying to test the cancellation limit, and that may be why it didn't trigger.
Comment 45 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-20 06:46:22 PDT
Created attachment 727176 [details] [diff] [review]
v7

https://tbpl.mozilla.org/?tree=Try&rev=91ae6e93ab92
Some manual testing for this would be nice.

At least I get the right permission handling.

Vlad, feel free to test.
Comment 46 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-20 09:25:41 PDT
Comment on attachment 727176 [details] [diff] [review]
v7

Thanks to humph I noticed a bug. Better patch coming.
Comment 47 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-20 09:33:50 PDT
Created attachment 727252 [details] [diff] [review]
v8

Just one line change to v7.

https://tbpl.mozilla.org/?tree=Try&rev=7800f2d169ea
Comment 48 David Humphrey (:humph) 2013-03-20 09:36:57 PDT
I did a bunch of testing on smaug's v7 patch.  We found a bug, which he's fixed.  Another point I want to raise here, and maybe others can test (smaug can't hit this).  When we originally did the pointer lock patch (fullscreen only), there was an edge case where if you had multiple monitors, you could move your mouse fast and break out of the browser window while locked, and end up with a pointer on the other monitor.  This patch to allow for non-fullscreen locking makes this much more noticable.  On my iMac with a regular Apple mouse, I'm able to easily break out of the browser while locked.  The same is true in a Windows VM.

If you want to try to reproduce this:

* http://www.html5rocks.com/en/tutorials/pointerlock/intro/
* Find the "Click Me!" button, click it
* Allow pointer lock in the door hanger
* Notice you get locked
* Try moving really fast, notice you can get a mouse pointer outside the browser
* Move back toward the browser, notice the arrow vanish

I'll re-test on smaug's new build to see if this bug he fixed also helps with this.  If not, we might want another bug.
Comment 49 Vladimir Vukicevic [:vlad] [:vladv] 2013-03-20 13:49:19 PDT
Separate bug on that please if it shows up :)
Comment 50 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-20 13:56:03 PDT
(In reply to Olli Pettay [:smaug] from comment #47)
> https://tbpl.mozilla.org/?tree=Try&rev=7800f2d169ea

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-7800f2d169ea/

Feel free to test.

http://www.html5rocks.com/en/tutorials/pointerlock/intro/ worked ok, and
so did http://media.tojicode.com/q3bsp/
Comment 51 David Humphrey (:humph) 2013-03-20 14:10:43 PDT
Comment on attachment 727252 [details] [diff] [review]
v8

This version works great for me.  I still see the issue I described above, but it's less frequent now.  I'll file a follow-up.  Nice work.
Comment 52 Chris Pearce (:cpearce) 2013-03-20 19:28:12 PDT
(In reply to Olli Pettay [:smaug] from comment #47)
> Created attachment 727252 [details] [diff] [review]
> v8

I get a linker error when building this patch locally:

18:25.20 DocumentBinding.obj : error LNK2019: unresolved external symbol "public: static void __cdecl nsIDocument::UnlockPointer(void)" (?UnlockPointer@nsIDocum
ent@@SAXXZ) referenced in function "bool __cdecl mozilla::dom::DocumentBinding::mozExitPointerLock(struct JSContext *,class JS::Handle<class JSObject *>,class n
sIDocument *,unsigned int,class JS::Value *)" (?mozExitPointerLock@DocumentBinding@dom@mozilla@@YA_NPAUJSContext@@V?$Handle@PAVJSObject@@@JS@@PAVnsIDocument@@IP
AVValue@6@@Z)
18:25.20
18:25.20 xul.dll : fatal error LNK1120: 1 unresolved externals

I... don't understand that... I'm building on Win8 with Visual Studio 2012.
Comment 53 Chris Pearce (:cpearce) 2013-03-20 20:13:48 PDT
Hmm... a clobber fixed my build error... must have had stable objdir somehow...
Comment 54 Chris Pearce (:cpearce) 2013-03-20 20:37:33 PDT
(In reply to Olli Pettay [:smaug] from comment #50)
> (In reply to Olli Pettay [:smaug] from comment #47)
> > https://tbpl.mozilla.org/?tree=Try&rev=7800f2d169ea
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.
> com-7800f2d169ea/
> 

I tested this build on Win8. It seems to work fine, except for a few remaining niggles:

* Upon entering a pre-approved pointer lock, I think you should display the warning message for 1 maybe 2 seconds longer. It disappears too quickly to be read.
* The mouse cursor is still visible while the warning message is showing. I think in the pre-approved case you should lock the pointer before showing the warning, so that users don't have to wait for the warning to hide to interact with their game or whatever. And once you do that, there's no need to have a close button on the warning, since the mouse can't be moved to hit it anyway.
* The "Page Info" permissions grants still seem buggy. Setting the page's "lock the pointer" permission to "block" in "Page Info" seems to render the page unblockable, i.e. once you set the permission to block, setting the permission to something other than blocked doesn't un-block the pointer lock on that page until you restart the browser.

I found some existing bugs while testing:
* If you click and drag the mouse while the pointer is locked, text can become selected. I filed bug 853292 for this.
* While pointer locked, right clicking brings up a context menu overlaying content. We should disable the context menu while pointer is locked. I filed bug 853293 for this.

And yeah, I think you should get someone more qualified than I to review the toolkit/ parts of this patch. ;)
Comment 55 Chris Pearce (:cpearce) 2013-03-20 21:16:11 PDT
If a non-pre-approved pointer lock request is made, and then a fullscreen request is made, a pointerlockerror event is dispatched. Is that intentional?
Comment 56 Chris Pearce (:cpearce) 2013-03-20 21:37:13 PDT
Comment on attachment 727252 [details] [diff] [review]
v8

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

::: content/base/src/nsDocument.cpp
@@ +10590,5 @@
> +  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocument);
> +  nsDocument* d = static_cast<nsDocument*>(doc.get());
> +  if (!e || !d || gPendingPointerLockRequest != this ||
> +      e->GetCurrentDoc() != d ||
> +      (!mUserInputOrChromCaller && !d->mIsApprovedForFullscreen)) {

s/mUserInputOrChromCaller/mUserInputOrChromeCaller/g
(missing 'e' in 'Chrome').

@@ +10593,5 @@
> +      e->GetCurrentDoc() != d ||
> +      (!mUserInputOrChromCaller && !d->mIsApprovedForFullscreen)) {
> +    Handled();
> +    DispatchPointerLockError(d);
> +    return NS_OK;                                                         

Nit: Trailing whitespace.

::: layout/base/nsPresShell.cpp
@@ +6684,5 @@
> +            }
> +          }
> +          nsCOMPtr<nsIDocument> pointerLockedDoc =
> +            do_QueryReferent(nsEventStateManager::sPointerLockedDoc);
> +          if (pointerLockedDoc) {

You're preventing the dispatch of the keydown to content but not the keypress and keyup, since nsIDocument::UnlockPointer() will null out nsEventStateManager::sPointerLockedDoc. You probably want to only call UnlockPointer in the key up case, like we do for fullscreen above, so that you block all three events for the ESC press.
Comment 57 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 01:29:07 PDT
(In reply to Chris Pearce (:cpearce) from comment #54)
> * Upon entering a pre-approved pointer lock, I think you should display the
> warning message for 1 maybe 2 seconds longer. It disappears too quickly to
> be read.
Ok.  right now it is just 1,5s, so perhaps 3s.


> * The mouse cursor is still visible while the warning message is showing.
On purpose.
 I
> think in the pre-approved case you should lock the pointer before showing
> the warning, so that users don't have to wait for the warning to hide to
> interact with their game or whatever. 
No need to wait. You can also just hide the doorhanger by clicking on the page

And once you do that, there's no need
> to have a close button on the warning, since the mouse can't be moved to hit
> it anyway.
Hmm, I don't know how to hide the close button.



> * The "Page Info" permissions grants still seem buggy.
Hmm, I was testing all the cases you mentioned earlier and they were working fine.

 Setting the page's
> "lock the pointer" permission to "block" in "Page Info" seems to render the
> page unblockable, i.e. once you set the permission to block, setting the
> permission to something other than blocked doesn't un-block the pointer lock
> on that page until you restart the browser.
I certainly can't reproduce this.
Reload is needed after unblocking.
Comment 58 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 01:34:22 PDT
(In reply to Chris Pearce (:cpearce) from comment #55)
> If a non-pre-approved pointer lock request is made, and then a fullscreen
> request is made, a pointerlockerror event is dispatched. Is that intentional?
Can't reproduce
http://mozilla.pettay.fi/moztests/pointerlock/pointerlock2_fullscreen.html 
lock+fullscreen case dispatches pointerlockchange as expected.
Comment 59 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 01:58:54 PDT
(In reply to Chris Pearce (:cpearce) from comment #56)
> You're preventing the dispatch of the keydown to content but not the
> keypress and keyup, since nsIDocument::UnlockPointer() will null out
> nsEventStateManager::sPointerLockedDoc. You probably want to only call
> UnlockPointer in the key up case, like we do for fullscreen above, so that
> you block all three events for the ESC press.
Nice catch.
Comment 60 Chris Pearce (:cpearce) 2013-03-21 02:42:19 PDT
> > * The "Page Info" permissions grants still seem buggy.
> Hmm, I was testing all the cases you mentioned earlier and they were working
> fine.
> 
>  Setting the page's
> > "lock the pointer" permission to "block" in "Page Info" seems to render the
> > page unblockable, i.e. once you set the permission to block, setting the
> > permission to something other than blocked doesn't un-block the pointer lock
> > on that page until you restart the browser.
> I certainly can't reproduce this.

Hmm... I played around for quite a while and kept hitting problems...

> Reload is needed after unblocking.

Ideally a reload would not be required after unblocking.

(In reply to Olli Pettay [:smaug] from comment #58)
> (In reply to Chris Pearce (:cpearce) from comment #55)
> > If a non-pre-approved pointer lock request is made, and then a fullscreen
> > request is made, a pointerlockerror event is dispatched. Is that intentional?
> Can't reproduce
> http://mozilla.pettay.fi/moztests/pointerlock/pointerlock2_fullscreen.html 
> lock+fullscreen case dispatches pointerlockchange as expected.

Repro's for me on http://pearce.org.nz/f press 'p' then 'f'. I'm using toji game shim on that page, it could be an interaction with that.
Comment 61 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 02:44:40 PDT
(In reply to Chris Pearce (:cpearce) from comment #60)
 
> > Reload is needed after unblocking.
> 
> Ideally a reload would not be required after unblocking.
Now I can't reproduce even the "needs reload" :/


> Repro's for me on http://pearce.org.nz/f press 'p' then 'f'. I'm using toji
> game shim on that page, it could be an interaction with that.
Comment 62 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 02:51:33 PDT
(In reply to Olli Pettay [:smaug] from comment #61)
> 
> > Repro's for me on http://pearce.org.nz/f press 'p' then 'f'. I'm using toji
> > game shim on that page, it could be an interaction with that.

Oh, that way. You first request pointer lock and then fullscreen.
Because entering to fullscreen changes focus, it is expected to cancel pointer lock per the spec.
And also, you really do want to be able to click the "Allow / Deny".

I guess we could temporarily exit pointer lock, and re-request so that after "Allow / Deny" it is
re-set.
Comment 63 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 03:05:13 PDT
I still don't see any error event though.

And the behavior or exiting pointer lock when going to fullscreen is the same as in Chrome.
Comment 64 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 03:07:52 PDT
(In reply to Chris Pearce (:cpearce) from comment #60)
> >  Setting the page's
> > > "lock the pointer" permission to "block" in "Page Info" seems to render the
> > > page unblockable, i.e. once you set the permission to block, setting the
> > > permission to something other than blocked doesn't un-block the pointer lock
> > > on that page until you restart the browser.
> > I certainly can't reproduce this.
> 
> Hmm... I played around for quite a while and kept hitting problems...
Any STR? I can't reproduce this problem now at all.
Comment 65 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 03:13:29 PDT
Created attachment 727599 [details] [diff] [review]
v9

Just those minor nits from the previous patch fixed.
Doug, could you review browser/* part?
Comment 66 Doug Turner (:dougt) 2013-03-21 08:53:55 PDT
Comment on attachment 727599 [details] [diff] [review]
v9

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

skimmed.  gavin should formally review.

::: browser/components/nsBrowserGlue.js
@@ +1784,5 @@
> +    if (request.type == "pointerLock") {
> +      this._promptPointerLock(request);
> +      return;
> +    }
> + 

white space.

@@ +1830,5 @@
> +    var requestingURI = requestingPrincipal.URI;
> +
> +    // Ignore requests from non-nsIStandardURLs
> +    if (!(requestingURI instanceof Ci.nsIStandardURL))
> +      return;

if () {
  stmt;
}

@@ +1836,5 @@
> +    var result = Services.perms.testExactPermissionFromPrincipal(requestingPrincipal, "pointerLock");
> +
> +    var autoAllow = false;
> +    if (result == Ci.nsIPermissionManager.ALLOW_ACTION) {
> +      autoAllow = true;

add a comment as to why you don't just call request.allow() and return here.

@@ +1853,5 @@
> +        label: browserBundle.GetStringFromName("pointerLock.lock"),
> +        accessKey: browserBundle.GetStringFromName("pointerLock.accesskey"),
> +        callback: function() {
> +          // If we allowed the lock automatically, nothing to do here.
> +          if (!autoAllow) {

why do you need this test?  the only way mainAction can be defined here is if autoAllow == false, right?  Or can this change... somehow out of band?

@@ +1860,5 @@
> +        }
> +      };
> +    }
> +
> +    var message = browserBundle.formatStringFromName(autoAllow ? "pointerLock.autoLockIitle" : "pointerLock.title",

autoLockIitle should probably be autoLockTitle

@@ +1871,5 @@
> +
> +    // Don't offer "always/never" in PB mode
> +    if (!autoAllow && !PrivateBrowsingUtils.isWindowPrivate(chromeWin)) {
> +      secondaryActions.push({
> +        label: browserBundle.GetStringFromName("pointerLock.alwaysAllowPointerLock"),

Maybe you should pull these strings out above and assign to local variables so that this is a bit cleaner.

@@ +1903,5 @@
> +                                                             function (type) {
> +                                                               if (type == "shown" && autoAllow) {
> +                                                                 chromeWin.setTimeout(function() {
> +                                                                   chromeWin.PopupNotifications.remove(notification);
> +                                                                 }, 2500);

2500?
Comment 67 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 08:58:57 PDT
(In reply to Doug Turner (:dougt) from comment #66)
> 
> if () {
>   stmt;
> }
> 
I'm using the coding style of the file.
(and I'm just now in a meeting about reviews and complained about patch authors not using
consistent coding style :) )

> @@ +1903,5 @@
> > +                                                             function (type) {
> > +                                                               if (type == "shown" && autoAllow) {
> > +                                                                 chromeWin.setTimeout(function() {
> > +                                                                   chromeWin.PopupNotifications.remove(notification);
> > +                                                                 }, 2500);
> 
> 2500?
Should I use some pref? or just comment that the time is something to hide the warning.
Comment 68 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 09:00:02 PDT
Also, native English speakers, please check whether the en-US/* localization makes sense.
Comment 69 Justin Dolske [:Dolske] 2013-03-21 09:40:49 PDT
Comment on attachment 727599 [details] [diff] [review]
v9

Gavin's at a workweek in Paris, stealing this. Will do in the next few hours.
Comment 70 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-21 09:43:03 PDT
I don't think "lock the pointer" and "pointer lock" are user-understandable strings, so we'll probably need to change that. Does Chrome have a similar notification? Are there strings from other contexts that might be better? Otherwise, seems like something related to "capturing the mouse" would be better.

We never label things "Yes" in the UI, generally - the label should be more descriptive (like "Allow X").

You shouldn't need two different types of notification, you could just re-use the same type for both actions. That would simplify the code somewhat.
Comment 71 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-21 09:49:45 PDT
Created attachment 727739 [details] [diff] [review]
v9 with some tweaks

This (untested) tweaked version addresses some comments I had (but doesn't address the terminology issue).
Comment 72 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-21 09:51:08 PDT
Created attachment 727742 [details] [diff] [review]
v9 with some tweaks
Comment 73 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-21 09:54:17 PDT
Comment on attachment 727742 [details] [diff] [review]
v9 with some tweaks

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+    let notification = chromeWin.PopupNotifications.show(browser, "pointerLock",
>+                                                         message, null, mainAction, secondaryActions,
>+                                                         {
>+                                                           removeOnDismissal: true,
>+                                                           eventCallback:
>+                                                             function (type) {
>+                                                               if (type == "removed") {
>+                                                                 if (autoAllow) {
>+                                                                   request.allow();
>+                                                                 } else {
>+                                                                   request.cancel();
>+                                                                 }

This seems like strange behavior. In the "autoAllow" case, we only allow the request once the popup notification has been dismissed?

It also seems weird to "time out" the notification (as in the original v9 patch, I removed it accidentally in this version).
Comment 74 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 10:45:16 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #73)
> 
> This seems like strange behavior. In the "autoAllow" case, we only allow the
> request once the popup notification has been dismissed?
Based on the discussion with Gavin this should be ok, since this way user just clicks once anywhere and the page gets
the pointer lock


> 
> It also seems weird to "time out" the notification (as in the original v9
> patch, I removed it accidentally in this version).
I disagree with this a bit, but don't care too much. Let's do the first version without timeout.
Comment 75 Chris Pearce (:cpearce) 2013-03-21 13:52:46 PDT
Comment on attachment 727742 [details] [diff] [review]
v9 with some tweaks

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

r=cpearce on the content/* dom/* parts.
Comment 76 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 14:31:48 PDT
Comment on attachment 727742 [details] [diff] [review]
v9 with some tweaks

Looks like gavin removed the other popupnotification in this patch, but
I was using to try to prevent the js exceptions cpearce was seeing.

Perhaps I could explicitly close pointerLock-notification before using it
for the warning.


For the en-US/* part I should replace pointer lock with mouse cursor or mouse cursor lock.
Comment 77 Justin Dolske [:Dolske] 2013-03-21 15:15:43 PDT
Comment on attachment 727742 [details] [diff] [review]
v9 with some tweaks

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

[Let me look at smaug's updated patch next.]

::: browser/base/content/pageinfo/permissions.js
@@ +135,5 @@
>    var command  = document.getElementById("cmd_" + aPartId + "Toggle");
> +  // Geolocation and PointerLock permission consumers use testExactPermission, not testPermission.
> +  var perm = (aPartId == "geo" || aPartId == "pointerLock") ?
> +    permissionManager.testExactPermission(gPermURI, aPartId) :
> +    permissionManager.testPermission(gPermURI, aPartId);

Nit: we're piling enough in here that this should just be a simple and readable if/else.

  var perm;
  if (aPartId == "geo" || aPartId == "pointerLock")
    perm = permissionManager.testExactPermission(gPermURI, aPartId);
  else
    perm = permissionManager.testPermission(gPermURI, aPartId);

::: browser/components/nsBrowserGlue.js
@@ +1783,5 @@
> +    // pointerLock has different kind of permission and prompt handling.
> +    if (request.type == "pointerLock") {
> +      this._promptPointerLock(request);
> +      return;
> +    }

This ends up duplicating a _lot_ of the existing machinery and checks. Seems like it would be better to add some special-casing for pointerLock and follow the existing path?

(Let me try that and see if it's sensible.)

@@ +1844,5 @@
> +    let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +
> +    let mainAction = null;
> +    if (!autoAllow) {
> +      mainAction = {

I'm a little dubious about using the doorhanger as the warning/notice UI. These prompts are intentionally designed to be easily dismissible, and so if the user clicks again it's going to immediately go away.

Maybe that's ok for now. And it's only relevant after the user has granted an ALWAYS permission for the site, so it's a little less critical to get this right.

@@ +1895,5 @@
> +                                                                   request.cancel();
> +                                                                 }
> +                                                               }
> +                                                             }
> +                                                         });

.                        Use |let options = { ... }|
                         so this isn't squished so
                         far to the right.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +278,5 @@
> +
> +pointerLock.lock=Allow pointer locking
> +pointerLock.accesskey=A
> +pointerLock.alwaysAllowPointerLock=Always allow pointer locking
> +pointerLock.alwaysAllowPointerLock.accesskey=A

This and pointerLock.accesskey should differ, I think.
Comment 78 Justin Dolske [:Dolske] 2013-03-21 15:18:08 PDT
(In reply to Justin Dolske [:Dolske] from comment #77)

> [Let me look at smaug's updated patch next.]

Err, I'm confused. Let me try the de-duplication I mentioned, back in a bit.
Comment 79 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-21 15:24:53 PDT
(In reply to Justin Dolske [:Dolske] from comment #77)
> This ends up duplicating a _lot_ of the existing machinery and checks. Seems
> like it would be better to add some special-casing for pointerLock and
> follow the existing path?
The code is actually reasonable different. Pointlock case needs to handle 
the permission case and warning case, geolocation and WebNotifications are simpler


> I'm a little dubious about using the doorhanger as the warning/notice UI.
> These prompts are intentionally designed to be easily dismissible, and so if
> the user clicks again it's going to immediately go away.
Well that is the whole point. Having a warning which user can get rid of easily

> 
> Maybe that's ok for now. And it's only relevant after the user has granted
> an ALWAYS permission for the site, so it's a little less critical to get
> this right.
Also, I'm not aware of any other ways to show warnings.




> > +
> > +pointerLock.lock=Allow pointer locking
> > +pointerLock.accesskey=A
> > +pointerLock.alwaysAllowPointerLock=Always allow pointer locking
> > +pointerLock.alwaysAllowPointerLock.accesskey=A
> 
> This and pointerLock.accesskey should differ, I think.
Yes
Comment 80 Chris Pearce (:cpearce) 2013-03-21 15:50:40 PDT
(In reply to Olli Pettay [:smaug] from comment #64)
> (In reply to Chris Pearce (:cpearce) from comment #60)
> > >  Setting the page's
> > > > "lock the pointer" permission to "block" in "Page Info" seems to render the
> > > > page unblockable, i.e. once you set the permission to block, setting the
> > > > permission to something other than blocked doesn't un-block the pointer lock
> > > > on that page until you restart the browser.
> > > I certainly can't reproduce this.
> > 
> > Hmm... I played around for quite a while and kept hitting problems...
> Any STR? I can't reproduce this problem now at all.

Ah, I think what was happening was that I hit the cancelation limit, so I was blocked irrespective of the permission grant.
Comment 81 Justin Dolske [:Dolske] 2013-03-21 17:27:10 PDT
(In reply to Olli Pettay [:smaug] from comment #74)

> > It also seems weird to "time out" the notification (as in the original v9
> > patch, I removed it accidentally in this version).
> I disagree with this a bit, but don't care too much. Let's do the first
> version without timeout.

As a general rule we avoid self-timeout UI... It's bad for accessibility (some users need more time to read and react), and can be confusing (user is unclear why it went away, or was about to interact with it).

It's probably OK as intended here (i.e., as an ambient "hey something's different" FYI), but because doorhangers are pretty distinctive UI I wouldn't want to recycle them for that purpose. We've talked about adding something like this for other purposes (mixed content blocking, most recently), but don't have anything implemented.

I think we should revisit this UI in a followup, because the current UI (and need for an extra click) is kind of disruptive for having granted an "always allow" permission. [But it does seem like some notice is needed, as getting into pointerlock unexpectedly could be extremely confusing. So I think the current patch is acceptable annoyance for now.]
Comment 82 Justin Dolske [:Dolske] 2013-03-21 17:32:02 PDT
(In reply to Chris Pearce (:cpearce) from comment #20)

> * In windowed mode, when a pointer lock request is made we drop down a
> permission dialog in a door hanger off the chrome (similar to the geoloation
> permission dialog) which asks permission before entering pointer lock. I
> think we should fail the pointerlock request if the user dismisses the door
> hanger.

Is there a particular reason for failing the request upon dismissal?

Other permission-doorhangers (like geolocation) don't do this, deliberately. It gives users a chance to explore the page and make a decision later, and also helps prevent nagging from the page -- an existing prompt (that's been dismissed to icon-only form) won't reopen if it's triggered again.

Unless there's a strong reason for changing the existing semantics, I'd prefer to keep this prompt consistent with others.
Comment 83 Justin Dolske [:Dolske] 2013-03-21 18:59:30 PDT
Created attachment 728004 [details] [diff] [review]
Patch v.10

Hot-potato patch! :)

* Fixed the nits I listed in comment 77 review
* Refactored _promptPointerLock() to be consisted with the nearby code and not
  duplicate all that goop.
* added the CSS+markup for icons.

Steven Horlander is still working on new icons, so for now I've just copied the images from toolkit/themes/*/information-##.png. When the final artwork is ready, it'll just be a simple file replacement.

TODOs:

* Address comment 82. I didn't change what patch v.9 was doing.
* Better strings, figured I'd checkpoint things so far first.
Comment 84 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 01:40:19 PDT
(In reply to Justin Dolske [:Dolske] from comment #82)
> (In reply to Chris Pearce (:cpearce) from comment #20)
> 
> > * In windowed mode, when a pointer lock request is made we drop down a
> > permission dialog in a door hanger off the chrome (similar to the geoloation
> > permission dialog) which asks permission before entering pointer lock. I
> > think we should fail the pointerlock request if the user dismisses the door
> > hanger.
> 
> Is there a particular reason for failing the request upon dismissal?
> 

What else would we do? And yes the patch does fail the request in case of dismissal
Comment 85 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 01:43:24 PDT
(In reply to Olli Pettay [:smaug] from comment #84)
> (In reply to Justin Dolske [:Dolske] from comment #82)
> > (In reply to Chris Pearce (:cpearce) from comment #20)
> > 
> > > * In windowed mode, when a pointer lock request is made we drop down a
> > > permission dialog in a door hanger off the chrome (similar to the geoloation
> > > permission dialog) which asks permission before entering pointer lock. I
> > > think we should fail the pointerlock request if the user dismisses the door
> > > hanger.
> > 
> > Is there a particular reason for failing the request upon dismissal?
> > 
> 
> What else would we do? And yes the patch does fail the request in case of
> dismissal
Well, I guess we could leave the doorhanger closed and user could reopen it...
Not sure that is good UI. Page can always re-ask lock if needed.
Comment 86 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 01:48:10 PDT
So, I'm planning to change the strings and get some native English speaker to review them and land the
patch today. Followup bugs for tweaking...
I hope that is ok.
Comment 87 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 02:36:44 PDT
Something regressed in that v10. Investigating.
Comment 88 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 02:48:28 PDT
Or it is a regression from gavin's v9 perhaps.
Since we're now reusing the same notification, you get kind of mix of both permission dialog and
warning dialog once you're added permanent permission.
Comment 89 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 04:00:18 PDT
Created attachment 728136 [details] [diff] [review]
v11

Changed en-US to talk about mouse cursor and fixed the warning prompt by
adding back the specific popupnotification

https://tbpl.mozilla.org/?tree=Try&rev=7b09d6302637

The changes need probably a review after all.
Comment 90 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 04:03:13 PDT
Created attachment 728137 [details] [diff] [review]
v10 -> v11 interdiff

Looks like bugzilla's diff can't create proper interdiff.
Comment 91 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-22 05:22:57 PDT
Opt builds https://tbpl.mozilla.org/?tree=Try&rev=6b601b28c1b2
so that I can test this on Windows.
Comment 92 Justin Dolske [:Dolske] 2013-03-22 16:12:31 PDT
(In reply to Olli Pettay [:smaug] from comment #85)

> > > Is there a particular reason for failing the request upon dismissal?
> > 
> > What else would we do? And yes the patch does fail the request in case of
> > dismissal
> Well, I guess we could leave the doorhanger closed and user could reopen
> it...
> Not sure that is good UI. Page can always re-ask lock if needed.

As I noted in comment 82, other similar permissions prompts all do this.


(In reply to Olli Pettay [:smaug] from comment #88)
> Or it is a regression from gavin's v9 perhaps.
> Since we're now reusing the same notification, you get kind of mix of both
> permission dialog and
> warning dialog once you're added permanent permission.

Bug 854035, simple fix.
Comment 93 Justin Dolske [:Dolske] 2013-03-22 16:13:09 PDT
Comment on attachment 728137 [details] [diff] [review]
v10 -> v11 interdiff

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

I'll update with this, previous comment, and new icons from bug 853171.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +284,3 @@
>  pointerLock.neverAllow.accesskey=N
>  pointerLock.title=Would you like to allow the mouse cursor to be disabled on %S?
>  pointerLock.autoLock.title=%S will disable the mouse cursor.

The main description is better (let's go with that and tweak later), but the buttons labels were better before (see comment 70).
Comment 94 Justin Dolske [:Dolske] 2013-03-22 17:09:09 PDT
Created attachment 728524 [details] [diff] [review]
Patch v.12

* Updated icons with artwork from bug 853171
* Tweaked strings per last comments
* Dismissal no longer denys per last comments
* Fixed the misindented "press ESC" <label>. (XUL is dumb)
* Since bug 854035 is simple and will remove the need for separate <popupnotificationcontent>s, I've kept the single item. I'll get that bug landed ASAP.
Comment 95 Justin Dolske [:Dolske] 2013-03-22 17:10:16 PDT
*** Bug 853171 has been marked as a duplicate of this bug. ***
Comment 96 Justin Dolske [:Dolske] 2013-03-22 17:11:48 PDT
Created attachment 728525 [details]
Screenshot

shorlander ui-r+'d this on IRC.
Comment 97 Justin Dolske [:Dolske] 2013-03-22 23:23:37 PDT
(To be clear: I think this is ready to land if you're ok with it, though from comment 91 sounds like you're still testing?)
Comment 98 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-23 17:09:41 PDT
"Lock the pointer" sounds wrong, since in other places we talk about mouse cursor.
Comment 99 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-23 17:38:48 PDT
And lock (don't accept) -> fullscreen -> esc is broken with the latest patch.
We end up showing the cursor icon in the locationbar, and it is possible to open it, but
it doesn't actually do anything anymore (since the pointer lock was already used in fullscreen mode).
Comment 100 Justin Dolske [:Dolske] 2013-03-23 18:01:06 PDT
(In reply to Olli Pettay [:smaug] from comment #98)
> "Lock the pointer" sounds wrong, since in other places we talk about mouse
> cursor.

Eh? Oh, I forgot to update pageInfo.dtd. Yes, it should be "Hide the mouse cursor."


(In reply to Olli Pettay [:smaug] from comment #99)
> And lock (don't accept) -> fullscreen -> esc is broken with the latest patch.
> We end up showing the cursor icon in the locationbar, and it is possible to
> open it, but
> it doesn't actually do anything anymore (since the pointer lock was already
> used in fullscreen mode).

Hmm. The front-end should probably have a listener for that, and remove any pending pointerlock prompt when that happens.
Comment 101 Justin Dolske [:Dolske] 2013-03-23 19:29:11 PDT
Created attachment 728683 [details] [diff] [review]
Patch v.13

C'mon lucky version 13. :)
Comment 102 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-24 02:45:14 PDT
Testing...
Comment 103 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-24 03:28:50 PDT
Comment on attachment 728683 [details] [diff] [review]
Patch v.13

I'll land this real soon.
Comment 104 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-24 05:34:01 PDT
Hmm, actually, the event listener is added in such way that scripts running in content can
affect to the behavior.
Comment 105 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-24 06:02:02 PDT
Created attachment 728720 [details] [diff] [review]
patch
Comment 107 Chris Pearce (:cpearce) 2013-03-24 13:53:33 PDT
Comment on attachment 728525 [details]
Screenshot

"Would you like to allow the mouse cursor to be hidden on $domain" is very verbose and long-winded. 

How about:

"Allow $domain to hide the mouse cursor?"
Comment 108 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2013-03-24 14:19:23 PDT
Indeed, that sounds better.
Comment 110 Justin Dolske [:Dolske] 2013-03-25 12:38:06 PDT
(In reply to Chris Pearce (:cpearce) from comment #107)

> "Would you like to allow the mouse cursor to be hidden on $domain" is very
> verbose and long-winded. 
> 
> How about:
> 
> "Allow $domain to hide the mouse cursor?"

Fodder for a followup bug, though I'll note this is consistent with similar strings:

$  grep -i would browser/locales/en-US/chrome/browser/browser.properties
activatePluginsMessage.message=Would you like to activate the plugins on this page?
geolocation.shareWithSite=Would you like to share your location with %S?
geolocation.shareWithFile=Would you like to share your location with the file %S?
webNotifications.showFromSite=Would you like to show notifications from %S?
pointerLock.title=Would you like to allow the mouse cursor to be hidden on %S?
keywordPrompt.message = %1$S is using '%2$S' for searches from the location bar. Would you like to restore the default search (%3$S)?
getUserMedia.shareCamera.message = Would you like to share your camera with %S?
getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S?
getUserMedia.shareCameraAndMicrophone.message = Would you like to share your camera and microphone with %S?
Comment 111 Virgil Dicu [:virgil] [QA] 2013-04-04 08:11:23 PDT
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130404 Firefox/22.0

Verified in Aurora on Mac OS 10.7.5, Windows 7 and Ubuntu 12.04.
Used http://media.tojicode.com/q3bsp/ and http://pearce.org.nz/fullscreen/pointerlock-position.html

Doorhanger is displayed as expected and one can select any option with expected results.

Filed bug 858082 for a slight misalignment in the notifications on Linux and Windows.
Comment 112 Alex Keybl [:akeybl] 2013-04-05 11:59:44 PDT
This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

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