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.

We probably need some kind of UI to request permission, similar to geolocation's UI.

It looks like this might have landed in Chrome already - https://twitter.com/Tojiro/status/207167169291751424

Confirmed: https://twitter.com/Vincent_Scheib/status/207171218414125056

We should check what their permission model is and whether it is strong enough.

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.

Seems like this is the sort of thing we'd like for Kilimanjaro.

Not blocking b2g right now.

Need to check with bus dev to see if our partners require this and reopen if so.

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

How does our implementation not conform to the specification? Please be more specific.

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.

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.

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.

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.

Assign sec review to Curtis to set up full security team review.

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.

Filled bug 822654 to get this scheduled

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.

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

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?

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

(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

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.

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.

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

I'll try to look at this soon.

Looks like we don't have a sane way to add simple doorhanger for this. So need to do something insane.

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 on attachment 725811 [details] [diff] [review] patch, v1 Bah, try doesn't like this. new patch coming.

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

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

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

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

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

(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 :)

(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

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

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 on attachment 727176 [details] [diff] [review] v7 Thanks to humph I noticed a bug. Better patch coming.

Just one line change to v7. https://tbpl.mozilla.org/?tree=Try&rev=7800f2d169ea

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.

Separate bug on that please if it shows up :)

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

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

Hmm... a clobber fixed my build error... must have had stable objdir somehow...

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

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

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

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

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

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

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

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

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.

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

Just those minor nits from the previous patch fixed. Doug, could you review browser/* part?

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?

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

Also, native English speakers, please check whether the en-US/* localization makes sense.

Comment on attachment 727599 [details] [diff] [review] v9 Gavin's at a workweek in Paris, stealing this. Will do in the next few hours.

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.

This (untested) tweaked version addresses some comments I had (but doesn't address the terminology issue).

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

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

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

(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

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

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

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

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.

(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

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

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.

Something regressed in that v10. Investigating.

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.

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.

Looks like bugzilla's diff can't create proper interdiff.

Opt builds https://tbpl.mozilla.org/?tree=Try&rev=6b601b28c1b2 so that I can test this on Windows.

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

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

shorlander ui-r+'d this on IRC.

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

"Lock the pointer" sounds wrong, since in other places we talk about mouse cursor.

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

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

C'mon lucky version 13. :)

Testing...

Comment on attachment 728683 [details] [diff] [review] Patch v.13 I'll land this real soon.

Hmm, actually, the event listener is added in such way that scripts running in content can affect to the behavior.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c656d41192f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f1fd136c04

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

Indeed, that sounds better.

https://hg.mozilla.org/mozilla-central/rev/c656d41192f6 https://hg.mozilla.org/mozilla-central/rev/c8f1fd136c04 https://hg.mozilla.org/mozilla-central/rev/c5c2aee6ebe6

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

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.

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/