Closed
Bug 1273351
Opened 9 years ago
Closed 8 years ago
Get rid of permission requirement for PointerLock API
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla50
People
(Reporter: xidorn, Assigned: daleharvey)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: btpp-fixlater)
Attachments
(3 files, 9 obsolete files)
116.02 KB,
image/png
|
Details | |
46.45 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Still need approval from security people. ni? dveditz for this.
Blocks: pointer-lock
Flags: needinfo?(dveditz)
Reporter | ||
Comment 2•9 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.
Blocks: 1255338
Updated•9 years ago
|
tracking-e10s:
--- → ?
Updated•9 years ago
|
Blocks: platform-ui-team
Comment 3•9 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.
Flags: needinfo?(dveditz)
Reporter | ||
Comment 4•9 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.
Flags: needinfo?(dveditz)
Updated•9 years ago
|
Whiteboard: btpp-fixlater
Comment 5•9 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.
Flags: needinfo?(dveditz)
Assignee | ||
Comment 6•9 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
Flags: needinfo?(mverdi)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dale
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
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
Attachment #8762020 -
Flags: feedback?(mverdi)
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.
Flags: needinfo?(mverdi)
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
Attachment #8763329 -
Flags: feedback?(mconley)
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.)
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 :)
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
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
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.
Attachment #8763329 -
Flags: feedback?(mconley) → feedback+
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.
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.
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.
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".
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.
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.
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.
Attachment #8764526 -
Flags: feedback?(dao+bmo)
Attachment #8764526 -
Flags: feedback?(bugzilla)
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
Attachment #8764526 -
Flags: feedback?(dao+bmo) → feedback+
Updated•8 years ago
|
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...
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8763329 -
Attachment is obsolete: true
Attachment #8764526 -
Attachment is obsolete: true
Attachment #8764526 -
Flags: feedback?(bugzilla)
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?
Flags: needinfo?(bugzilla)
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...
Flags: needinfo?(bugzilla) → needinfo?(dale)
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?
Flags: needinfo?(matej)
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?
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.
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?
Flags: needinfo?(mverdi)
Flags: needinfo?(dale)
Flags: needinfo?(bugzilla)
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.
Flags: needinfo?(matej) → needinfo?(mheubusch)
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.
(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.
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.
Flags: needinfo?(bugzilla)
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.
Flags: needinfo?(mheubusch)
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
Attachment #8764726 -
Attachment is obsolete: true
Attachment #8765861 -
Flags: review?(dao+bmo)
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);
> },
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?
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
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().
Assignee | ||
Comment 42•8 years ago
|
||
Amended the patch to only show pointerlock or fullscreen warnings, not both combined on Xidorns advice
Attachment #8765861 -
Attachment is obsolete: true
Attachment #8765861 -
Flags: review?(dao+bmo)
Attachment #8765880 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 43•8 years ago
|
||
And pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=6face9bfa952
Comment 44•8 years ago
|
||
Can frames (or non-same-origin-frames) request pointer lock access?
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.
Attachment #8765880 -
Flags: review?(dao+bmo) → review-
Assignee | ||
Comment 46•8 years ago
|
||
ok using -M let git detect this as a move, hopefully this is easier, cheers
Attachment #8765880 -
Attachment is obsolete: true
Attachment #8766268 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 47•8 years ago
|
||
Updated to fix xpcshell tests
Attachment #8766268 -
Attachment is obsolete: true
Attachment #8766268 -
Flags: review?(dao+bmo)
Attachment #8766372 -
Flags: review?(dao+bmo)
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!
Attachment #8766372 -
Flags: review?(dao+bmo)
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.
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
Attachment #8766372 -
Attachment is obsolete: true
Attachment #8768848 -
Flags: review?(dao+bmo)
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).
Updated•8 years ago
|
Attachment #8768848 -
Flags: review?(dao+bmo) → review+
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
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.
Comment 54•8 years ago
|
||
Mike pinged me on IRC about doing some testing on this once it makes it to m-c.
Flags: needinfo?(rares.bologa)
Comment 55•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Flags: needinfo?(dale)
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.
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).
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.
Attachment #8769662 -
Attachment is obsolete: true
Attachment #8769662 -
Flags: review?(xidorn+moz)
Attachment #8769671 -
Flags: review?(xidorn+moz)
Attachment #8769671 -
Flags: feedback?(francesco.lodolo)
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.
Attachment #8769671 -
Flags: review?(xidorn+moz) → review?(dao+bmo)
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.
Attachment #8769671 -
Flags: feedback?(francesco.lodolo) → feedback+
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?
Flags: needinfo?(francesco.lodolo)
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.
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.
Flags: needinfo?(francesco.lodolo)
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.
Attachment #8769671 -
Flags: review?(dao+bmo) → review-
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
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
Attachment #8769671 -
Attachment is obsolete: true
Attachment #8770484 -
Flags: review?(dao+bmo)
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.
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.
Attachment #8770484 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 72•8 years ago
|
||
Should we include or remove the trailing period?
Flags: needinfo?(mheubusch)
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?
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"
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
Comment 76•8 years ago
|
||
Start working on test cases for verification of this bug.
Adding myself to QA Contact.
Flags: needinfo?(brindusa.tot)
QA Contact: brindusa.tot
Comment 77•8 years ago
|
||
bugherder |
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
Attachment #8762020 -
Flags: feedback?(mverdi)
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.
Flags: needinfo?(mverdi)
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
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 82•8 years ago
|
||
Changing QA Contact since this feature will be handled by the Release QA Team until it's shipped.
QA Contact: brindusa.tot → alexandru.simonca
Comment 83•8 years ago
|
||
Updated Firefox 50 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/50
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•