Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 8 years ago

We've removed the permission requirement for Fullscreen API since several versions before, and we do not require additional permission for PointerLock when the document is in fullscreen.

I think we could probably also remove the permission requirement for PointerLock API.

Our current behavior of permission requirement of pointer lock when not in fullscreen isn't very sensible. Even if the user has granted the permission to a site forever, they would still need an additional click to actually have the pointer locked.

Chrome has removed the permission requirement for PointerLock API like for Fullscreen API.

As far as we can have clear warning on the UI to notice user about the way to exit pointer lock, I don't see there would be any security issue. We probably want an improved UI for that, though. I guess perferably the notice could persist on the UI somehow until the pointer is unlocked.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 1 • 8 years ago

Still need approval from security people. ni? dveditz for this.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 8 years ago

It seems to me doing this first would make bug 1255338 much easier, because removing the permission requirement for the PointerLock API means we can decouple it with the Fullscreen API. Currently, a bunch of code in nsDocument.cpp for pointer lock is for handling the interaction between those two API, which makes the whole thing a bit complicated, and harder to split into two processes.

So I'd hope we can do this first so that those code would get removed / simplified before I start refactoring them in a more efficient way on e10s.

	Daniel Veditz [:dveditz]
	Comment 3 • 8 years ago

Are you proposing an "announcement bar" that goes away after a second or two like our fullscreen one? ("foo has disabled your mouse cursor  Press ESC to exit"). That would be OK for the most part.

My one concern is a site that can lock the user in with no way to get out: an ESC/pointerlock battle that the user can't win. If the page also captures keystrokes (the way Google docs do, for example) there may be no obvious way to close the tab or switch to another without having to kill the browser. To combat a similar kind of annoyingness--while(1)alert('ha ha');--we add a 'no more prompts from this page' checkbox after the first few. Not sure what the equivalent here would be.
 * could keep a counter and revert to the doorhanger after a few (ugly, more code)
 * ignore pointerlock for some timeout period after leaving pointerlock. Could be a fixed time
   long enough to let the user close the tab (2 seconds?) or maybe a small timeout that gets
   longer (doubles? add 0.5 seconds?) every time the user hits ESC.

In both scenarios you may want to reset the counter/timeout if the page loses focus (and pointerlock) without the user hitting ESC, although be careful with how you do that because we don't want a malicious page to be able to drop and re-gain pointerlock to re-set and keep the user locked out. A fixed 2s timeout avoids that, but maybe that punishes good sites.

I assume that we'd still require the window/frame requesting pointerlock to be the active tab? Not sure how you've coded that but if ensuring the page was active happened "for free" by using a door-hanger then you might need to add some more active checks.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 8 years ago

Two points:
1. the ESC to exit pointerlock cannot be caught by the content (like what we do for fullscreen)
2. pointerlock still requires a user input (keypress, click, etc) so content should not be able to start locking without user's explicit action
Given these, user can always press ESC and close tab with their pointer, so there shouldn't be a battle. Does this solve your concern?

And also I guess we can display the announcement bar forever when the pointer is locked if that makes more sense.

	Daniel Veditz [:dveditz]
	Comment 5 • 8 years ago

> 1. the ESC to exit pointerlock cannot be caught by the content

Right, but other keystrokes could (to switch or close the tab, for example) and my worry was that the page could re-pointerlock faster than the user could move his mouse

> 2. pointerlock still requires a user input (keypress, click, etc) so content
> should not be able to start locking without user's explicit action

> Does this solve your concern?

As long as the ESC-to-leave doesn't count as a keypress that enables pointerlock then yes--sounds good!

> And also I guess we can display the announcement bar forever when the
> pointer is locked if that makes more sense.

People will absolutely hate that and it wouldn't do much good. We announce entering pointerlock (except in the full-screen case, which is OK because that's announced, too) and the page loses pointerlock if it loses focus--those combined should prevent spoofing the user so you can't be spoofed by a fake cursor after switching back from somewhere else. Those should handle "spoofing" abuse. The notice won't address the malicious harassment case -- the point of those is that the user is well aware that they're being trapped. See the various mean things ("eviltraps") we've found as dependencies bug 432687.

	Dale Harvey (:daleharvey) Assignee
	Comment 6 • 8 years ago

Hi Verdi

Looking to pick this up and Mike mentioned asking you about it after you working on the fullscreen bug, the main thing it seems like this needs is UX, I dont know of us using attention bars in non fullscreen contexts so that may need some thought, similiarly we likely want to handle the user going into fullscreen + pointerlock at the same time nicely.

If you have any thoughts / pointers for ux or other things to think about that would be great, thanks a lot
Dale

	Dale Harvey (:daleharvey) Assignee
	Comment 7 • 8 years ago

So we already allow pointerLock in fullscreen without additional permission prompting, this only really needs to handle the case of a non fullscreen pointerlock triggering an attention prompt. I am fairly close to a patch which does one similiar to the fullscreen attention prompt

	Dale Harvey (:daleharvey) Assignee
	Comment 8 • 8 years ago

So this is how this looks in my current version, it doesnt make sense to have the same button as the fullscreen prompt (as the page is controlling the cursor). however some prompt should be there to inform the user about pressing "esc". Some UX guidance would be great here, thanks

	Verdi [:verdi] Best to slack me
	Comment 9 • 8 years ago

(In reply to Dale Harvey (:daleharvey) from comment #8)
> Created attachment 8762020 [details]
> Screen Shot 2016-06-10 at 13.10.24.png
> 
> So this is how this looks in my current version, it doesnt make sense to
> have the same button as the fullscreen prompt (as the page is controlling
> the cursor). however some prompt should be there to inform the user about
> pressing "esc". Some UX guidance would be great here, thanks

This will be a nice addition. Thanks for working on it.

I think, like in your screenshot, we can use the same dialog as fullscreen. I suggest something like:
* Show the connection status like we do with fullscreen
* Text: website.com controls the pointer. Press Esc to show it again.

(In reply to Dale Harvey (:daleharvey) from comment #6)
> similiarly we likely want to handle the user
> going into fullscreen + pointerlock at the same time nicely.

* Show the connection status like we do with fullscreen
* Text: website.com is full screen and controls the pointer. Press Esc to return.

	Dale Harvey (:daleharvey) Assignee
	Comment 10 • 8 years ago

So the main thing I didnt do in this patch was the preferences, right now we use the full screen preference names, I dont think we particularly need to have seperate preferences for each warning, I am going to guess we should rename the current ones to 'warning-box' ones, however I wasnt sure what the prefered method for that is.

I expect nsDocument.cpp will need some changes, what is there works, however not sure if sending the event inside DispatchPointerLockChange would be more robust.

Working on the tests now

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 11 • 8 years ago

I believe we want separate prefs for warnings of fullscreen and pointerlock.

nsDocument would need some changes, which may be nontrivial. If you are not familiar with that or you are not confident about doing so, I can take that work. In that case, please file a new bug and assign me there, and you just need to make request always end up being approved.

If you'd also like to work on the DOM part, I'd like to provide some feedback.

(Actually I didn't expect other people take this. I was just waiting for the design... But if you feel good working on that, I'm fine with that as well.)

	Dale Harvey (:daleharvey) Assignee
	Comment 12 • 8 years ago

Xidorn the patch above has it all working including changes to nsDocument.cpp, I meant that my changes to nsDocument.cpp probably need some improving, if you could give feedback on the patch that would be great :)

	Dale Harvey (:daleharvey) Assignee
	Comment 13 • 8 years ago

And to explain the patch a little, I extracted the code that shows the warning from fullscreen into its own object we can create instances of but the rest of the code is identical, we then use the same code to show the warning for pointerlock

	Dale Harvey (:daleharvey) Assignee
	Comment 14 • 8 years ago

And one issue with this is the frontend still needs to detect when pointerlock + fullscreen is enabled and handle appropriately

	Mike Conley (:mconley) (:⚙️)
	Comment 15 • 8 years ago

Comment on attachment 8763329 [details] [diff] [review]
Remove pointerlock permission requirement

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

From a high level, these look like the right moves to me. dao reviewed the fullscreen implementation of this, so he might be a good choice to review this as well. If dao's busy, you can wait for me - except I'm about to be on PTO, so dao might be able to recommend alternatives.

::: browser/base/content/tab-content.js
@@ +40,5 @@
>  // TabChildGlobal
>  var global = this;
>  
>  
> +addEventListener("MozDOMPointerLock:Entered", function(aEvent) {

Typically, from what I've seen, we don't normally have :'s in our event names. We use them in message names, but event names not so much. If there are no other event names with :'s, we might want to stick with convention and call this MozDOMPointerLockEntered.

@@ +46,5 @@
> +    originNoSuffix: aEvent.target.nodePrincipal.originNoSuffix
> +  });
> +});
> +
> +addEventListener("MozDOMPointerLock:Exited", function(aEvent) {

Same as above.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +129,5 @@
>  <!ENTITY fullscreenWarning.afterDomain.label "is now full screen">
>  <!ENTITY fullscreenWarning.generic.label "This document is now full screen">
> +
> +<!ENTITY pointerlockWarning.beforeDomain.label "">
> +<!ENTITY pointerlockWarning.afterDomain.label "controls the pointer. Press Esc to show it again.">

We might want matej to thumbs up the language here.

::: dom/base/nsDocument.cpp
@@ +12357,5 @@
>        DispatchPointerLockError(d);
>        return NS_OK;
>      }
>  
> +    Allow(JS::UndefinedHandleValue);

We'll likely want to file some follow-up bugs to make this runnable not need to implement nsIContentPermissionRequest, etc.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 16 • 8 years ago

(In reply to Mike Conley (:mconley) - (Away until June 29th) from comment #15)
> Comment on attachment 8763329 [details] [diff] [review]
> Remove pointerlock permission requirement
> 
> Review of attachment 8763329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From a high level, these look like the right moves to me. dao reviewed the
> fullscreen implementation of this, so he might be a good choice to review
> this as well. If dao's busy, you can wait for me - except I'm about to be on
> PTO, so dao might be able to recommend alternatives.
> 
> ::: browser/base/content/tab-content.js
> @@ +40,5 @@
> >  // TabChildGlobal
> >  var global = this;
> >  
> >  
> > +addEventListener("MozDOMPointerLock:Entered", function(aEvent) {
> 
> Typically, from what I've seen, we don't normally have :'s in our event
> names. We use them in message names, but event names not so much. If there
> are no other event names with :'s, we might want to stick with convention
> and call this MozDOMPointerLockEntered.

We use that form of event name for internal fullscreen events, and smaug thought this kind of namespace makes code easier to read. See bug 1053413 comment 42.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 17 • 8 years ago

(In reply to Mike Conley (:mconley) - (Away until June 29th) from comment #15)
> Comment on attachment 8763329 [details] [diff] [review]
> Remove pointerlock permission requirement
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +129,5 @@
> >  <!ENTITY fullscreenWarning.afterDomain.label "is now full screen">
> >  <!ENTITY fullscreenWarning.generic.label "This document is now full screen">
> > +
> > +<!ENTITY pointerlockWarning.beforeDomain.label "">
> > +<!ENTITY pointerlockWarning.afterDomain.label "controls the pointer. Press Esc to show it again.">
> 
> We might want matej to thumbs up the language here.

Wait, what?! Press Esc should exit pointerlock directly, not show the message again. Esc is the only reliable way for user to get the browser out from the website's control of the pointer.

	Mike Conley (:mconley) (:⚙️)
	Comment 18 • 8 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+1) from comment #17)
> (In reply to Mike Conley (:mconley) - (Away until June 29th) from comment
> #15)
> > Comment on attachment 8763329 [details] [diff] [review]
> > Remove pointerlock permission requirement
> > 
> > ::: browser/locales/en-US/chrome/browser/browser.dtd
> > @@ +129,5 @@
> > >  <!ENTITY fullscreenWarning.afterDomain.label "is now full screen">
> > >  <!ENTITY fullscreenWarning.generic.label "This document is now full screen">
> > > +
> > > +<!ENTITY pointerlockWarning.beforeDomain.label "">
> > > +<!ENTITY pointerlockWarning.afterDomain.label "controls the pointer. Press Esc to show it again.">
> > 
> > We might want matej to thumbs up the language here.
> 
> Wait, what?! Press Esc should exit pointerlock directly, not show the
> message again. Esc is the only reliable way for user to get the browser out
> from the website's control of the pointer.

To be clear, that's what the message is saying:

http://i.imgur.com/vpT04Ef.png

In that message, it's referring to having Esc show the pointer again, not the warning message.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 19 • 8 years ago

Hmmm, okay. That still sounds confusing, because the message doesn't say the pointer is hidden, so "show it again" could probably be confusing... Probaby better to make it clear via e.g. "to show the pointer again" or "to stop its control".

	Mike Conley (:mconley) (:⚙️)
	Comment 20 • 8 years ago

I agree that we might want some wordsmithing here to ensure maximum clarity - matej is the right folk for the job.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 21 • 8 years ago

BTW, daleharvey, you probably also want to remove pointerlock from the permission UI. You can see my work in bug 1160017 as a reference.

	Dale Harvey (:daleharvey) Assignee
	Comment 22 • 8 years ago

Dao, Mike suggested you would be good to look at this patch, thanks in advance :)

Change from the last patch is that this removes the current doorhanger implementation, I also couldnt ignore the case where both pointerlock + fullscreen were requested at the same time since it was relatively common so I introduced 'browser-fullScreenOrPointerLock.js' which delays showing the warning box for 100ms so it can combine the warnings if needed.

	Dão Gottwald [:dao]
	Comment 23 • 8 years ago

Comment on attachment 8764526 [details] [diff] [review]
1273351-2.patch

Hmm, the separation between browser-warningBox.js, browser-fullScreenOrPointerLock.js, browser-fullScreen.js and browser-pointerLock.js seems somewhat excessive. Can we fold all that stuff into browser-fullScreenOrPointerLock.js? If for instance we need the warning box thingy for other purposes later, we can still split and move that code.

nit: use double quotes rather than single quotes

	Dão Gottwald [:dao]
	Comment 24 • 8 years ago

(In reply to Dão Gottwald [:dao] from comment #23)
> Can we fold all that stuff into browser-fullScreenOrPointerLock.js?

Actually I'd prefer browser-fullScreenAndPointerLock.js...

	Dale Harvey (:daleharvey) Assignee
	Comment 26 • 8 years ago

Ok updated, last thing is I cant find any test around fullScreen that specifically is testing the display + contents of the warning UI, Xidorn am I missing something?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 27 • 8 years ago

Could you use hg move rather than renaming and adding stuff to the new file directly? Removing a whole file and have its content in another file is hard to follow...

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 28 • 8 years ago

> <!ENTITY pointerlockWarning.afterDomain.label "controls the pointer. Press Esc to show it again.">
> <!ENTITY pointerlockWarning.generic.label "This document controls the pointer. Press Esc to show it again.">
> 
> <!ENTITY pointerlockFullscreenWarning.afterDomain.label "is full screen and controls the pointer. Press Esc to show it again.">
> <!ENTITY pointerlockFullscreenWarning.generic.label "This document is full screen and controls the pointer. Press Esc to show it again.">

I'd like to restate my concern on comment 17 and comment 19 that I think the message here is ambiguous, and should be made clearer.

Given comment 20, matej, could you provide some advice on the wording here?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 29 • 8 years ago

Another issue, I don't think we want to show anything when the pointer is locked on fullscreen. We do not require permission for pointerlock when in fullscreen now, so after entering fullscreen, using pointerlock is free for the page.

Given that adding another warning message for pointerlock in fullscreen is unlikely an improvement on security, I'd suggest doing so looks like a UX regression, and adds complexity to the code.

Could we just skip showing the warning box about pointerlock if we are already in fullscreen?

	Dão Gottwald [:dao]
	Comment 30 • 8 years ago

Please also rename WarningBox (and .warning-box) to something more distinctive. FullscreenAndPointerLockWarningBox is verbose, maybe you can come up with something shorter.

	Dale Harvey (:daleharvey) Assignee
	Comment 31 • 8 years ago

> Could you use hg move rather than renaming and adding stuff to the new 
> file directly? Removing a whole file and have its content in another file is hard to follow...

I have always used git for my patches to m-c, so late now, but will try mc in future

> Could we just skip showing the warning box about pointerlock if we are already in fullscreen?

Would be marginally easier code wise, I think either way makes sense, Verdi do you want to make a call?

> Please also rename WarningBox (and .warning-box) to something more distinctive. 
> FullscreenAndPointerLockWarningBox is verbose, maybe you can come up with something shorter.

FSPointerWarning?

Xidorn, are there any tests for the showing and the contents of the warning (for fullscreen)? I would like to test this but I cant find tests for related fullscreen functionality, will it be sufficient if the current pointerlock tests pass?

	Matej Novak [:matej]
	Comment 32 • 8 years ago

(In reply to Mike Conley (:mconley) - (Away until June 29th) from comment #20)
> I agree that we might want some wordsmithing here to ensure maximum clarity
> - matej is the right folk for the job.

	Matej Novak [:matej]
	Comment 33 • 8 years ago

(In reply to Matej Novak [:matej] from comment #32)
> (In reply to Mike Conley (:mconley) - (Away until June 29th) from comment
> #20)
> > I agree that we might want some wordsmithing here to ensure maximum clarity
> > - matej is the right folk for the job.

The right person for this is now Michelle Heubusch, who I have flagged here.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 34 • 8 years ago

(In reply to Dale Harvey (:daleharvey) from comment #31)
> > Could you use hg move rather than renaming and adding stuff to the new 
> > file directly? Removing a whole file and have its content in another file is hard to follow...
> 
> I have always used git for my patches to m-c, so late now, but will try mc
> in future

You can tweak the -M and -C git options to adjust its move / copy detection so it records the right thing.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 35 • 8 years ago

(In reply to Dale Harvey (:daleharvey) from comment #31)
> Xidorn, are there any tests for the showing and the contents of the warning
> (for fullscreen)? I would like to test this but I cant find tests for
> related fullscreen functionality, will it be sufficient if the current
> pointerlock tests pass?

I'm not aware of any UI test around the warning. I think it is sufficient if the current pointerlock tests pass.

	mheubusch
	Comment 36 • 8 years ago

Hi - I recommend the following wording:

<site> has control of your pointer. Press Esc to take back control. 

I realize this is longer than the existing message, but think it is more specific/precise than what is there now.

	Dale Harvey (:daleharvey) Assignee
	Comment 37 • 8 years ago

Made the name change recommendations, I remove the separate WarningBox + PointerLockAndFullScreen implementations into a single PointerFsWarning since it was an unnecessary decoupling and the warning needs to know when both are active. 

Xidorn has mentioned we could get rid of the combined message when pointerlock + fullscreen since we dont currently warn when pointerlock is requested in fullscreen. However code at the least needs to understand when both are requested so it doesnt attempt to show both warnings, showing a more informative message in this situation is then pretty much a free improvement, but I can take it out.

Went through the mochitests and all seem good, but will do a try run as well

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 38 • 8 years ago

(In reply to Dale Harvey (:daleharvey) from comment #37)
> Created attachment 8765861 [details] [diff] [review]
> Remove pointerlock permission requirement
> 
> Xidorn has mentioned we could get rid of the combined message when
> pointerlock + fullscreen since we dont currently warn when pointerlock is
> requested in fullscreen. However code at the least needs to understand when
> both are requested so it doesnt attempt to show both warnings, showing a
> more informative message in this situation is then pretty much a free
> improvement, but I can take it out.

That is simply because you have made the code more complicated... (And probably also because you were not aware of that DOM API can also be used.)

For example, what is the point of PointerFsWarning._toShow? If we don't show warning for pointerlock in fullscreen at all, the two functions can be simplified to something like
> showPointerLock: function(aOrigin) {
>   // We don't show warning for pointer lock in fullscreen.
>   if (document.fullscreen) {
>     return;
>   }
>   this._timer =
>     setTimeout(this._show.bind(this, "pointerlock", aOrigin), this._delay);
> },
>
> showFullScreen: function(aOrigin) {
>   this._timer =
>     setTimeout(this._show.bind(this, "fullscreen", aOrigin), this._delay);
> },

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 39 • 8 years ago

Or probably even simplier... I don't see how PointerFsWarning._delay is needed if we do not need to aggregate both warnings. So actually the two functions can be something like
> showPointerLock: function(aOrigin) {
>   // We don't show warning for pointer lock in fullscreen.
>   if (!document.fullscreen) {
>     this._show("pointerlock", aOrigin);
>   }
> },
> 
> showFullScreen: function(aOrigin) {
>   this._show("fullscreen", aOrigin);
> },

Much simpler than your current patch, no?

	Dale Harvey (:daleharvey) Assignee
	Comment 40 • 8 years ago

This would show 2 warnings overlayed if the pointerlock was acquired just before the fullscreen though would it not? we could hide the pointerlock one however I didnt think flashing / changing warnings quickly on screen was a good idea

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 41 • 8 years ago

Given that currently pointerlock is only free when in fullscreen, I don't think people would normally request pointer just before fullscreen.

Also by default we have a fullscreen transition, which would hide everything behind black before we are ready.

In addition, even if things above are all false, the size / position / content of the window would in general be changed significantly when entering fullscreen.

Given all these, I don't think there could be any problem to just hide any on-going pointerlock warning directly when we enter fullscreen. You could probably do that in FullScreen.enterDomFullscreen().

	Dale Harvey (:daleharvey) Assignee
	Comment 42 • 8 years ago

Amended the patch to only show pointerlock or fullscreen warnings, not both combined on Xidorns advice

	Dale Harvey (:daleharvey) Assignee
	Comment 43 • 8 years ago

And pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=6face9bfa952

	Tanvi Vyas[:tanvi]
	Comment 44 • 8 years ago

Can frames (or non-same-origin-frames) request pointer lock access?

	Dão Gottwald [:dao]
	Comment 45 • 8 years ago

Comment on attachment 8765880 [details] [diff] [review]
Remove pointerlock permission requirement.

As noted earlier, please make sure that browser-fullScreen.js -> browser-fullScreenAndPointerLock.js is properly registered as a move rather than a deletion and a new file addition. We don't want to lose this file's annotations. This will also make it easier to review the patch.

	Dale Harvey (:daleharvey) Assignee
	Comment 46 • 8 years ago

ok using -M let git detect this as a move, hopefully this is easier, cheers

	Dale Harvey (:daleharvey) Assignee
	Comment 47 • 8 years ago

Updated to fix xpcshell tests

	Dão Gottwald [:dao]
	Comment 48 • 8 years ago

Comment on attachment 8766372 [details] [diff] [review]
Remove pointerlock permission requirement.

nit: I'd prefer PointerlockFsWarning over PointerFsWarning for the object, and pointerlockFsWarning or pointerlock-fs-warning over pointerfswarning for the CSS classes. Brevity is nice but not at the price of making code harder to read and understand.

>@@ -625,8 +667,13 @@ var FullScreen = {
> XPCOMUtils.defineLazyGetter(FullScreen, "useLionFullScreen", function() {
>   // We'll only use OS X Lion full screen if we're
>   // * on OS X
>   // * on Lion or higher (Darwin 11+)
>   // * have fullscreenbutton="true"
>   return AppConstants.isPlatformAndVersionAtLeast("macosx", 11) &&
>          document.documentElement.getAttribute("fullscreenbutton") == "true";
> });
>+
>+
>+
>+
>+

please drop this change

>+addEventListener("MozDOMPointerLock:Entered", function(aEvent) {
>+  sendAsyncMessage('PointerLock:Entered', {

nit: double quotes

>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
> pref("full-screen-api.warning.timeout", 3000);
> // delay for the warning box to show when pointer stays on the top, unit: ms
> pref("full-screen-api.warning.delay", 500);
> 
>+// DOM pointerlock API
>+// time for the warning box stays on the screen before sliding out, unit: ms
>+pref("pointerlock-api.warning.timeout", 3000);
>+// delay for the warning box to show when pointer stays on the top, unit: ms
>+pref("pointerlock-api.warning.delay", 500);
>+
>+// time for the warning box stays on the screen before sliding out, unit: ms
>+pref("pointerlock-fullscreen-api.warning.timeout", 3000);
>+// delay for the warning box to show when pointer stays on the top, unit: ms
>+pref("pointerlock-fullscreen-api.warning.delay", 500);

Can we just use pointerlock-fullscreen-api.warning.* in all three cases?

Look good otherwise!

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 49 • 8 years ago

(In reply to Dão Gottwald [:dao] from comment #48)
> Comment on attachment 8766372 [details] [diff] [review]
> Remove pointerlock permission requirement.
> 
> >--- a/modules/libpref/init/all.js
> >+++ b/modules/libpref/init/all.js
> > pref("full-screen-api.warning.timeout", 3000);
> > // delay for the warning box to show when pointer stays on the top, unit: ms
> > pref("full-screen-api.warning.delay", 500);
> > 
> >+// DOM pointerlock API
> >+// time for the warning box stays on the screen before sliding out, unit: ms
> >+pref("pointerlock-api.warning.timeout", 3000);
> >+// delay for the warning box to show when pointer stays on the top, unit: ms
> >+pref("pointerlock-api.warning.delay", 500);
> >+
> >+// time for the warning box stays on the screen before sliding out, unit: ms
> >+pref("pointerlock-fullscreen-api.warning.timeout", 3000);
> >+// delay for the warning box to show when pointer stays on the top, unit: ms
> >+pref("pointerlock-fullscreen-api.warning.delay", 500);
> 
> Can we just use pointerlock-fullscreen-api.warning.* in all three cases?

There should be only two cases, since we shouldn't display pointerlock warning in fullscreen.

And let's not merge the two prefs, because some people tend to disable warning of fullscreen, and I would be worry about that they have to disable the warning of pointerlock at the same time without knowing what that means.

Also, pointerlock doesn't need the delay pref, because you have no way to trigger the warning again after your pointer gets locked.

	Dale Harvey (:daleharvey) Assignee
	Comment 50 • 8 years ago

Renamed PointerFsWarning to PointerlockFsWarning along with consistent class names, removed whitespace changes + fixed quoting and removed the unneeded pref that Xidorn pointed out. I left a new pref there for the reason Xidorn gave. 

Thanks

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 51 • 8 years ago

I would recommend you use MozReview for this kind of patch next time, because that handles code movement more sensible than Splinter (which happens to be an issue in this case).

	Pulsebot
	Comment 52 • 8 years ago

Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/06241d818e99
Remove pointerlock permission requirement. r=dao

	Dale Harvey (:daleharvey) Assignee
	Comment 53 • 8 years ago

Great thanks, been pushed to try twice and hasnt shown any errors related to the patch (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5eccdf0fbd1&selectedJob=23509914) so have landed. Will try MozReview next time.

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 8 years ago

Mike pinged me on IRC about doing some testing on this once it makes it to m-c.

	Phil Ringnalda (:philor)
	Comment 55 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/06241d818e99

	Francesco Lodolo [:flod]
	Comment 56 • 8 years ago

http://hg.mozilla.org/mozilla-central/diff/06241d818e99/browser/locales/en-US/chrome/browser/browser.dtd

Two notes:
* One sentence ends with a period, the other doesn't. Is that wanted?
* Can we please add localization comments to these strings, explaining how they're used and what the final result will be? Happy to file a follow-up bug if you prefer, or you can use this for a follow-up patch.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 57 • 8 years ago

Ahh, in addition, I think this should be put after leaveDOMFullScreen.accesskey, and it should probably have a PC version and a Mac version like exitDOMFullscreen.button.

	Dale Harvey (:daleharvey) Assignee
	Comment 58 • 8 years ago

I will follow up here

	Dale Harvey (:daleharvey) Assignee
	Comment 59 • 8 years ago

How about this?

	Francesco Lodolo [:flod]
	Comment 60 • 8 years ago

Comment on attachment 8769662 [details] [diff] [review]
ollow up to fix localisation

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +138,5 @@
>  <!ENTITY leaveDOMFullScreen.accesskey "u">
>  
> +<!-- LOCALIZATION NOTE (pointerlockWarning.afterDomain.label,
> +     pointerlockWarning.generic.label): the "escape" button on PC keyboards
> +     is uppercase, while on Mac keyboards it is lowercase. -->

Please explain how strings are concatenated, and why the beforeDomain label is empty for English.

I would also order the strings in a more logical way (the Mac afterDomain string is in the middle of generic strings).

	Dale Harvey (:daleharvey) Assignee
	Comment 61 • 8 years ago

Any better? its exactly the same reasoning as fullscreen so I was trying to skip the duplication by referring to it, but if thats what is best.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 62 • 8 years ago

Comment on attachment 8769671 [details] [diff] [review]
Follow up to fix localisation.

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

Not a peer of browser. Redirect to dao.

This looks mostly fine to me. I guess it might still be worth to add a localization note for the non-mac and mac strings to describe their difference.

This does looks annoying. I hope there could be an entity for string of escape key which is platform-specific, so that we don't need to have two separate strings for everything needs it... Not sure whether that is feasible.

	Francesco Lodolo [:flod]
	Comment 63 • 8 years ago

Comment on attachment 8769671 [details] [diff] [review]
Follow up to fix localisation.

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

Thanks, this looks good. What seems like a duplication for devs (seeing the whole file) is not for localizers, who only see the comment for the strings they're localizing.

	Mike Conley (:mconley) (:⚙️)
	Comment 64 • 8 years ago

Hey flod - is it not necessary to have a Mac-version of the beforeDomain label, in case the Mac-specific key description needs to go before the domain?

	Kris Maglione [:kmag]
	Comment 65 • 8 years ago

This file rename causes ESLint to spew errors like:

Could not load globals from file browser/base/content/browser-fullScreen.js: Error: ENOENT: no such file or directory, open '/home/kris/code/mozilla-central/browser/base/content/browser-fullScreen.js'

I'm not sure why it doesn't cause a treeherder failure, but please fix it or back this out.

	Francesco Lodolo [:flod]
	Comment 66 • 8 years ago

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #64)
> Hey flod - is it not necessary to have a Mac-version of the beforeDomain
> label, in case the Mac-specific key description needs to go before the
> domain?

No need for a different beforeDomain label, the comment can go in the current localization comment, explaining that 'esc' is specific for Mac, 'Esc' is for keyboards in other platforms.

	Dão Gottwald [:dao]
	Comment 67 • 8 years ago

Comment on attachment 8769671 [details] [diff] [review]
Follow up to fix localisation.

I don't think we need to or should lower-case Esc for mac mid-sentence. It looks weird and a quick online search for '"press the esc key" mac' seems to confirm that it's commonly written as Esc.

	Dale Harvey (:daleharvey) Assignee
	Comment 68 • 8 years ago

> This file rename causes ESLint to spew errors like:

Apologies, will figure out why I am not seeing linting errors, this is being fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1285936

> I don't think we need to or should lower-case Esc for mac mid-sentence. It looks weird and a 
> quick online search for '"press the esc key" mac' seems to confirm that it's commonly written as Esc.

Sounds reasonable, will do a patch without the special cases

	Dale Harvey (:daleharvey) Assignee
	Comment 69 • 8 years ago

Removed the mac especial casing, but fixed the extra period and added the comment which were flod's primary concerns

	Julian Descottes [:jdescottes]
	Comment 70 • 8 years ago

(In reply to Kris Maglione [:kmag] from comment #65)
> This file rename causes ESLint to spew errors like:
> 
> Could not load globals from file browser/base/content/browser-fullScreen.js:
> Error: ENOENT: no such file or directory, open
> '/home/kris/code/mozilla-central/browser/base/content/browser-fullScreen.js'
> 
> I'm not sure why it doesn't cause a treeherder failure, but please fix it or
> back this out.

This is not causing treeherder failures because this is only a warning. It pollutes the eslint report, but only eslint errors will make treeherder fail. 

As Dale mentioned, I am working on updating the eslint plugin in Bug 1285936. Just running into some issues with plugin dependencies that got updated in the meantime.

	Dão Gottwald [:dao]
	Comment 71 • 8 years ago

Comment on attachment 8770484 [details] [diff] [review]
Follow up to fix localisation

Since the message consists of two sentences separated by a period, it would seem more consistent to me to have the second sentence end with a period too.

r=me with that fixed or some UX person confirming that we should indeed omit the final period.

	Dale Harvey (:daleharvey) Assignee
	Comment 72 • 8 years ago

Should we include or remove the trailing period?

	Dale Harvey (:daleharvey) Assignee
	Comment 73 • 8 years ago

Sorry meant to post more context, should "has control of your pointer. Press Esc to take back control." have a trialing period or not?

	mheubusch
	Comment 74 • 8 years ago

Yes, trailing period please. It is a complete sentence. Normally, the only time I wouldn't end with a period is when there is a link like "Learn more"

	Pulsebot
	Comment 75 • 8 years ago

Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6648e2942cf9
Follow up to fix localisation. r=dao

	Brindusa Tot, Desktop QA
	Comment 76 • 8 years ago

Start working on test cases for verification of this bug.
Adding myself to QA Contact.

	Carsten Book [:Tomcat]
	Comment 77 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6648e2942cf9

	Verdi [:verdi] Best to slack me
	Comment 78 • 8 years ago

Comment on attachment 8762020 [details]
Screen Shot 2016-06-10 at 13.10.24.png

Clearing the feedback request - was done in comment #9

	Verdi [:verdi] Best to slack me
	Comment 79 • 8 years ago

(In reply to Dale Harvey (:daleharvey) from comment #31)
> > Could we just skip showing the warning box about pointerlock if we are already in fullscreen?
> 
> Would be marginally easier code wise, I think either way makes sense, Verdi
> do you want to make a call?

Sorry for taking too long! Yes this is good.

	Dale Harvey (:daleharvey) Assignee
	Comment 80 • 8 years ago

Clearing needinfo

	Simona Badau, Desktop QA
	Comment 81 • 8 years ago
testplan

I can confirm that the notification for permission requirement for PointerLock API was removed and when pointer lock is activated, a warning bar is displayed with the following message "<site> has control of your pointer. Press Esc to take back control". If full screen is requested as well as Pointer Lock, then only the full screen warning is shown.

More details in regards to the testing activity that was done on this feature, are available here:
https://wiki.mozilla.org/QA/Pointer_Lock_API_Improvement

	Andrei Vaida [:avaida]
	Comment 82 • 8 years ago

Changing QA Contact since this feature will be handled by the Release QA Team until it's shipped.

	Will Bamberg [:wbamberg]
	Comment 83 • 8 years ago

Updated Firefox 50 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/50