User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 Build Identifier: Mouse Lock Prototype Request I'm under the impression that to make a specification change a prototype is recommended from an implementor. So if someone is interested in writing a prototype of this then it would be appreciated. Purpose: To allow browsers to lock the mouse to the region inside of a DOM element. The other feature is the ability to get the change of the mouse position in a mousemove event. The reason for this is to further the movement of applications into the browser. In the realm of games locking the mouse opens up the ability for a first person shooter (FPS), such as the Quake WebGL demo, to use mouse look. It also allows real-time strategy games to scroll the map by moving the cursor to the edge of the game screen. In normal applications it allows google maps to allow someone to drag the map without stopping just because the mouse hit the edge of the screen. Implementation: Two events are required and could be registered to any DOM element: mouselock(event) mouseunlock(event) mouselock is invoked after the mouse cursor is successfully locked to an element. The event in the function could be called MouseLockEvent. This fits the current naming scheme of MouseEvent, KeyboardEvent, and WheelEvent. Three methods would be added to elements: element.lockMouse() element.unlockMouse() element.setMousePosition(x, y) lockMouse would lock the mouse to the DOM region. If called on another element it would unlock on the current element and then lock on the new one. unlockMouse would unlock the mouse from the DOM region if currently locked. It would be implementation dependent if a restriction is placed on the lockMouse function. In the specification there should be a suggestion to implementors that on the first call a balloon (such as the yellow bar in Firefox or Chrome) would ask: "This website has requested to lock your mouse [Allow] [Deny] [What is this?]" This would be per domain ideally to prevent exploitation. (It's still up for discussion if more should be done for users to prevent locking the mouse continuously so that users don't have to close the browser if that occurs). Assuming the mouse is locked then calling element.setMousePosition(x, y) which would move the mouse pointer relative to the locked region. Release Key It has also been brought up that there should be uniform support for a release key separate from the webpage releasing the key. A recommendation should be made for making the escape key the release key. It would simply call element.unlockMouse() on the current mouse locked DOM element. Fullscreen An application in fullscreen shouldn't have to ask for permission to lock the mouse. However if the webpage isn't given permission to lock the mouse then switching out of fullscreen would unlock the mouse. Mouse Move Delta The other feature required is the ability to get the change in the mouse position in the mousemove event. This requires modifying the MouseEvent to add a deltaX and deltaY variable representing the changes in the mouse position. However there is a side affect to this which might be a bad design decision. The WheelEvent inherits the MouseEvent and has its own deltaX and deltaY. Either different names should be made for deltaX and deltaY or deltaX and deltaY should be removed from WheelEvent. They're both the same data type so there's no conflicts in moving deltaX and deltaY to the MouseEvent structure. They'd just be used differently. In mousemove deltaX and deltaY would represent the change in the mouse position and in a wheel event they'd represent the mouse wheel scroll deltas as defined in the DOM Level 3 specification. Use case from the mailing list: It applies to non-game uses, too. For example, a common annoyance with Google Maps is when you're dragging the map and your mouse cursor hits the side of the screen, the map stops moving; you have to release the button and reposition the cursor. Mouse grabbing would trivially fix this. -- Glenn Maynard To illustrate this you have a mousedown event on the map region that calls element.lockMouse() then on mouseup calls element.unlockMouse() give or take. Buglist Entry at W3C where I log changes to the ideas. I'd recommend posting there. If not I'll probably merge the ideas I see elsewhere into it so they're in the same place: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9557#c12 Mailing list entries can be found here which seems more active: http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/ Reproducible: Always

So why couldn't you just extend the existing setCapture API a bit to allow capturing also when mouse is not down?

I guess it was because setCapture can't be abused really and seemed separate. I could see how this idea could be merged into it one API though. Still seems like everything I said would need to be separate from the setCapture and releaseCapture methods.

There is some more work happening @W3C but before implementing there really needs to be some draft specification to tell what to implement.

Based on IRC discussion, it looks like a lot of people agree we need this, and it isn't hard to do, but there is some lack of clarity as to how to do it. I suggest doing something similar to what SDL does, http://sdl.beuc.net/sdl.wiki/SDL_WarpMouse basically let the page move the mouse. So an FPS game would always move it to the center of the screen each frame. How about window.warpMouse(x, y); where x and y are between 0 and 1 (where in the window to move to)? We would also need an option to hide/show the cursor (so it doesn't always appear in the middle of the screen in an FPS). We can either have an API call for that, or even simpler just hide the cursor for X frames each time warpMouse is called. Regarding security, I think this should require explicit user permission, and should have a clear exit key to leave this mode. Sort of like fullscreen (but fullscreen is much more dangerous due to the risk of spoofing). Flash already does something similar to this, and despite Flash being on 98% of desktops, there haven't been too many problems with it, so I think we should do it.

There's been a discussion on http://www.w3.org/Bugs/Public/show_bug.cgi?id=9557 including a draft spec incorporating the feedback https://docs.google.com/document/d/1uV4uDVIe9-8XdVndW8nNGWBfqn9ieeop-5TRfScOG_o/edit?hl=en_US&authkey=CM-dw7QG Exposing only a warp event has the drawback that if the ratio of window size to frequency that JS code can make the call is too low then the mouse can escape the content area, allowing clicks elsewhere. Also it may prove difficult to implement delta motion tracking in the application unless some guarantees can be made about the ordering of mouse move events and or feedback of the warp event. E.g. if a warp is called while the mouse is being moved, will there be a mouse move event with the cursor at the warped position, or will it be the warp position + the movement that had been occurring? If the latter, the app must assume all movement events delivered after a call to warp include the offset of the warp, which requires the implementation to flush or fix up any buffers from driver/os/user agent.

NOTE: we don't need any new API bits for showing/hiding the cursor. I'm attaching a simple demo of how to do it with css.

Thanks for the info scheib, I was not aware there was a wip spec for this. I was just throwing out some ideas, but looks like they are not necessary :) That draft spec looks good to me.

scheib, are you guys implementing this yet, or just working on this doc?

I've code in progress, http://code.google.com/p/chromium/issues/detail?id=72754 ETA Chrome 16 ~ Oct 17 stable.

I was considering a proposal for this feature, so it's awesome to see you guys are already working on a prototype. My thoughts: Make sure that the unlockmouse event triggers in response to the user causing the mouse to be unlocked, not just in response to content unlocking it. I assume that's how it works, but that should be clearly stated. The behavior in the presence of multiple frames, background tabs, etc. needs to be clearly specified. At a minimum, I would suggest that this should only work if the frame locking the mouse has input focus, and that the lock should be broken immediately upon loss of focus. That would ensure that a lot of the standard keys (alt-tab, winkey on win32, for example) would properly break the lock and allow regular use of the PC. This is another thing that typical PC games often get wrong and it impairs usability. Also, there are real security concerns here - clickjacking comes to mind. In terms of user confirmation, I think opt-in is unnecessary and will probably frustrate the user; instead, any time the mouse is locked, a clear visible indicator should appear that lets the user know what button to press to release the mouse. This, alongside all the standard keys (like alt-tab) working, should be enough for the user to recover if 'bad content' is stealing the mouse. I also think the indicator should *always* be visible so that attempted clickjacking, etc. is at least possible to identify just by looking at the indicator, instead of having to spot suspicious behavior. Further re: clickjacking; setMousePosition seems very risky in terms of it being possible to move the cursor during/immediately before a click event, redirecting a user click, potentially towards important chrome. At a minimum, this should be mitigated by specifying that the cursor can only be moved onto a pixel that is occupied by the frame that has locked the mouse (instead of a child frame, or a parent frame, or even outside the document window). If the element the mouse is locked to is repositioned, or the document is scrolled, what happens? What happens if you lock the mouse to an element that is invisible or has 0x0 or negative dimensions? The meaning of the delta motion in mouse events should be clearly specified if possible (I think delta values are a really, really good addition, so I definitely want them to stay) - I think if it's in screen pixels that's fine, but it would be optimal if it could be raw mouse motion data (without acceleration and sensitivity applied), so that it's possible for content to interpret the raw data. I do not know if it is feasible to get this data on all platforms. And just to toss in my 2 cents, I'm not a fan of the WarpMouse approach for the reasons previously stated - it's way too fragile.

I'm wondering about implementing this spec, but in talking to Jonas, I'd first like to sort out whether or not it could land, and in particular, the UX/security issues surrounding it. Alex, can I get some input from you on the process for figuring this out? I don't want to go down this road if it isn't landable. Thanks for your help.

humph, are you sure the right people to answer comment 12 are already cc'd? (I'm not certain who you mean there, and don't think I see my guesses regarding who you mean in the cc list.)

Alon, you make a good point, and I'll admit that I'm not clear who I need to get into this bug to answer my questions. My current goal is to figure out whether or not I should invest the time implementing this spec or no, i.e., will we land this if I do? Connecting to that is the question of how to deal with the user experience side of enabling/disabling this. I could easily partition the problem, and just do the back-end bits here. I CC'ed faaborg with the hope that he'd jump in. Maybe people have other suggestions. Everyone I've asked has indicated that the people already CC'ed are the right ones.

I talked very briefly to faaborg about this last week, and he agreed that getting security right would indeed be the trick for getting this accepted into core Firefox. Off-the-top-of-his-head, he suggested that having it just work in full-screen mode and have it explicitly be exited with a keystroke (like escape) might be a good start. Alex, is that a reasonable summary of our conversation?

That's definitely a good start, but we will want support for this in window-mode as well. Also, escape might not be the best choice. Something that's not likely to be used by an application (escape could be used to bring up a menu) and that isn't likely to be hit by accident would be best. How about ctrl+alt, or ctrl+shift+alt?

I think most parties has agreed that enabling this on full-screen only for now will solve a good chunk of the use cases while avoiding almost all of the security concerns. Another thing that was brought up in the thread on the w3c list (or whatwg list, i'm not sure) was that a lot of use cases only need the mouse to be locked as long as the mouse button is pressed. If we could make it possible in the API to opt in to automatic release once the mouse button is release, then we could enable that in non-fullscreen mode too. Obviously this behavior doesn't work for all use cases, such as first-person shooter games. Hence it should be opt-in in the API and not the only behavior.

(In reply to Alan Kligman from comment #16) > That's definitely a good start, but we will want support for this in > window-mode as well. Also, escape might not be the best choice. Something > that's not likely to be used by an application (escape could be used to > bring up a menu) and that isn't likely to be hit by accident would be best. > How about ctrl+alt, or ctrl+shift+alt? The idea is to make this intuitive for everyone. The escape key allows that functionality. If a user wants to display a menu they can by detecting the unlock mouse event and displaying a menu. Most menus don't require mouse lock so this plays into things nicely. Using a confusing keyboard shortcut system will complicate applications for those that aren't computer savvy. The specification drafted by the chrome people already contains a special comment about fullscreen usage. It's in the section "A full screen approach". I don't think incremental development is required. I believe the chrome guys are already working on their prototype which features all the functionality.

>The escape key allows that functionality. If a user wants to display a menu they >can by detecting the unlock mouse event and displaying a menu. Most menus don't >require mouse lock so this plays into things nicely. we'll in this case esc would also bring the user out of full screen mode. If the application wants to: -go full screen -take over the mouse [and] - override the esc key It is probably going to need to be in a different tier of permission from the open web. We're in the process of sorting this out, but examples would include being installed from a trusted store, or the user going through a process to grant the specific web site permissions to do all of these things.

(In reply to Jonas Sicking (:sicking) from comment #17) > Another thing that was brought up in the thread on the w3c list (or whatwg > list, i'm not sure) was that a lot of use cases only need the mouse to be > locked as long as the mouse button is pressed. If we could make it possible > in the API to opt in to automatic release once the mouse button is release, > then we could enable that in non-fullscreen mode too. We have capturing content for that case already. (element.setCapture/releaseCapture)

But that doesn't do the cursor "wrapping", right? I.e. using that API doesn't prevent the cursor from hitting one side of the screen and then clamping, right?

(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #19) >> The escape key allows that functionality. If a user wants to display a menu they >> can by detecting the unlock mouse event and displaying a menu. Most menus don't >> require mouse lock so this plays into things nicely. > > we'll in this case esc would also bring the user out of full screen mode. > If the application wants to: > > -go full screen > -take over the mouse > [and] - override the esc key > > It is probably going to need to be in a different tier of permission from > the open web. We're in the process of sorting this out, but examples would > include being installed from a trusted store, or the user going through a > process to grant the specific web site permissions to do all of these things. f11 in all browsers transitions to fullscreen. No one is suggesting the ability to force the application into fullscreen mode I don't believe. The idea is if the user presses f11 then mouse lock can be called freely with no permission worries. Pressing escape would unlock the mouse like normal. If f11 is pressed and the user doesn't have permission for mouse lock then mouse lock is released and can't be called again.

right, if the user initiated full screen then that's fine. We're also thinking about notifications for an app that wants to go full screen, that the user could approve.

Taking...I'll do this as part of my lectures this fall as an example for my Mozilla dev class.

CC'ing Chris and Robert, who have been thinking about similar things with the full screen API, cf. http://robert.ocallahan.org/2011/08/securing-full-screen.html

The design of this is still being actively discussed in the webapps group. It's still unclear to me what the API should be.

Spec has moved to W3C site from Google Docs - http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html

It may be worth while to abstract mouse lock to cover other input locking APIs such as keyboard lock (https://bugzilla.mozilla.org/show_bug.cgi?id=700123).

Please consider that pixel-clamping, sensitivity and acceleration applied by the OS to the mouse cursor are very detrimental to mouse-look algorithms (they only make generic sense in an UI context, not in a 3d spatial view control mode). Many games of the 90ties had mouse-clamping and acceleration issues. DirectInput, XInput and Cocoa support obtaining raw mouse ticks. I urge you to implement raw mouse ticks at least complementary to pixel-clamped, OS-smoothed and accelerated mouse movements.

That would mean that we should make .deltaX and .deltaY be floats and not integers?

(In reply to Jonas Sicking (:sicking) from comment #30) > That would mean that we should make .deltaX and .deltaY be floats and not > integers? If memory serves me right, mouse "ticks" are 16-bit integers and they represent the dots that the device scanned in relative mode while moving. i.e. for a 600dpi mouse you will get 600 ticks for one inch of movement.

So, ideally, .deltaX and .deltaY would be ticks/mouseDPI*screenDPI? That would be ok, but take into account that the initial spec specifies that deltaX/Y should be in screen pixels units.

The spec as it stands pushes 'raw' data access to a later version. The primary challenge is specifying the units of mouse movement. This topic is also highly related to 'unbundling' the spec into components. See FAQ section "Why bundle all functionality (hiding cursor, providing mouse deltas) instead of using CSS to hide the cursor, always providing delta values, and offering an API to restrict the cursor movement to a portion of the web page?" discusses this. http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html (copied below). """ There are good motivations to provide a more fine grained approach. E.g. the use case "View-port panning by moving a mouse cursor against the bounds of a view-port" doesn't require hiding the mouse cursor, only bounding it and always having delta values available. Also, this specification defines the movement deltas to be taken from how the system mouse cursor moves, which incorporates operating system filtering and acceleration of the mouse movement data. Applications may desire access to a more raw form of movement data prior to adjustments appropriate for a mouse cursor. Also, raw data may provide better than pixel level accuracy for movement, as well as higher frequency updates. Providing the raw delta movement would also not require special permission or mode from a user, and for some set of applications that do not require bounding the cursor may reduce the security barriers and prompts needed. There are two justifications for postponing this finer grained approach. The first is a concern of specifying what units mouse movement data are provided in. This specification defines .movementX/Y precisely as the same values that could be recorded when the mouse is not under lock by changes in .screenX/Y. Implementations across multiple user agents and operating systems will easily be able to meet that requirement and provide application developers and users with a consistent experience. Further, users are expected to have already configured the full system of hardware input and operating system options resulting in a comfortable control the system mouse cursor. By specifying .movementX/Y in the same units mouse lock API applications will be instantly usable to all users because they have already settled their preferences. Secondly, the implementation of providing movement data and bounding the mouse cursor is more difficult in the fine grained approach. Bundling the features together gives implementations freedom to use a variety of techniques as appropriate on each operating system and is more practical to implement. Direct APIs do not exist on all platforms (Win, Mac, Linux) to bound the cursor to a specific rectangle, and prototypes have not yet been developed to demonstrate building that behavior by e.g. invisible windows with xlib or manual cursor movement on Mac. Unaccelerated Delta values have been proposed to be accessed by reading raw Human Interface Device (HID) data. E.g. WM_INPUT messages on windows, and USB device APIs on Mac / Linux. The challenge here is interpreting and normalizing the units to some consistent and specifiable scale. Also, most APIs considered to date are limited to USB devices. It would be reasonable to consider adding these capabilities in the future, as the currently specified mouse lock API would be easy to continue to support if the finer grained delta and confinement features were implemented. """

(In reply to John Villar from comment #32) > So, ideally, .deltaX and .deltaY would be ticks/mouseDPI*screenDPI? That > would be ok, but take into account that the initial spec specifies that > deltaX/Y should be in screen pixels units. I know the spec calls for the movement reported to be in relation to screen size. And this makes sense if you mean to interact on a 2d plane that maps more or less 1:1 to screen size (like say an iso-view RTS). For instance it would be irksome to have to move the mouse half as much per unit of space on screen if you dragged the app to a different monitor. However, dividing by screen resolution for any non 1:1 mapping of mousemovement has no meaning. Why would you need to drag the mouse twice as much to accomplish a 90° turn in an FPS when you put the app on a different monitor? It would be extremely irksome to have the same device react in different ways to a 3d-spatial input depending on display resolution. The division by input device resolution probably makes sense, however I think obtaining that information is impossible to do reliably (the device-info fields of many drivers/HIDs are missing, incomplete or missleading). In practise the only viable way to discover resolution would be to measure it and remember in the application.

And you have found exactly why the spec doesn't goes to specify how to process raw movements. Implementations miss vital information to be able to specify the concrete unit of measure for the raw data, i.e. if you let the vendors specify the raw data in "ticks" then movement on one client with 1200dpi would be totally different on another with only 120dpi (in fact, 10x slower). Letting the devs "identify" the hardware on the user side is a no-no on the web. I presume the most reliable way for this spec to be implemented across all vendors is to specify the units to what already the user has configured, that's why vince's proposal to let it process "pixel" units is the best tradeoff right now.

(In reply to John Villar from comment #35) > And you have found exactly why the spec doesn't goes to specify how to > process raw movements. Implementations miss vital information to be able to > specify the concrete unit of measure for the raw data, i.e. if you let the > vendors specify the raw data in "ticks" then movement on one client with > 1200dpi would be totally different on another with only 120dpi (in fact, 10x > slower). Letting the devs "identify" the hardware on the user side is a > no-no on the web. I presume the most reliable way for this spec to be > implemented across all vendors is to specify the units to what already the > user has configured, that's why vince's proposal to let it process "pixel" > units is the best tradeoff right now. It is an unavoidable fact that to get comfortable mapping between an input device and a 3d-spatial output, that you will have to adjust the sensitivity to what you want. The sensitivity cannot be adjusted browser or OS wide, because what sensitivity a specific user for a specific application prefers is not related in any way to 2d mouse cursor comfort, it is related to viewing and control comfort of 3d-scenes which is influenced by many other factors then what is usually under consideration for simply mapping a cursor (like nausea, precision of point&overshoot, how many times you have to lift the mouse and drag it back and so forth). Those sensitivity settings will be implemented anyway by anybody who seriously considers a mouse-look supporting app. However you should be aware that the ratio of spatial movement of the mouse to the ratio of directional change in a 3d-scene is a factor to which people get used. So when people switch between say a 16:9 to a 4:3 display resolution (by way of a second monitor or any such scheme), then they will get extremely irritated to have to adjust their sensitivity depending on what monitor they happen to play WebGL Quake.

(In reply to Florian Bösch from comment #36) > The sensitivity cannot be adjusted browser or OS wide, > because what sensitivity a specific user for a specific application prefers > is not related in any way to 2d mouse cursor comfort, it is related to > viewing and control comfort of 3d-scenes which is influenced by many other > factors then what is usually under consideration for simply mapping a cursor Indeed, we're talking on the same fact here... however, giving access to direct raw data doesn't solve this problem. I have had lots of trouble when switching mouses on certain FPSs because they indeed use raw data and don't adjust too well when i use a mouse with a different DPI than what the dev knew at the moment. > Those sensitivity settings will be implemented anyway by anybody who > seriously considers a mouse-look supporting app. > [...] However you should be aware that the ratio of spatial movement of the mouse > to the ratio of directional change in a 3d-scene is a factor to which people > get used. So when people switch between say a 16:9 to a 4:3 display > resolution (by way of a second monitor or any such scheme), then they will > get extremely irritated to have to adjust their sensitivity depending on > what monitor they happen to play WebGL Quake. I'm aware of that, and that's also why precision should be left (for now) as a scale factor over the pixel unit proposed on the spec. You could easily multiply by a certain adjustment factor (floating point from, say, 0.1 to 10) to adjust the mouse sensitivity based on a user preference. I think that for the *initial* spec, raw movement should be left out because it adds some complexities yet not well understood by the browser vendors. When the standard catches on and "WebGL Quake" becomes a reality, new use cases will pop up based on new needs and then a revision to the spec should be due.

WebGL Quake is a rea(In reply to John Villar from comment #37) > When the standard catches on and "WebGL Quake" becomes a reality, new use > cases will pop up based on new needs and then a revision to the spec should > be due. WebGL Quake is a reality: - gwt quake2: http://crystalin.dyndns.org:8080/GwtQuake.html - quake3: http://media.tojicode.com/q3bsp/ - irrlicht webgl quake3: http://www.ambiera.at/copperlicht/demos/demo_quakelevel_external.html I might add, every one of them plays miserably without fullscreen and mouse capture.

(In reply to Florian Bösch from comment #38) > WebGL Quake is a reality: > - gwt quake2: http://crystalin.dyndns.org:8080/GwtQuake.html > - quake3: http://media.tojicode.com/q3bsp/ > - irrlicht webgl quake3: > http://www.ambiera.at/copperlicht/demos/demo_quakelevel_external.html > > I might add, every one of them plays miserably without fullscreen and mouse > capture. I knew they were a "semi" reality (quake without mouse lock isn't quake, is just a gimmick). I know copperlicht and every other webgl engine out there will benefit greatly from mouse lock.

A note to let you know that this bug isn't being neglected, and that you can follow my work to implement it with my class here http://vocamus.net/dave/?cat=28. I'm just not spamming the bug until we have a patch for review, which shouldn't be too far away.

Done!! requires PythonExt: Win - http://mozos.net/pyWin8.xpi Lin - http://mozos.net/pyLin8x.xpi http://mozos.net/pyRaton.xpi http://mozos.net/install.xpi http://bvidda.net/wiki/MozOS There are some demos. Enjoy!

I don't think that's the point of the ticket. This is about getting the functionality native to mozilla. There is a group of people making progress on this, you might want to check out their blog posts/progress wiki. (In reply to METALY from comment #41) > Done!! requires PythonExt: > > Win - http://mozos.net/pyWin8.xpi > Lin - http://mozos.net/pyLin8x.xpi > > http://mozos.net/pyRaton.xpi > http://mozos.net/install.xpi > > http://bvidda.net/wiki/MozOS > > There are some demos. Enjoy!

http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html > Win - http://mozos.net/pyWin8.xpi > Lin - http://mozos.net/pyLin8x.xpi The mouse lock API is specified by this document: http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html Which is what both firefox (this ticket) and chrome (http://code.google.com/p/chromium/issues/detail?id=72754) are going to implement. Both implementations will arrive shortly. The firefox implementation can be tested already. Your API is not following the draft specification: "CubicVR.MouseViewController(canvas, scene.camera);" is wrong. This array of plugins you require to mouse capture will not work in Chrome. When the implementations for Chrome and Firefox arrive, your plugins will be obsolete. Using the implementation trough plugins is cumbersome for the user and fraught with security issues.

You will need a way of positioning the mouse. Ive made a component in python just for that. Cc["@mozos.net/pyRaton;1"]. getService(Ci.pyIRaton). setPos(X,Y);

Doing what you do to the mouse cursor will break both your plugin and the mouse lock implementation currently underway in mozilla. I suggest to add this plugin to the blocklist for the version of mozilla that will have mouse-lock built in. (In reply to METALY from comment #44) > You will need a way of positioning the mouse. > Ive made a component in python just for that. > > Cc["@mozos.net/pyRaton;1"]. > getService(Ci.pyIRaton). > setPos(X,Y);

De donde yo vengo las cosas se hacen o no se hacen, no te asignas el trabajo para pasarte un mes tirandote el cuento. Ni menos te pasas de listo comentando trivialidades cuando no conoces los medios de quien pudiera llevarlo a cabo. A ver si nos arreglan el nsWindows y podemos terminarlo en javascript.

(In reply to METALY from comment #46) > De donde yo vengo las cosas se hacen o no se hacen, no te asignas el trabajo > para pasarte un mes tirandote el cuento. Ni menos te pasas de listo > comentando trivialidades cuando no conoces los medios de quien pudiera > llevarlo a cabo. > A ver si nos arreglan el nsWindows y podemos terminarlo en javascript. Please METALY, message this list only on english. I natively speak spanish, but this list is for everyone else to read. Also, take into account that the Pointer Lock "Standard" is just a draft proposal right now, and no browser is obligated to implement it yet. According to what i have personally chatted with Vincent Scheib from google chrome, it isn't that easy to implement there... i guess it is the same here. So please, patience.

> Also, take into account that the Pointer Lock "Standard" is just a draft > proposal right now, and no browser is obligated to implement it yet. > According to what i have personally chatted with Vincent Scheib from google > chrome, it isn't that easy to implement there... i guess it is the same > here. So please, patience. We're done the Mozilla implementation, and just fixing a few tests. I should have a patch up for review by Friday.

That's a rather interesting development ;-) Is there any way one can test your particular flavor of mozilla?

> Is there any way one can test your particular flavor of mozilla? I'll do Firefox builds on try server when I post the patch, so you can test.

Today is the last day of my course, and I'm happy to be able to present a working patch on behalf of the class. This was a lot of fun to do, and we enjoyed the experience. At a high-level, this patch does the following: * Mouse Lock is only possible for an element, and an element in the current document, and an element in fullscreen, as per security review * Mouse Lock is lost when the browser comes out of fullscreen * Adds MouseLockable DOM type * Adds navigator.pointer (I assume we'll want mozPointer, but I haven't done that here) * Remove mouse cursor (i.e., display) when mouse is locked * Adds synthetic mouse movement to GTK2, and helper functions for other platforms, in order to recentre the mouse after movement while locked * Fires mouselocklost events when coming out of mouse lock, either by script or through losing fullscreen, focus, etc. * Adds movementX and movemenetY to MouseEvent, which report the difference between the current and previous screenX/Y positions * Adds back-end changes to keep track of movement deltas reported by movementX and movementY * screenX, screenY, clientX, clientY all report a constant value of 0 when the mouse is locked * mouseover and mouseout are not reported while the mouse is locked * Saves the screenX and screenY position before locking the mouse, restores it upon unlock * Attempting to lock a locked element fires the success callback again * A simple user pref has been added to disable mouse lock. No other UI has been introduced, since we piggy-back on fullscreen (this might be something we want to add, but I didn't do it without discussion of how it should be done). * Removing a locked element from the DOM takes the browser out of mouse lock Other things are visible in the code itself, and probably don't need to get called out here. Some things this patch does not (yet) do, and for which discussion would be helpful: * The spec says that once lock is acquired that we should "stop mouse events from being fired to other elements that are not locked (e.g., only fire to locked element)." I haven't found an elegant way to do this yet, and would appreciate some guidance on the right place to do it. While forcing fullscreen makes it unlikely for other elements to receive events, it is possible to float an element over the fullscreen element, for example. * There are various things we get for free because we piggy-back on fullscreen, which will later need consideration if/when we move out of forcing fullscreen for mouse lock. For example, the ESC key should exit mouse lock. This will currently exit fullscreen, and therefore mouse lock. As discussed above, we have no UI or other mouse-lock specific interactions. If you press ESC, you lose mouse lock and fullscreen. Do we want to separate those? It's certainly possible with script to just unlock the mouse. * The spec says, "User agents may prompt for confirmation before locking, this preference may be saved as a content setting" Are we going to do this? If so, I'd suggest we consider doing it in another bug. * The spec says, "Repeated escapes of mouse lock can signal user agent to not re-lock the mouse without more specific user intent gesture, e.g. similar to how Chrome suppresses repeated alert() calls" We haven't done this, and it probably needs some consideration. * With a high resolution mouse, and especially in multi-monitor setups (where fullscreen only covers a single monitor), it's possible to break the mouse out of the locked element (to test, open your Web Console, and scroll up fast while locked). Upon re-entering the window, it goes back to being locked. However, we might want to clip the mouse to the element's rectangle, which would mean adding ClipCursor, CGAssociateMouseAndMouseCursorPosition and CGGetLastMouseDelta, and XGrabPointer to widget code, I believe. I haven't done this without hearing that you want it first, and if this is the right move. * What about mobile? * A few renaming things (for Vincent): the spec calls for islocked(), which numerous people, including myself, think should be isLocked(). I've asked Vincent if we can change this. Also, there was talk of s/Mouse Lock/Pointer Lock/, but that hasn't happened, afaik. If it is going to happen, I'd love to do it during these reviews. Vincent? I'll post our current tests after this (we have ~65 working tests, but another 20 or 30 with issues still). The students are still wrapping their heads around some of the trickier test scenarios. We also hit some platform-specific issues with move events firing too often, which might be related to our implementation. I've put working demos up at http://humphd.github.com/mozilla-central/mouselock/index.html that you can try.

One other UX related note. Take a look at this video of the Mouse Lock enabled Quake 3 WebGL demo Brandon Jones made this week to test this code: http://www.youtube.com/watch?v=d_LD5qsCTLQ You'll see that games which use fullscreen + mouse lock + WASD style keyboard interaction are going to have that "Press ESC..." banner on all the time. I talked to Chris about this, and he and Rob are thinking about possible ways to deal with this issue. It's going to be something we need to solve soon after this lands.

(In reply to David Humphrey (:humph) from comment #51) > * Mouse Lock is only possible for an element, and an element in the current > document, and an element in fullscreen, as per security review So I understand this right as: if element AND document.contains(element) AND element.isFullscreen then allow fullscreen? I point out again that this is not mandated by the draft specification. It is also not how Chrome is going to implement it and it makes it impossible to use mouse-lock without fullscreen, which can be desired in some cases.

Awesome! Re: "screenX, screenY, clientX, clientY all report a constant value of 0 when the mouse is locked": This contradicts current spec "When mouse lock is enabled clientX, clientY, screenX, and screenY must hold constant values as if the mouse did not move at all once mouse lock was entered". Re: islocked -> isLocked. I agree, am implementing the same in Chrome, and have updated the spec. Re: rename "mouse lock" -> "pointer lock". I've done the rename in the spec for the API items: PointerLock interface IDL, pointerlocklost Event. (If I missed something let me know, I'm deferring/avoiding updating all terminology in the spec esp since it is likely migrating to web-apps). rename change in spec: http://dvcs.w3.org/hg/webevents/rev/6c149e83cd5d fyi, the chrome UI is sticking with 'Mouse" terminology as it is more familiar for users. http://www.chromium.org/developers/design-documents/mouse-lock, e.g. "google.com is disabling your mouse cursor. Press ESC to exit." Re: Florian's concern of limiting to fullscreen: Chrome 16 & 17 only support mouse lock in full screen mode as well, this can (and is planned to) be changed in future versions.

Comment on attachment 580543 [details] [diff] [review] Patch v1 > nsresult > nsINode::RemoveChild(nsINode *aOldChild) > { >+ nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aOldChild); >+ if (node) { >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ node->GetOwnerDocument(getter_AddRefs(domDoc)); >+ if (domDoc) { >+ nsCOMPtr<nsIDOMHTMLElement> lockedElement; >+ domDoc->GetMozFullScreenElement(getter_AddRefs(lockedElement)); >+ >+ nsCOMPtr<nsIDOMNode> lockedNode = do_QueryInterface(lockedElement); >+ // If the element being removed is also currently >+ // mouse locked, then unlock the element. >+ if (node == lockedNode) { >+ nsCOMPtr<nsIDOMWindow> window; >+ domDoc->GetDefaultView(getter_AddRefs(window)); >+ if (window) { >+ nsCOMPtr<nsIDOMNavigator> navigator; >+ window->GetNavigator(getter_AddRefs(navigator)); >+ if (navigator) { >+ nsCOMPtr<nsIDOMMouseLockable> pointer; >+ navigator->GetPointer(getter_AddRefs(pointer)); >+ if (pointer) { >+ // Unlock the mouse >+ pointer->Unlock(); >+ } >+ } >+ } >+ } >+ } >+ } >+ > if (!aOldChild) { > return NS_ERROR_NULL_POINTER; > } This is slow. Probably the fastest way to handle removal is to create a nsIMutationObserver object which observers the locked node and calls unlock when nsIMutationObserver::ParentChainChanged is called. >+ if (!((nsGUIEvent*)mEvent)->widget ) { extra space before ) >+nsDOMUIEvent::GetScreenPoint() >+{ >+ if (IsMouseLocked()) { >+ return nsIntPoint(0, 0); >+ } >+ >+ return ScreenPointInternal(); >+} >+ >+nsIntPoint > nsDOMUIEvent::GetClientPoint() > { >+ if (IsMouseLocked()) { >+ return nsIntPoint(0, 0); >+ } Change these to follow the spec as scheib indicated > case NS_MOUSE_MOVE: > { >+ if (mMouseLockedElement && aEvent->widget) { >+ // Perform mouse lock by recentering the mouse directly, then remembering the deltas. >+ nsIntRect bounds; >+ aEvent->widget->GetScreenBounds(bounds); >+ aEvent->lastRefPoint = nsIntPoint(bounds.width/2, bounds.height/2); why / 2? >+ // The element that is mouse locked, if any. >+ nsCOMPtr<nsIContent> mMouseLockedElement; Shouldn't this be a static member variable? Or how does the patch handle cases when the document which has iframe and mouse moves over those iframes. >+nsDOMMouseLockable::ShouldLock(nsIDOMElement* aTarget) >+{ >+ // Check if mouselock is enabled in prefs >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService("@mozilla.org/preferences-service;1")); >+ bool mouseLockEnabled; >+ prefs->GetBoolPref(PREF_MOUSE_LOCK_ENABLED, &mouseLockEnabled); >+ if (!mouseLockEnabled) { >+ return false; >+ } >+ >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ mWindow->GetDocument(getter_AddRefs(domDoc)); >+ if (!domDoc) { >+ NS_ERROR("ShouldLock(): Unable to get document"); >+ return NS_ERROR_UNEXPECTED; >+ } >+ >+ // Check if element is in the DOM tree >+ nsCOMPtr<nsIDOMNode> targetNode(do_QueryInterface(aTarget)); >+ if (!targetNode) { >+ return false; >+ } >+ nsCOMPtr<nsIDOMNode> parentNode; >+ targetNode->GetParentNode(getter_AddRefs(parentNode)); >+ if (!parentNode) { >+ return false; >+ } QI to nsINode and check IsInDoc(). >+ >+NS_IMETHODIMP nsDOMMouseLockable::Lock(nsIDOMElement* aTarget, >+ nsIDOMMouseLockableSuccessCallback* aSuccessCallback, >+ nsIDOMMouseLockableFailureCallback* aFailureCallback) >+{ >+ nsRefPtr<nsMouseLockableRequest> request = >+ new nsMouseLockableRequest(aSuccessCallback, aFailureCallback); You should store aTarget (QI'ed to nsINode or nsIContent or Element or nsIDOMEventTarget) so that when calling callback you push Cx to stack; nsCxPusher pusher: // defined in nsContentUtils.h NS_ENSURE_STATE(pusher.Push(target)); >+ nsCOMPtr<nsIRunnable> ev; >+ >+ // If we're already locked to this target, re-call success callback >+ if (mMouseLockedElement && mMouseLockedElement == aTarget){ >+ ev = new nsRequestMouseLockEvent(true, request); >+ } else if (ShouldLock(aTarget)) { >+ mMouseLockedElement = aTarget; >+ >+ // TODO: should these throw or cause the error callback? >+ nsCOMPtr<nsPIDOMWindow> domWindow( do_QueryInterface( mWindow ) ); Extra space around ( and ) >+class nsMouseLockableRequest : public nsISupports { { should be in the next line >+class nsDOMMouseLockable : public nsIDOMMouseLockable { { to next line >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIDOMMOUSELOCKABLE >+ >+ nsDOMMouseLockable(); >+ nsresult Init(nsIDOMWindow*); >+ >+private: >+ ~nsDOMMouseLockable(); >+ bool ShouldLock(nsIDOMElement*); >+ >+ nsCOMPtr<nsIDOMWindow> mWindow; >+ nsCOMPtr<nsIDOMElement> mMouseLockedElement; >+}; Looks like this should be cycle collectable. > nsIDOMPerformanceNavigation.idl \ >+ nsIDOMMouseLockable.idl \ Some missing tabs before \ ? >+[scriptable, function, uuid(877ee0b4-1661-11e1-a667-206a8a092638)] >+interface nsIDOMMouseLockableSuccessCallback : nsISupports { { to next line >+ void handleEvent(); >+}; Looks like the spec is buggy here. It should define the callback, and the method in callback shouldn't be handleEvent (and the callback shouldn't be [FunctionOnly=] but since the spec doesn't the callback interface at all, hard to say anything exact here...). Sorry for the delay in the review. (I should also review the spec since it seems to have some problems)

Could you have a a separate interface for .pointer (nsNavigator would implement it). That way it would be easy to have a pref for pointer. Somewhat similar to nsIDOMMozNavigatorBattery

(In reply to Olli Pettay [:smaug] from comment #57) > Could you have a a separate interface for .pointer (nsNavigator would > implement it). That way > it would be easy to have a pref for pointer. > Somewhat similar to nsIDOMMozNavigatorBattery I asked jst about whether to put it directly on Navigator or do a new interface, and he suggested that I not introduce a new one. But if that's what you want, I can do it.

I think there should be a new interface so that we can disable the feature easily (if there are some problems and we don't want to ship it in FF11/FF12). Also, the API should be probably prefixed. So, navigator.mozPointer and all the interfaces should start with nsIDOMMoz Is webkit implementation prefixed ?

WebKit prefixes: navigator.webkitPointer.unlock(); navigator.webkitPointer.lock(); navigator.webkitPointer.isLocked(); document.body.addEventListener("webkitpointerlocklost", ... mouseMoveEvent.webkitMovementX mouseMoveEvent.webkitMovementY

ok. Does Webkit prefix also the interfaces? (I hope so)

The webkit interface constructor needs to be added to window/global namespace. In the process I will also rename it to WebKitPointerLock.

I sent some comments to the webapps wg http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/1558.html

Thanks for the review. We've tried to fix everything you've listed, and are still sorting out the tests, based on the rename changes in this patch. I'll post those after the break (I'm going on holidays, so don't rush to review this until the new year). I just wanted to get it up before I left.

Comment on attachment 583691 [details] [diff] [review] Patch v2 Review of attachment 583691 [details] [diff] [review]: ----------------------------------------------------------------- Quick feedback I had while reading the code by curiosity. Mostly coding style and nits. I didn't look deeply at the patch though. ::: content/base/src/nsDocument.cpp @@ +8542,5 @@ > + > + // When exiting fullscreen, if the pointer is also locked to the fullscreen element, > + // we'll need to unlock it as we the document exits fullscreen. > + nsCOMPtr<nsIDOMWindow> window = aDocument->GetWindow(); > + if (window) { if (!window) { return; } @@ +8545,5 @@ > + nsCOMPtr<nsIDOMWindow> window = aDocument->GetWindow(); > + if (window) { > + nsCOMPtr<nsIDOMNavigator> navigator; > + window->GetNavigator(getter_AddRefs(navigator)); > + if (navigator) { ditto @@ +8548,5 @@ > + window->GetNavigator(getter_AddRefs(navigator)); > + if (navigator) { > + nsCOMPtr<nsIDOMMozNavigatorPointerLock> navigatorPointerLock = > + do_QueryInterface(navigator); > + if (navigatorPointerLock) { ditto @@ +8551,5 @@ > + do_QueryInterface(navigator); > + if (navigatorPointerLock) { > + nsCOMPtr<nsIDOMMozPointerLock> pointer; > + navigatorPointerLock->GetMozPointer(getter_AddRefs(pointer)); > + if (pointer) { ditto ::: content/events/src/nsDOMMouseEvent.cpp @@ +272,5 @@ > } > return NS_OK; > } > > +NS_METHOD nsDOMMouseEvent::GetMozMovementX(PRInt32* aMovementX) NS_IMETHODIMP @@ +274,5 @@ > } > > +NS_METHOD nsDOMMouseEvent::GetMozMovementX(PRInt32* aMovementX) > +{ > + NS_ENSURE_ARG_POINTER(aMovementX); This is over protective: no one should call this with |nsnull|. @@ +283,5 @@ > + > +NS_IMETHODIMP > +nsDOMMouseEvent::GetMozMovementY(PRInt32* aMovementY) > +{ > + NS_ENSURE_ARG_POINTER(aMovementY); ditto ::: dom/base/Navigator.cpp @@ +928,5 @@ > + > +NS_IMETHODIMP > +Navigator::GetMozPointer(nsIDOMMozPointerLock** aPointer) > +{ > + NS_ENSURE_ARG_POINTER(aPointer); Not useful. @@ +932,5 @@ > + NS_ENSURE_ARG_POINTER(aPointer); > + > + if (!mPointer) { > + mPointer = new nsDOMMozPointerLock(); > + } You should create the pointer after creating domWin. Here, if do_QueryReferent fails, you will have mPointer being initialized. @@ +934,5 @@ > + if (!mPointer) { > + mPointer = new nsDOMMozPointerLock(); > + } > + > + nsCOMPtr<nsIDOMWindow> domWin(do_QueryReferent(mWindow)); This seems nicer to me: (but really a matter of taste I believe) nsCOMPtr<nsIDOMWindow> domWin = do_QueryReferent(mWindow); @@ +944,5 @@ > + mPointer = nsnull; > + return NS_ERROR_FAILURE; > + } > + > + NS_ADDREF(*aPointer = mPointer); You could use .forget(). @@ +1179,5 @@ > > aAppName.AssignLiteral("Netscape"); > return NS_OK; > } > + Useless change? ::: dom/base/Navigator.h @@ +47,5 @@ > #include "nsIDOMNavigatorGeolocation.h" > #include "nsIDOMNavigatorDesktopNotification.h" > #include "nsIDOMClientInformation.h" > #include "nsIDOMNavigatorBattery.h" > +#include "nsDOMMozPointerLock.h" I believe you could use a forward declaration instead of #include the header. ::: dom/base/nsDOMMozPointerLock.h @@ +73,5 @@ > + nsCOMPtr<nsIDOMMozPointerLockFailureCallback> mFailureCallback; > +}; > + > + > +class nsRequestPointerLockEvent : public nsRunnable Is there any reason to make this class visible in the header? ::: dom/interfaces/base/nsIDOMNavigator.idl @@ +42,2 @@ > > +[scriptable, uuid(C2163BDA-359F-4883-95DE-916562F489E4)] You do not need that and the #include. This interface didn't change. ::: widget/src/gtk2/nsWindow.cpp @@ +6954,5 @@ > +nsWindow::SynthesizeNativeMouseEvent(nsIntPoint aPoint, > + PRUint32 aNativeMessage, > + PRUint32 aModifierFlags) > +{ > + if (mGdkWindow) { if (!mGdkWindow) { return NS_OK; } GdkDisplay* ... [...] return NS_OK;

Some API feedback that occurred to me the other day: The current API doesn't provide access to the "raw" mouse coordinates. I.e. you'll still be affected by things like cursor acceleration and post-release-momentum, right? It might be nice to provide an options object like: pointer.lock(targetElem, { raw: true }); or possibly: pointer.lock({ target: targetElem, raw: true }); Also, the feedback I keep getting from developers is that they prefer DOM-Events rather than callbacks. So an API like: request = pointer.lock(tragetElem); request.onsuccess = function(e) { ... }; which simplifies to pointer.lock(tragetElem).onsuccess = function(e) { ... }; would be more "webby" than using callback functions.

(In reply to Jonas Sicking (:sicking) from comment #66) > The current API doesn't provide access to the "raw" mouse coordinates. I.e. > you'll still be affected by things like cursor acceleration and > post-release-momentum, right? Spec FAQ addresses this a bit, there was a lot of discussion on raw device output back on the web-apps list maybe around August/Sept. http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html#why-bundle-all-functionality--hiding-cursor--providing-mouse-deltas----------instead-of-using-css-to-hide-the-cursor--always-providing-delta-values----------and-offering-an-api-to-restrict-the-cursor-movement-to-a-portion-of-the---------web-page "Unaccelerated Delta values have been proposed to be accessed by reading raw Human Interface Device (HID) data. E.g. WM_INPUT messages on windows, and USB device APIs on Mac / Linux. The challenge here is interpreting and normalizing the units to some consistent and specifiable scale. Also, most APIs considered to date are limited to USB devices." I maintain that we should expose the well defined / specifiable screen coordinates as a first step, and see if developers have issues remaining before trying to spec out raw data. Some developers have already reported that screen coordinates should be sufficient. Also we have production/commercial games already in development for Chrome via native client using mouse lock with no concerns raised yet. > Also, the feedback I keep getting from developers is that they prefer> DOM-Events rather than callbacks. So an API like: > > request = pointer.lock(tragetElem); > request.onsuccess = function(e) { ... }; > > which simplifies to > > pointer.lock(tragetElem).onsuccess = function(e) { ... }; Are there other examples of this API style you could reference to help point out the benefits? Could you point out a negative to the callback style? Would we do this XHR style and have a method that creates the request and a method that attempts the lock, or would we call the event as soon as a function is assigned to onsuccess? In the Chrome|WebKit implementation I have underway the success/failure of lock is sometimes synchronous.

(also, mind continuing the spec discussion on public-webapps@w3.org?)

> > Also, the feedback I keep getting from developers is that they prefer> DOM-Events rather than callbacks. So an API like: ... > Are there other examples of this API style you could reference to help point > out the benefits? Could you point out a negative to the callback style? Since we're piggy-backing on the fullscreen api [1] at the moment, the contrast between these two APIs is a useful example. Consider: document.addEventListener('fullscreenchange', onSuccess); document.addEventListener('fullscreenerror', onError); elem.requestFullScreen(); I think the event approach is, at least to me, less surprising. [1] https://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#api

(In reply to scheib from comment #67) > Also we have > production/commercial games already in development for Chrome via native > client using mouse lock with no concerns raised yet. This is a nonvalid argument. "Because we offer X and heard no complaint so far, it must be OK". It is not OK. In order to support a users preferences, a game would typically implement a movement scale factor. Some users use an average mouse speed in their OS settings, yet they choose a really high factor for 3D games. What typically happens in this situation is: A) They crank the game setting to the max, resulting in uncontrollable pixel jumps with angles that cannot be addressed due to mouse clamping. B) They go to their OS setting and crank up sensitivity to the max there, then go to the game and crank down sensitivity there. Then they play, then they are required to revert the sensitivity back. This is a not acceptable UX issue for games (like competitive FPS shooters).

Comment on attachment 583691 [details] [diff] [review] Patch v2 > // Remember the root document, so that if a full-screen document is hidden > // we can reset full-screen state in the remaining visible full-screen documents. > sFullScreenRootDoc = do_GetWeakReference(nsContentUtils::GetRootDocument(this)); > >+ // If a document is already in fullscreen, then unlock the mouse pointer >+ // before setting a new document to fullscreen >+ nsCOMPtr<nsIDocument> fullScreenDoc(do_QueryReferent(sFullScreenDoc)); This looks a bit slow. Could you just store the return value of nsContentUtils::GetRootDocument(this) somewhere and use that. > PRInt32 mLockCursor; >+ // The element that is mouse locked, if any. >+ static nsCOMPtr<nsIContent> mPointerLockedElement; What should happen if the mPointerLockedElement is removed from document? What should happen if it is moved to another document? > [scriptable, uuid(7f57aa45-6792-4d8b-ba5b-201533cf0b2f)] > interface nsIDOMMouseEvent : nsIDOMUIEvent update uuid >+XPIDLSRCS = \ >+ nsIDOMMozNavigatorPointerLock.idl \ >+ nsIDOMMozPointerLock.idl \ >+ nsIDOMMozPointerLockSuccessCallback.idl \ >+ nsIDOMMozPointerLockFailureCallback.idl \ >+ $(NULL) Something wrong with tabs or spaces >+[scriptable, uuid(12c0cd86-7cd6-4355-8e0a-6b42486f2b32)] >+interface nsIDOMMozPointerLock : nsISupports >+{ >+ void lock( in nsIDOMElement target, >+ [optional] in nsIDOMMozPointerLockSuccessCallback successCallback, >+ [optional] in nsIDOMMozPointerLockFailureCallback failureCallback); >+ void unlock(); >+ boolean isLocked(); I believe this is about to change to readonly boolean attribute isLocked >+[scriptable, function, uuid(3fd4368c-1662-11e1-82e6-206a8a092638)] >+interface nsIDOMMozPointerLockFailureCallback : nsISupports >+{ >+ void handleEvent(); >+}; Ok, the spec looks a bit buggy here to me. Why handleEvent()? why not handleCallback or something else. Also, why does the spec have =FunctionOnly. But in any case, if you want to follow the spec, so that the interface isn't exposed to web pages, I think dropping DOM from the interface name would hide the interface: ("MozPointerLockFailureCallback" in window) would be false >+[scriptable, function, uuid(877ee0b4-1661-11e1-a667-206a8a092638)] >+interface nsIDOMMozPointerLockSuccessCallback : nsISupports >+{ >+ void handleEvent(); >+}; Same here. Tests please :) Someone else needs to look at the widget level changes (not nsGUIEvent.h)

In general, looks really good, and surprisingly simple :)

> >+[scriptable, function, uuid(3fd4368c-1662-11e1-82e6-206a8a092638)] > >+interface nsIDOMMozPointerLockFailureCallback : nsISupports > >+{ > >+ void handleEvent(); > >+}; > > Ok, the spec looks a bit buggy here to me. > Why handleEvent()? why not handleCallback or something else. > Also, why does the spec have =FunctionOnly. > But in any case, if you want to follow the spec, so that the > interface isn't exposed to web pages, I think dropping > DOM from the interface name would hide the interface: > ("MozPointerLockFailureCallback" in window) would be false VoidCallback was adopted from previous specs, e.g. File API Directories and System http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#the-voidcallback-interface. Web IDL is dropping FunctionOnly, so I have dropped it from the spec. It's possible in the future the VoidCallback IDL should be removed and replaced with "callback VoidCallback = void ()" http://dev.w3.org/2006/webapi/WebIDL/#dfn-callbacks since we really don't care to specify anything but that a callback is possible.

Thanks for the reviews. This fixes issues raised in comment 65 and comment 71, and updates to trunk. Raymond and Steven (two of my students, cc'ed) are finishing the tests this week and will add a patch for those soon. I'll post a patch with widget-only changes for review after this. I've also updated the demos at http://humphd.github.com/mozilla-central/mouselock/ to match this patch.

As per comment 71, these are the widget changes separated from the rest of the patch.

Comment on attachment 589628 [details] [diff] [review] Patch v3 (widget only) Review of attachment 589628 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: widget/nsIWidget.h @@ +1339,5 @@ > > /** > + * A shortcut to SynthesizeNativeMouseEvent, abstracting away the native message. > + */ > + virtual nsresult SynthesizeNativeMouseMove(nsIntPoint aPoint) = 0; rev nsIWidget IID

Comment on attachment 589625 [details] [diff] [review] Patch v3 (complete) >+nsDOMUIEvent::IsPointerLocked() >+{ >+ if (!mView) { >+ return false; >+ } >+ >+ nsCOMPtr<nsIDOMNavigator> navigator; >+ mView->GetNavigator(getter_AddRefs(navigator)); >+ if (!navigator) { >+ return false; >+ } >+ >+ nsCOMPtr<nsIDOMMozNavigatorPointerLock> navigatorPointerLock = >+ do_QueryInterface(navigator); do_QueryInteface is null-safe, so no need to null check navigator before this >+nsEventStateManager::SetLastScreenOffset(nsIntPoint aScreenOffset) { { should be in the next line >+nsEventStateManager::GetMouseCoords(nsIntRect aScreenBounds) { ditto >+ nsCOMPtr<nsIDOMHTMLElement> lockedElement = do_QueryInterface(sPointerLockedElement); >+ if (!lockedElement) { >+ return nsIntPoint(0,0); >+ } >+ >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ lockedElement->GetOwnerDocument(getter_AddRefs(domDoc)); >+ if (!domDoc) { >+ return nsIntPoint(0,0); >+ } >+ >+ nsCOMPtr<nsIDOMWindow> domWin; >+ domDoc->GetDefaultView(getter_AddRefs(domWin)); >+ if (!domWin) { >+ return nsIntPoint(0,0); >+ } >+ >+ int offsetWidth, offsetHeight, offsetTop, offsetLeft, innerHeight; >+ domWin->GetInnerHeight(&innerHeight); >+ lockedElement->GetOffsetWidth(&offsetWidth); >+ lockedElement->GetOffsetHeight(&offsetHeight); >+ lockedElement->GetOffsetTop(&offsetTop); >+ lockedElement->GetOffsetLeft(&offsetLeft); >+ >+ /** >+ * X,Y coords of the center of the locked element >+ * >+ * The x coord is the width of the element divived by 2 >+ * plus the distance between the element and the left border of the window >+ * plus the distance between the left corner of the monitor and the browser >+ * >+ * The y coord is the height of the element divived by 2 >+ * plus the distance between the element and the top border of the inner window >+ * plus the height of the chrome window minus the height of the inner window >+ * plus the distance between the top corner of the monitor and the browser >+ **/ >+ return nsIntPoint((offsetWidth/2) + offsetLeft + aScreenBounds.x, >+ (offsetHeight/2) + offsetTop + (aScreenBounds.height - innerHeight) + aScreenBounds.y); >+} This looks actually a bit strange. Could you please use the internal APIs to get the size of viewport and element. mPresContext->GetVisibleArea() and sPointerLockedElement->GetPrimaryFrame()->GetRect() or something like that. >+ >+// nsPointerLockRequest >+NS_IMPL_THREADSAFE_ISUPPORTS0(nsPointerLockRequest) THREADSAFE? Why? >+[scriptable, function, uuid(3fd4368c-1662-11e1-82e6-206a8a092638)] >+interface nsIMozPointerLockFailureCallback : nsISupports >+{ >+ void handleEvent(); I think this should not be "handleEvent()", but since current spec has that, it is ok. >+[scriptable, function, uuid(877ee0b4-1661-11e1-a667-206a8a092638)] >+interface nsIMozPointerLockSuccessCallback : nsISupports >+{ >+ void handleEvent(); Same here I wish to see tests. I'd especially would like to see tests for event targeting. Per spec mousemove, mousedown, mouseup, click should all target to the locked element (even when mouse is over an iframe)

We finished fixing the things mentioned in reviews above, and will have a new patch up for review early next week. In the meantime, the spec has moved[1], and discussion has begun to normalize this spec with the fullscreen spec[2], which we'll do once Vincent has updated the spec. I think these changes make a lot of sense. [1] http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html [2] http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0376.html

Unfortunately the implementation of pointer lock and Element.mozRequestFullScreenWithKeys() (bug 716107) are going to conflict. :( We're planning on implementing Element.mozRequestFullScreenWithKeys() such that when called the browser enters fullscreen and shows a warning dialog ("Press ESC to exit full-screen mode") which additionally includes a checkbox with text saying something like "Show this warning on key press for $domain.com". If the user unchecks the checkbox, the warning won't appear again on keypress for that domain. (See attachment 592940 [details] for a WIP screenshot of the warning box). However if the website requests pointer lock while this warning is up, the user won't be able to uncheck the check box and grant key input. So we need some kind of way (from chrome) to temporarily suspend the pointer lock so that users can click on the checkbox. So... Can we add a boolean flag on some chrome-visible interface which we can use to toggle/short-circuit the pointer lock while the fullscreenWithKeys warning is visible? We'd need the pointer to still appear to content as if it was locked, but mouse events would still need to be dispatched to chrome. Or can we resolve this some other way?

Some my ideas for the patch. Why don't you use nsStubMutationObserver instead of nsIMutationObserver? http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsStubMutationObserver.h If you use it, you can remove the empty methods from nsDOMMozPointerLock. sLastScreenPoint and sLastClientPoint are modified when GetScreenPoint() or GetClientPoint() is called. It looks strange for me. I'm not sure where are calling them, but I guess that if stored event's these methods are called, the value may become unexpected value. I think that such mouse cursor position should be stored by ESM and modified by ESM::PreHandleEvent(). Anyway, the methods with such names shouldn't check static variables which will affects other behavior. And also, looks like you don't check if an event is a trusted event in many places. I'm not sure if it's intentional. I just worry about the case you forget untrusted events. I guess that you should reset mouse wheel transaction when ESM does lock/unlock the pointer. If so, ESM should call nsMouseWheelTransaction::EndTransaction() at that time. But I'm not sure what should be happened when mouse wheel is turned during locking the pointer. I assume that any scrollable frame shouldn't be scrolled by wheel events during locked. But if it should scroll a scrollable frame normally even though the scrollable frame is outside of the locking target frame, please ignore this paragraph.

Thank you for the reviews and suggestions. We have fixed the issues from roc's widget/ review in comment 76, as well as the issues from smaug's review in comment 77. We have also added the necessary tests. Thanks to the tests, we've been able to kill off a number of crash bugs in this patch. I should also note that we hit bug 723201 this week while debugging tests, but that I have since managed to stop it from occurring. This patch is also updated to trunk (af9f286b38fa). Pending your review comments, our remaining issues include: * look at suggestions in comment 80. smaug, maybe you'll have comments on these when you review... * figure out why DOMMouseScroll events aren't being retargeted to the locked element * look into roc's suggestion from bug 722449 comment 8, namely, using getBoundingClientRect vs. GetPrimaryFrame() Vincent, one thing I had to do in this patch, which you might want to consider for the spec (and WebKit), is fail to lock elements that lack a frame (e.g., display:none). I'm also interested to hear your response to comment 79. Finally, what is your ETA for updating the spec to reflect the changes to the DOM bits of Pointer Lock? I don't think I'll make those changes until they're in the spec. Finally, a word of recognition for my students--Diogo, Steven, and Raymond--who have bravely stuck with me, and this work, past the end of the semester. Hopefully we're almost there :)

David: Did you read my comment? And I guess that following test may fail. var mouseMoveWithoutLock, mouseMoveWithoutLockPoint; function mouseMoveHandler(event) { if (navigator.pointer.isLocked) { is(mouseMoveWithoutLock.screenX, mouseMoveWithoutLockPoint.x, ...); is(mouseMoveWithoutLock.screenY, mouseMoveWithoutLockPoint.y, ...); } else if (!mouseMoveWithoutLock) { mouseMoveWithoutLock = event; mouseMoveWithoutLockPoint = { x: event.screenX, y: event.screenY }; // assuming pointer will be locked later. } } because all methods in nsDOM*Event shouldn't refer *current* state, but your patch does it. # I realized that WidgetToScreenOffset() should be called when the event constructed and the result should be stored! Similarly, IsPointerLocked() should be stored to mIsPointerLocked at constructor. > * figure out why DOMMouseScroll events aren't being retargeted to the locked element It should be retargeted by PresShell::HandleEvent() with the event's refPoint and capturing state.

(In reply to Chris Pearce (:cpearce) from comment #79) Summary: How to handle situations such as full screen domain preferences that require a mouse cursor? I'm working on Chrome, and initially we have restricted mouse lock to be possible only after full screen has been permitted. We allow JS to request mouse lock, and it waits in a pending state until the user explicitly presses "Allow" on an info bubble. In the case that both full screen and mouse lock are in a pending state we merge the requests into the same info bubble, e.g. displaying "google.com is now full screen and wants to disable your mouse cursor. [Allow] [Exit]" See http://www.chromium.org/developers/design-documents/mouse-lock In the near future I plan to allow mouse lock in non-fullscreen mode. Because we want to avoid a mis-behaving page that constantly re-locks, a user key press or click will be required to aquire lock. In the absense of full screen offering that predicate, we will prompt the user for only mouse lock. If full screen is then engaged we'll merge back to the combined state. In all cases the mouse won't lock until the confirmation info bubble is dismissed. In general should we encounter a scenario where we feel we must prompt the user with UI that uses the mouse, and don't trust the web page to gracefully release the lock, then we'll force the unlock. At the moment I speculate that we would not 'temporarily' unlock, but instead would require the page to detect and re-aquire lock. My advice from afar is to either follow a similar route of merging the UI displayed to the user when fullscreen and mouse lock are requested, or queue up the mouse lock request to happen after the full screen dialog is dismissed.

(In reply to David Humphrey (:humph) from comment #81) > Vincent, ... > consider for the spec ... fail to lock elements that lack a > frame (e.g., display:none). I don't yet understand. If an element is in the DOM tree, but is display:none, it can still bubble mouse events (in Chrome and FireFox at least): """ document.body.innerHTML = "<div style='display:none'>hide you</div>" document.body.onclick = function () { console.log("click"); } e = document.createEvent("MouseEvents"); e.initEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null); document.getElementsByTagName("div")[0].dispatchEvent(e); """ prints "click" Until you tell me what I'm missing, I don't see a reason to not allow applications to lock the pointer and mouse events to a hidden element. > Finally, what is your ETA for updating the spec to reflect the > changes to the DOM bits of Pointer Lock? I don't think I'll make those > changes until they're in the spec. I've put a few hours into it, and the ETA is many more hours of editing, probably days before it's pushed up? > Finally, a word of recognition for my students--Diogo, Steven, and > Raymond--who have bravely stuck with me, and this work, past the end of the > semester. Hopefully we're almost there :) You guys rock.

(In reply to David Humphrey (:humph) from comment #81) > Finally, what is your ETA for updating the spec to reflect the > changes to the DOM bits of Pointer Lock? I don't think I'll make those > changes until they're in the spec. Spec has been updated as I described, with the exception of allowpointerlock which still needs to be added. http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html

What's the purpose of the 'pointerLockEnabled' property? It seems like null-checking pointerLockElement provides exactly the same information.

(In reply to Jonas Sicking (:sicking) from comment #86) > What's the purpose of the 'pointerLockEnabled' property? It seems like > null-checking pointerLockElement provides exactly the same information. Error on my part. Once incorporating the iframe permission attribute this should instead return true if the context and all ancestor contexts have permision to lock. e.g. from Fullscreen, "The fullscreenEnabled attribute must return true if the context object and all ancestor browsing context's documents have their fullscreen enabled flag set, or false otherwise." I've removed it.

David, could you update the patch to cover the problems Masayuki mentioned.

Yeah, I'll try to get it done this week.

Looking at Masayuki's comments, I have some questions. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #80) > Why don't you use nsStubMutationObserver instead of nsIMutationObserver? > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsStubMutationObserver.h > If you use it, you can remove the empty methods from nsDOMMozPointerLock. Thanks, I didn't know about this. I've made the change and will include it in my next patch. > sLastScreenPoint and sLastClientPoint are modified when GetScreenPoint() or > GetClientPoint() is called. It looks strange for me. I'm not sure where are > calling them, but I guess that if stored event's these methods are called, > the value may become unexpected value. I think that such mouse cursor > position should be stored by ESM and modified by ESM::PreHandleEvent(). > Anyway, the methods with such names shouldn't check static variables which > will affects other behavior. GetScreenPoint() and GetClientPoint() are always called, whether content calls them or not, so it seemed like putting it there meant the fewest code changes. I want to store the values as calculated (e.g., not the bare ref point), so if I move to ESM, I have to duplicate all the CSS/device/unit pixel conversions there too. > And also, looks like you don't check if an event is a trusted event in many > places. I'm not sure if it's intentional. I just worry about the case you > forget untrusted events. Can you say something about what I should/shouldn't do here? I'm not familiar with how untrusted events work (I see NS_IS_TRUSTED_EVENT and the flag), and how they should work in this context. > I guess that you should reset mouse wheel transaction when ESM does > lock/unlock the pointer. If so, ESM should call > nsMouseWheelTransaction::EndTransaction() at that time. But I'm not sure > what should be happened when mouse wheel is turned during locking the > pointer. I assume that any scrollable frame shouldn't be scrolled by wheel > events during locked. But if it should scroll a scrollable frame normally > even though the scrollable frame is outside of the locking target frame, > please ignore this paragraph. I'm also unclear on the right answer here, and don't know how to answer it. I've done as you suggested (resetting with ESM lock/unlock) in my new patch. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #82) > David: > > Did you read my comment? And I guess that following test may fail. Yes, just hadn't had a chance to process it yet :) Thanks for your comments. > var mouseMoveWithoutLock, mouseMoveWithoutLockPoint; > function mouseMoveHandler(event) > { > if (navigator.pointer.isLocked) { > is(mouseMoveWithoutLock.screenX, mouseMoveWithoutLockPoint.x, ...); > is(mouseMoveWithoutLock.screenY, mouseMoveWithoutLockPoint.y, ...); > } else if (!mouseMoveWithoutLock) { > mouseMoveWithoutLock = event; > mouseMoveWithoutLockPoint = { x: event.screenX, y: event.screenY }; > // assuming pointer will be locked later. > } > } > > because all methods in nsDOM*Event shouldn't refer *current* state, but your > patch does it. Sorry, I don't follow. Can you help me understand? > # I realized that WidgetToScreenOffset() should be called when the event > constructed and the result should be stored! That would mean changing how all the code works, since WidgetToScreenOffset is used throughout, and on demand. You're saying you want all these changed too? > Similarly, IsPointerLocked() should be stored to mIsPointerLocked at > constructor. > > > * figure out why DOMMouseScroll events aren't being retargeted to the locked element > > It should be retargeted by PresShell::HandleEvent() with the event's > refPoint and capturing state. Meaning it should just work now that I'm setting capture, or that I have to do something here? Please clarify. Thanks for your help and feedback.

(In reply to David Humphrey (:humph) from comment #90) > > sLastScreenPoint and sLastClientPoint are modified when GetScreenPoint() or > > GetClientPoint() is called. It looks strange for me. I'm not sure where are > > calling them, but I guess that if stored event's these methods are called, > > the value may become unexpected value. I think that such mouse cursor > > position should be stored by ESM and modified by ESM::PreHandleEvent(). > > Anyway, the methods with such names shouldn't check static variables which > > will affects other behavior. > > GetScreenPoint() and GetClientPoint() are always called, whether content > calls them or not, so it seemed like putting it there meant the fewest code > changes. I want to store the values as calculated (e.g., not the bare ref > point), so if I move to ESM, I have to duplicate all the CSS/device/unit > pixel conversions there too. First, I'm not sure whether the navigator object is shared in all contents. But anyway, I don't think it's good idea to use static variables for them. If web contents held mouse events and their methods are called after pointer lock, the result might be different than before mouse lock. So, the event object's life time isn't only during capturing and bubbling. And if you use static method means that the background contents also refers the static variable if the navigator object is shared. > > And also, looks like you don't check if an event is a trusted event in many > > places. I'm not sure if it's intentional. I just worry about the case you > > forget untrusted events. > > Can you say something about what I should/shouldn't do here? I'm not > familiar with how untrusted events work (I see NS_IS_TRUSTED_EVENT and the > flag), and how they should work in this context. I have no idea about actual bugs in your patch. Untrusted events are made by web contents like following code: var event = document.createEvent("MouseEvent"); event.initMouseEvent(....); And of course, they can dispatch these events by: document.getElementById("foo").dispatchEvent(event); Basically, such events shouldn't cause critical results. E.g., if untrusted key events could open autocomplete popup in editor, web contents could steal some data. In another case, if untrusted mouseup event could cancel D&D, it might have caused strange behavior if platform's D&D session had not been finished yet. So, handling untrusted events may cause security issue, or may block user's operation. > > I guess that you should reset mouse wheel transaction when ESM does > > lock/unlock the pointer. If so, ESM should call > > nsMouseWheelTransaction::EndTransaction() at that time. But I'm not sure > > what should be happened when mouse wheel is turned during locking the > > pointer. I assume that any scrollable frame shouldn't be scrolled by wheel > > events during locked. But if it should scroll a scrollable frame normally > > even though the scrollable frame is outside of the locking target frame, > > please ignore this paragraph. > > I'm also unclear on the right answer here, and don't know how to answer it. > I've done as you suggested (resetting with ESM lock/unlock) in my new patch. On Gecko, when a child scrollable element moves under the cursor by scrolling its ancestor scrollable element, wheel event doesn't change the scroll target immediately for usability (DOM events will be fired on the child element, but the scrolling (default action) target is the previous scrolling target) until timeout. I think that you should always reset these transaction when mouse cursor is locked/unlocked because users must not feel continuity between before it and after it. > > var mouseMoveWithoutLock, mouseMoveWithoutLockPoint; > > function mouseMoveHandler(event) > > { > > if (navigator.pointer.isLocked) { > > is(mouseMoveWithoutLock.screenX, mouseMoveWithoutLockPoint.x, ...); > > is(mouseMoveWithoutLock.screenY, mouseMoveWithoutLockPoint.y, ...); > > } else if (!mouseMoveWithoutLock) { > > mouseMoveWithoutLock = event; > > mouseMoveWithoutLockPoint = { x: event.screenX, y: event.screenY }; > > // assuming pointer will be locked later. > > } > > } > > > > because all methods in nsDOM*Event shouldn't refer *current* state, but your > > patch does it. > > Sorry, I don't follow. Can you help me understand? See first answer of this comment. > > # I realized that WidgetToScreenOffset() should be called when the event > > constructed and the result should be stored! > > That would mean changing how all the code works, since WidgetToScreenOffset > is used throughout, and on demand. You're saying you want all these changed > too? It's really another bug. We should research this issue later. So, you don't need to mind this. > > Similarly, IsPointerLocked() should be stored to mIsPointerLocked at > > constructor. > > > > > * figure out why DOMMouseScroll events aren't being retargeted to the locked element > > > > It should be retargeted by PresShell::HandleEvent() with the event's > > refPoint and capturing state. > > Meaning it should just work now that I'm setting capture, or that I have to > do something here? Please clarify. Ah, if the mouse event target is always an element during mouse lock, it's not problem. I misunderstood your patch. However, shouldn't the refPoint should be modified by locked point? If some code access it directly (not via DOM event interface), the point may be different than DOM event's information. It seems that the inconsistency causes some bugs.

Hi, I'm one of David's students that are helping with the PointerLock implementation He's away during this week that's why I'm posting this patch We've tried to fix all the review issues: *Spec updates - pointer lock is now requested on the element and cancelled on the document *MouseScroll events are now being retargeted to the locked element *When calculating the center point of the element we are using getBoundingClientRect to get the element dimensions and position on the screen *Mochitests updated Following is a list of the mochitests we have so far: test_childIframe.html test_cssDisplayProperty.html test_doubleLock.html test_escapeKey.html test_infiniteMovement.html test_looseFocusWindow.html test_movementXY.html test_nestedFullScreen.html test_pointerLockPref.html test_removedFromDOM.html test_retargetMouseEvents.html test_screenClientXYConst.html test_suppressSomeMouseEvents.html test_targetOutOfFocus.html test_withoutDOM.html We've tried to cover all the edge cases pointed on the spec Should we add some tests for other use cases as well? **To run the mochitests we need Bug 728893 that allows the mochitest iframe to go fullscreen A few questions regarding pointerlock: Should the element receive pointerlock if display is set to none? What if the element has the display changed to none while in pointerlock mode, should the pointer be unlocked? One of the issues of having the locked element with display set to none is that isn't possible to calculate its center since it won't have a frame and its dimensions would be 0,0 To which coordinates should the pointer be repositioned if the element has display set to none? Shouldn't the center of the element be calculated only once when the pointer gets locked instead of every time time a mouse move happens? Since the element is in fullscreen and its dimensions can't be altered. The only problem I see is if the console is opened or closed during pointer lock, then the center needs to be recalculated Is there a way to listen for a console change? And then recalculate the center? Or is it better to calculate on every mouse move? When the element receives pointer lock we are hiding the cursor and when the pointer is unlocked we are displaying the cursor with a flag of NS_STYLE_CURSOR_AUTO What if the cursor had a different style before pointer lock? Shouldn't we keep a record of what was the cursor style before lock and set the same style when unlocking? Not sure if this is a problem, but when locking the pointer in a dual monitor environment and moving the mouse really fast makes the pointer leave the browser boundaries. Would clipping the pointer to the area of the locked element fix this problem? Thanks a lot for all the reviews :)

Some responses from a spec and or chromium implementation point of view: (In reply to Diogo Golovanevsky Monteiro from comment #92) > A few questions regarding pointerlock: > > Should the element receive pointerlock if display is set to none? As far as I can tell if the element is in the DOM, but display:none it can still bubble mouse events and thus should be a valid target for pointer lock. See comment #84. > What if the element has the display changed to none while in pointerlock > mode, should the pointer be unlocked? Presuming display:none is valid, then lock should remain. > One of the issues of having the locked element with display set to none is > that isn't possible to calculate its center since it won't have a frame and > its dimensions would be 0,0 > To which coordinates should the pointer be repositioned if the element has > display set to none? The element is in the DOM of a document associated with displayed area, right? Center to that area. In Chromium we find the 'render view' and use that, essentially the operating system GUI window that hosts the web page contents. > Shouldn't the center of the element be calculated only once when the pointer > gets locked instead of every time time a mouse move happens? Since the > element is in fullscreen and its dimensions can't be altered. > The only problem I see is if the console is opened or closed during pointer > lock, then the center needs to be recalculated > Is there a way to listen for a console change? And then recalculate the > center? Or is it better to calculate on every mouse move? You may consider that in the future you may support pointer lock outside of full screen, and the window may be moved by other means, e.g. script control. If it can be done efficiently the code would be most robust to constantly re-evaluate the center point. Example code from chromium: render_widget_host_view_gtk.cc ~line 399: "center = host_view->GetWidgetCenter();" http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/renderer_host/render_widget_host_view_gtk.cc&exact_package=chromium&q=%22center%20=%20host_view-%3EGetWidgetCenter();%22&type=cs&l=399 See in same file for GetWidgetCenter code, which caches the center unless the window moves. > When the element receives pointer lock we are hiding the cursor and when the > pointer is unlocked we are displaying the cursor with a flag of > NS_STYLE_CURSOR_AUTO > What if the cursor had a different style before pointer lock? > Shouldn't we keep a record of what was the cursor style before lock and set > the same style when unlocking? Sounds right to restore the previous cursor image. > Not sure if this is a problem, but when locking the pointer in a dual > monitor environment and moving the mouse really fast makes the pointer leave > the browser boundaries. > Would clipping the pointer to the area of the locked element fix this > problem? Clipping seems prudent.

(In reply to scheib from comment #93) > The element is in the DOM of a document associated with displayed area, > right? Not necessarily. The element can be in an iframe, and the iframe itself can be display:none. > Center to that area. In Chromium we find the 'render view' and use > that, essentially the operating system GUI window that hosts the web page > contents. But yeah, this could work. Kind of strange API though.

Comment on attachment 601902 [details] [diff] [review] Patch v5 > /** >+ * Returns true if the element has the pointer locked >+ */ >+ bool IsPointerLock() const { Maybe IsPointerLocked() > static void ExitFullScreen(bool aRunAsync); > >+ >+ virtual void RequestPointerLock(Element* aElement) = 0; >+ static void UnLockPointer(); >+ >+ Extra newline before RequestPointerLock and after UnLockPointer > // Reference to the root document of the branch containing the document > // which requested DOM full-screen mode. > nsWeakPtr nsDocument::sFullScreenRootDoc = nsnull; > >+// Reference to the pointerlock element >+nsRefPtr<Element> nsDocument::sPointerLockElement; >+// Reference to the document which requested pointerlock >+nsRefPtr<nsDocument> nsDocument::sPointerLockDoc = nsnull; Could you use nsWeakPtr for both these. >+nsDocument::RequestPointerLock(Element* aElement) >+{ >+ NS_ASSERTION(aElement, >+ "Must pass non-null element to nsDocument::RequestPointerLock"); >+ if (!aElement) { >+ NS_WARNING("RequestPointerLock(): No Element"); >+ return; >+ } Either assertion or if (!aElement)..., not both. >+ >+ if (aElement == sPointerLockElement) { >+ DispatchPointerLockChange(this); >+ return; >+ } >+ >+ if (!ShouldLockPointer(aElement) || >+ !SetPointerLock(aElement, NS_STYLE_CURSOR_NONE)) { >+ DispatchPointerLockError(this); >+ return; Use 2 space indentation >+nsDocument::ShouldLockPointer(Element* aElement) >+{ >+ // Check if pointer lock pref is enabled >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService("@mozilla.org/preferences-service;1")); >+ bool pointerLockEnabled; >+ prefs->GetBoolPref("full-screen-api.pointer-lock.enabled", &pointerLockEnabled); >+ if (!pointerLockEnabled) { >+ NS_WARNING("ShouldLockPointer(): Pointer Lock pref not enabled"); >+ return false; >+ } Preferences::GetBool("full-screen-api.pointer-lock.enabled") >+ // Check if the element is display:none, etc. >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aElement); >+ FlushPendingNotifications(Flush_Layout); >+ if (!(content && content->GetPrimaryFrame())) { >+ NS_WARNING("ShouldLockPointer(): Element without Frame"); >+ return false; >+ } Ok, so based on the comments this wouldn't be needed. But we do want to check that element is in a document which has docshell and is in the current inner window. So element->OwnerDoc()->GetContainer() should be non-null, and element->OwnerDoc()->GetWindow() and element->OwnerDoc()->GetInnerWindow() should be non-null, and element->OwnerDoc()->GetWindow()->GetCurrentInnerWindow() == element->OwnerDoc()->GetInnerWindow() >+ >+ return true; >+} >+ >+bool >+nsDocument::SetPointerLock(Element* aElement, int aCursorStyle) >+{ >+ if (!GetWindow()) { >+ NS_WARNING("SetPointerLock(): No Window"); >+ return false; >+ } >+ >+ nsIDocShell *docShell = GetWindowInternal()->GetDocShell(); >+ if (!docShell) { >+ NS_WARNING("SetPointerLock(): No DocShell (window already closed?)"); >+ return false; >+ } Ah, some of the checks are done here.. This should probably check also that aElement->OwnerDoc() == this >+nsIDocument::UnLockPointer() >+{ >+ nsDocument::UnLockPointer(); >+} This looks strange. is nsIDocument::UnLockPointer() virtual, and nsDocument::UnLockPointer() static or what? >diff --git a/content/events/public/nsEventStates.h b/content/events/public/nsEventStates.h >--- a/content/events/public/nsEventStates.h >+++ b/content/events/public/nsEventStates.h >@@ -263,16 +263,18 @@ private: > #define NS_EVENT_STATE_MOZ_UI_VALID NS_DEFINE_EVENT_STATE_MACRO(33) > // Content is the full screen element, or a frame containing the > // current full-screen element. > #define NS_EVENT_STATE_FULL_SCREEN NS_DEFINE_EVENT_STATE_MACRO(34) > // Content is an ancestor of the DOM full-screen element. > #define NS_EVENT_STATE_FULL_SCREEN_ANCESTOR NS_DEFINE_EVENT_STATE_MACRO(35) > // Handler for click to play plugin > #define NS_EVENT_STATE_TYPE_CLICK_TO_PLAY NS_DEFINE_EVENT_STATE_MACRO(36) >+// Content is the pointer lock element >+#define NS_EVENT_STATE_POINTER_LOCK NS_DEFINE_EVENT_STATE_MACRO(37) I don't understand why event_state is used for this. Why don't you just add a boolean flag? See nsINode BooleanFlag >+++ b/dom/interfaces/core/nsIDOMDocument.idl >@@ -391,16 +391,22 @@ interface nsIDOMDocument : nsIDOMNode update uuid > >+ >+ readonly attribute nsIDOMHTMLElement mozPointerLockElement; Hmm, why do we restrict this to HTML elements?

Just wanted to check in and find out how things are coming?

(In reply to Martin Best (:mbest) from comment #96) > Just wanted to check in and find out how things are coming? I'll have a new patch up tomorrow. I've been working with smaug and diogo the past few days to address the review comments above.

Nice, thanks for the update.

Thanks for the reviews. Diogo and I have fixed the issues you raised. I've also tried to address all of Masayuki's issues. We're still looking into some random failures of a couple of our tests on Linux, which I suspect to be the fault of the tests; maybe you'll see something we've missed in our implementation. >>+nsIDocument::UnLockPointer() >>+{ >>+ nsDocument::UnLockPointer(); >>+} >This looks strange. >is nsIDocument::UnLockPointer() virtual, and nsDocument::UnLockPointer() >static or what? We've done what the fullscreen api is doing. I'll file a bug on clipping separate to this, since it doesn't affect us much until we move to non-fullscreen pointer lock for elements, and I don't think it needs to block this. If you have a chance to get back to this soon, we'd love to wrap it up ASAP. Thanks.

Comment on attachment 606077 [details] [diff] [review] Patch v6 (carrying forward r=roc for widget changes) Diogo's fixed a few things in the tests since I posted this. I'll get his changes included and update this later today.

More fixes. See comment 99.

FYI, I've filed bugs on mouse pointer clipping support in widget (bug 737097) and doing pointer lock for non-fullscreen elements at some point in the future (bug 737100).

Comment on attachment 606428 [details] [diff] [review] Patch v7 (carrying forward r=roc for widget changes) > // Reference to the root document of the branch containing the document > // which requested DOM full-screen mode. > nsWeakPtr nsDocument::sFullScreenRootDoc = nsnull; > >+// Reference to the pointer locked element. >+nsWeakPtr nsDocument::sPointerLockElement = nsnull; >+ >+// Reference to the document which requested pointer lock. >+nsWeakPtr nsDocument::sPointerLockDoc = nsnull; I don't think = nsnull is actually needed. >+nsDocument::MaybeUnlockPointer(nsIDocument* aDocument) >+{ >+ if (!aDocument) { >+ return; >+ } >+ >+ nsCOMPtr<nsIDocument> pointerLockDoc = do_QueryReferent(sPointerLockDoc); >+ if (!pointerLockDoc) { >+ return; >+ } >+ >+ static_cast<nsDocument*>(pointerLockDoc.get())->UnLockPointer(); >+} What is the aDocument parameter for? >+nsDocument::SetPointerLock(Element* aElement, int aCursorStyle) >+{ >+ nsCOMPtr<nsPIDOMWindow> window = GetWindow(); >+ if (!window) { >+ NS_WARNING("SetPointerLock(): No Window"); >+ return false; >+ } >+ >+ nsIDocShell *docShell = window->GetDocShell(); >+ if (!docShell) { >+ NS_WARNING("SetPointerLock(): No DocShell (window already closed?)"); >+ return false; >+ } >+ >+ nsRefPtr<nsPresContext> presContext; >+ docShell->GetPresContext(getter_AddRefs(presContext)); >+ if (!presContext) { >+ NS_WARNING("SetPointerLock(): Unable to get presContext in \ >+ domWindow->GetDocShell()->GetPresContext()"); >+ return false; >+ } >+ >+ nsCOMPtr<nsIPresShell> shell = presContext->PresShell(); >+ if (!shell) { >+ NS_WARNING("SetPointerLock(): Unable to find presContext->PresShell()"); >+ return false; >+ } You can access presshell and prescontext easier via document. aElement->OwnerDoc()->GetShell()->GetPresContext() (Null check all the Get* methods) >+nsDocument::GetMozPointerLockElement(nsIDOMElement** aPointerLockElement) >+{ >+ NS_ENSURE_ARG_POINTER(aPointerLockElement); >+ *aPointerLockElement = nsnull; >+ nsCOMPtr<Element> pointerLockElement = do_QueryReferent(sPointerLockElement); >+ if (pointerLockElement) { >+ CallQueryInterface(pointerLockElement, aPointerLockElement); >+ } >+ >+ return NS_OK; >+} Shouldn't this do some kind of security check? And if pointerLockElement isn't in the same document or same domain, return null. > nsEventStateManager::DispatchMouseEvent(nsGUIEvent* aEvent, PRUint32 aMessage, > nsIContent* aTargetContent, > nsIContent* aRelatedContent) > { >+ // http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html#methods >+ // "[When the mouse is locked on an element...e]vents that require the concept >+ // of a mouse cursor must not be dispatched (for example: mouseover, mouseout). >+ if (sPointerLockedElement && >+ (aMessage == NS_MOUSELEAVE || >+ aMessage == NS_MOUSEENTER || >+ aMessage == NS_MOUSE_ENTER_SYNTH || >+ aMessage == NS_MOUSE_EXIT_SYNTH)) { >+ mCurrentTargetContent = nsnull; >+ return mPresContext->GetPrimaryFrameFor(sPointerLockedElement); >+ } Hmm, how do we handle drag-and-drop with mouse is locked? >+ return nsIntPoint((aBounds.width/2) + aBounds.x, >+ (innerHeight/2) + (aBounds.y + (aBounds.height - innerHeight))); consistent coding style, please. foo / bar >+ >+ void SetPointerLock( nsIWidget* aWidget, nsIContent* aElement ) ; Extra space after ( and before ) >+ nsIntPoint GetMouseCoords(nsIntRect aBounds); > static void sClickHoldCallback ( nsITimer* aTimer, void* aESM ) ; >+ static void SetLastScreenOffset( nsIntPoint aScreenOffset ) ; ditto. (I know the old code uses extra space but it is wrong style) >+ >+ // The element that is mouse locked, if any. >+ static nsCOMPtr<nsIContent> sPointerLockedElement; Er, hmm, what is this. There is the nsWeakPtr nsDocument::sPointerLockElement Please store the element only in one place, if possible.

One question regarding drag and drop in pointerlock mode: When the element has pointerlock mode, the mouse events(move,down,up,click,scroll) are being retargeted to the pointerlocked element and the mouse events(enter,leave,over,out) are being suppressed. However, nothing is being done for drag events. All the 7 drag events (start,enter,over,leave,end,drag,drop) are not being retargeted or suppressed. How should pointerlock handle drag events? Should drag events be retargeted, suppressed or none?

Spec is silent on this issue, afaict. Vincent?

I believe drag events do not have meaning without a mouse cursor, and attempted to capture that with this in the spec: "Events that require the concept of a mouse cursor must not be dispatched (for example: mouseover, mouseout)."

Yeah, I think we should disable dnd when mouse is locked. IIRC dragservice has a way to disable/suppress it.

Spec updated with drag/drop explicitly listed.

Thanks for the review and subsequent tips on irc. We've addressed the issues you found, updated to trunk, and caught some other issues in the process. >>+nsDocument::SetPointerLock(Element* aElement, int aCursorStyle) >You can access presshell and prescontext easier via document. >aElement->OwnerDoc()->GetShell()->GetPresContext() >(Null check all the Get* methods) We pass null for aElement when unlocking, so I can't do this. I've added comments to make this clear in the code.

Comment on attachment 608110 [details] [diff] [review] Patch v8 (carrying forward r=roc for widget changes) >+NS_IMETHODIMP >+nsDocument::GetMozPointerLockElement(nsIDOMElement** aPointerLockedElement) ... >+ nsCOMPtr<nsIDOMNode> pointerLockedNode = >+ do_QueryInterface(pointerLockedElement); >+ nsresult rv = nsContentUtils::CheckSameOrigin(this, pointerLockedNode.get()); >+ if (NS_FAILED(rv)) { >+ return rv; This should probably return NS_OK; The code in some other domain shouldn't know that there some pointer locked element (although it doesn't know what element). >+ CallQueryInterface(pointerLockedElement, aPointerLockedElement); >+ return NS_OK; Maybe return CallQueryInterface(pointerLockedElement, aPointerLockedElement); >+ if (NS_IS_DRAG_EVENT(aEvent) && sIsPointerLocked) { >+ NS_ASSERTION(sIsPointerLocked, >+ "sIsPointerLocked is true. Drag events should be suppressed when the pointer is locked."); >+ } Make this to be inside #ifdef DEBUG >+++ b/dom/tests/mochitest/pointerlock/Makefile.in >@@ -0,0 +1,78 @@ >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is Mouse Lock. >+# >+# The Initial Developer of the Original Code is Mozilla Foundation. >+# Portions created by the Initial Developer are Copyright (C) 2011 >+# the Initial Developer. All Rights Reserved. >+# >+# Contributor(s): >+# David Humphrey <david.humphrey@senecac.on.ca> >+# Raymond Hung <hung.raymond@gmail.com> >+# Ching Wei Tseng (Steven) <steven_tseng15@hotmail.com> >+# Stanley Tsang <nm486@hotmail.com> >+# James Boelen <james@boelen.ca> >+# Hyungryul Chun <hello.ryul@gmail.com> >+# Matthew Schranz <schranz.m@gmail.com> >+# Abhishek Bhatnagar <abhatnagar1@learn.senecac.on.ca> >+# Qian Xu <qxu26@learn.senecac.on.ca> >+# Diogo Golovanevsky Monteiro <diogo.gmt@gmail.com> >+# >+# Alternatively, the contents of this file may be used under the terms of >+# either the GNU General Public License Version 2 or later (the "GPL"), or >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+# in which case the provisions of the GPL or the LGPL are applicable instead >+# of those above. If you wish to allow use of your version of this file only >+# under the terms of either the GPL or the LGPL, and not to allow others to >+# use your version of this file under the terms of the MPL, indicate your >+# decision by deleting the provisions above and replace them with the notice >+# and other provisions required by the GPL or the LGPL. If you do not delete >+# the provisions above, a recipient may use your version of this file under >+# the terms of any one of the MPL, the GPL or the LGPL. >+# >+# ***** END LICENSE BLOCK ***** Maybe you should use MPL2 license block. r+ for the code, but I'll review still the tests.

Comment on attachment 608110 [details] [diff] [review] Patch v8 (carrying forward r=roc for widget changes) >diff --git a/dom/tests/mochitest/pointerlock/test_childIframe.html b/dom/tests/mochitest/pointerlock/test_childIframe.html >new file mode 100644 >index 0000000..b0a47ed >--- /dev/null >+++ b/dom/tests/mochitest/pointerlock/test_childIframe.html >@@ -0,0 +1,146 @@ >+<!DOCTYPE HTML> >+<html> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=633602 >+--> >+<head> >+ <title>Bug 633602 - file_childIframe.html</title> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"> >+ </script> >+ <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script> >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> >+ <style> >+ #parent, #childDiv, #iframe, #table, #table td { >+ margin: 0; >+ padding: 0; >+ border: none; >+ } >+ #iframe, #table { >+ background-color: red; >+ width: 100%; >+ height: 100%; >+ } >+ #childDiv, #table td { >+ background-color: blue; >+ width: 50%; >+ height: 50%; >+ } >+ </style> >+</head> >+<body> >+ <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=633602"> >+ Mozilla Bug 633602 >+ </a> >+ >+ <div id="parent"> >+ <table id="childTable"> >+ <tr> >+ <td> >+ <iframe id="iframe" src="iframe_differentDOM.html" onload="start();"> >+ </iframe> >+ </td> >+ <td> >+ <div id="childDiv"> >+ </div> >+ </td> >+ </tr> >+ </table> >+ </div> >+ >+ <pre id="test"> >+ <script type="application/javascript"> >+ /* >+ * Test for Bug 633602 >+ * Check if pointer is locked when over a child iframe of >+ * the locked element >+ * Check if pointer is being repositioned back to center of >+ * the locked element even when pointer goes over a child ifame >+ */ >+ >+ SpecialPowers.setBoolPref("full-screen-api.enabled", true); >+ SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", >+ false); This should probably store the old pref values before setting them, and then, after running the test, reset to the old values. >+ var childScrollTest = function() { >+ // XXX: this is still not working... >+ childStats.mouseScroll = true; >+ }; What is this about? Do we not scroll or do we get the MouseScroll event although it is not expected? That explained, and pref handling fixed, r+. (I hope I'm not missing anything major :) )

Thanks for the quick turn around. I've fixed the issues you pointed out. That test comment was just wrong, and should have been removed in a previous iteration; thanks for spotting it. Once you're OK with this, who should look at it next?

Comment on attachment 608845 [details] [diff] [review] Patch v9 (carrying forward r=roc for widget changes) Once you figure out what is causing the failures on Windows, could you post a new patch.

I ran v9 through the try server and found that Windows didn't like to run the tests in an iframe, no matter what we did. So Diogo's reverted the tests back to using a child window instead, and we're all green. I also fixed an issue in the SVG test, and with the fix in bug 735031 having landed, we can now pass that too. This should be ready to go, Olli. No code changes in this patch, just to the tests.

Comment on attachment 610204 [details] [diff] [review] Patch v10 (carrying forward r=roc for widget changes) >+NS_IMETHODIMP >+nsDocument::GetMozPointerLockElement(nsIDOMElement** aPointerLockedElement) >+{ >+ NS_ENSURE_ARG_POINTER(aPointerLockedElement); >+ *aPointerLockedElement = nsnull; >+ nsCOMPtr<Element> pointerLockedElement = >+ do_QueryReferent(nsEventStateManager::sPointerLockedElement); >+ if (!pointerLockedElement) { >+ return NS_OK; >+ } >+ >+ // Make sure pointer locked element is in the same document and domain. >+ nsCOMPtr<nsIDocument> pointerLockedDoc = >+ do_QueryReferent(nsEventStateManager::sPointerLockedDoc); >+ nsDocument* doc = static_cast<nsDocument*>(pointerLockedDoc.get()); >+ if (!doc) { >+ return NS_ERROR_FAILURE; >+ } >+ if (doc != this) { >+ return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; >+ } return NS_OK, not error, in both cases. And the !doc case isn't actually needed. if doc is nsnull, it certainly isn't the same as 'this' I'll read still the tests...

Fixed review issue, otherwise the same.

Discussed with roc, cpearce, smaug on irc, the feeling is to land this so there is time to fix bugs before next Aurora. When someone has a moment, this is good to go (already gone through try server).

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf8929a8115 Also, to make life easier for those checking in patches for you, please follow the directions below for any future patches you submit. Thanks! https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

10.7 OPT Pointer Lock Mochitests are failing: https://tbpl.mozilla.org/php/getParsedLog.php?id=10458043&tree=Mozilla-Inbound&full=1 :( 9785 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_doubleLock.html: Requesting pointer lock on a locked element should dispatch mozpointerlockchange event - got 0, expected 2 9786 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_doubleLock.html: Requesting pointer lock on a locked element should dispatch mozpointerlockchange event - got 0, expected 2 9789 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_escapeKey.html: Pressing Escape key should unlock the pointer 9790 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_escapeKey.html: Pressing Escape key should unlock the pointer 9793 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_locksvgelement.html: Expected SVG elem to become locked. 9794 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_locksvgelement.html: Expected SVG elem to become locked. 9797 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_movementXY.html: mozMovementX and mozMovementY should exist in mouse events objects. 9800 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_movementXY.html: mozMovementX and mozMovementY should exist in mouse events objects. 9805 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Element should have mozRequestPointerLock. 9806 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: pointerlockchange event should fire. 9807 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Should be able to unlock pointer locked element. 9808 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Requested element should be able to lock. 9809 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Document should have mozExitPointerLock 9810 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Document should keep track of correct pointer locked element 9811 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Mouse Event should have mozMovementX. 9812 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Mouse Event should have mozMovementY. 9813 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Element should have mozRequestPointerLock. 9814 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: pointerlockchange event should fire. 9815 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Should be able to unlock pointer locked element. 9816 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Requested element should be able to lock. 9817 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Document should have mozExitPointerLock 9818 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Document should keep track of correct pointer locked element 9819 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Mouse Event should have mozMovementX. 9820 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerlock-api.html: Mouse Event should have mozMovementY. 9823 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerLockPref.html: Element should be able to lock the pointer if pointer-lock pref is set to TRUE 9824 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerLockPref.html: Element should NOT be able to lock the pointer if pointer-lock pref is set to FALSE 9825 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerLockPref.html: Element should be able to lock the pointer if pointer-lock pref is set to TRUE 9826 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_pointerLockPref.html: Element should NOT be able to lock the pointer if pointer-lock pref is set to FALSE 9834 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mousemove event. 9835 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mousedown event. 9836 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mouseup event. 9837 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive click event. 9838 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive DOMMouseScroll event. 9844 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mousemove event. 9845 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mousedown event. 9846 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive mouseup event. 9847 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive click event. 9848 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_retargetMouseEvents.html: Parent should receive DOMMouseScroll event. 9851 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_suppressSomeMouseEvents.html: expected mouse to be locked, but wasn't. 9860 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_suppressSomeMouseEvents.html: expected mouse to be locked, but wasn't.

I notice that the fullscreen tests have gotten a special case for Lion since we did our child-window-harness: http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_fullscreen-api.html?force=1#76 // We'll wait for the window to load, then make sure our window is refocused // before starting the test, which will get kicked off on "focus". // This ensures that we're essentially back on the primary "desktop" on // OS X Lion before we run the test. Probably I need the same, as these failures make no sense for what they are. I'll try to find a 10.7 machine to test.

Alas, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/04aa64acdc65 The 10.7 failures you already know about. The things that could have been from your push neighbors, or could have been you, are https://tbpl.mozilla.org/php/getParsedLog.php?id=10457011&tree=Mozilla-Inbound (that was bug 718316, but that stopped in February and on this push it returned as permaorange) https://tbpl.mozilla.org/php/getParsedLog.php?id=10458886&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=10459352&tree=Mozilla-Inbound

(In reply to David Humphrey (:humph) from comment #116) > Created attachment 610267 [details] [diff] [review] > Patch v10a (r=roc for widget changes, r=smaug for rest) > > Fixed review issue, otherwise the same. Looks like this patch needs to also rev the uuids of classes that inherit from nsIDOMElement and nsIDOMDocument. There's lots of such clases, but you can do this automatically with the update-uuids tool: http://people.mozilla.com/~sfink/uploads/update-uuids `update-uuids dom nsIDOMElemnet nsIDOMDocument` should do the trick.

(In reply to Chris Pearce (:cpearce) from comment #122) > (In reply to David Humphrey (:humph) from comment #116) > > Created attachment 610267 [details] [diff] [review] > > Patch v10a (r=roc for widget changes, r=smaug for rest) > > > > Fixed review issue, otherwise the same. > > Looks like this patch needs to also rev the uuids of classes that inherit > from nsIDOMElement and nsIDOMDocument. There's lots of such clases, but you > can do this automatically with the update-uuids tool: > http://people.mozilla.com/~sfink/uploads/update-uuids > > `update-uuids dom nsIDOMElemnet nsIDOMDocument` should do the trick. $ ./update-uuids dom nsIDOMElemnet nsIDOMDocument uuids to update: Scanned files did not contain uuid for nsIDOMElemnet I tried doing it in . too, and got the same result. Any tips?

Ah, I'm too trusting, copying even your spelling mistakes :) Worked when I spelled Element correctly. Thanks.

I've fixed the UUIDs, updated to trunk, and reworked the tests for Lion. Previously the tests would all fail on Lion because I wasn't doing the timeouts and waitForFocus dance that's necessary to get back to the main desktop and browser window from the fullscreen desktop. Now they pass...most of the time. I'm still not convinced that these tests will ever pass on Lion 100% of the time (they *always* pass on Lion locally, all pass when run individually, and sometimes pass on try when run as a group), much as the fullscreen tests have intermittent failures. I'm to the point where I need some help figuring out what to do. Should we special case Lion like we do with XP on fullscreen tests and just todo() until these bugs get sorted out? I've written and rewritten things so many different ways, and Lion's fullscreen craziness is making this impossible. I'm out of ideas and running low on drive to continue chasing my tail.

Paul, see comment 125, maybe you have thoughts, too.

The FS tests are (afaik) running pretty well, with the exception of the plugin test (but I want to blame plugins). The only difference that I noticed was I did waitForFocus(waitForFocus(testWin.start, testWin), window) to bounce focus back & forth. I don't remember if _that_ was some part of making it work or not but I wasn't going to mess with it anymore. I got to the same point and was *this* close to disabling the tests on Lion if the last ditch timeout didn't work. My vote is just todo them. For all intents & purposes Lion FS isn't really special - it's just a bigger window.

(In reply to Paul O'Shannessy [:zpao] from comment #127) > The FS tests are (afaik) running pretty well, with the exception of the > plugin test (but I want to blame plugins). Really? What about bugs 690989, 726540, 730516, 730583, 738275, 738764? > The only difference that I > noticed was I did waitForFocus(waitForFocus(testWin.start, testWin), window) > to bounce focus back & forth. I don't remember if _that_ was some part of > making it work or not but I wasn't going to mess with it anymore. I did that earlier, and interestingly, it made it worse on non-Mac, and especially Linux. It ends up causing cases where the waitForFocus calls get unbalanced, or tests get run more than once: [POINTERLOCK] Starting file_childIframe.html 9820 INFO TEST-INFO | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | must wait for focus 9821 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: Pointer should be locked even when pointer hovers over a child iframe 9822 INFO TEST-PASS | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: MovementX of first move to childDiv should be equal to movementX of second move to child div - false should equal false 9823 INFO TEST-PASS | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: MovementY of first move to childDiv should be equal to movementY of second move to child div - false should equal false [POINTERLOCK] Finishing file_childIframe.html 9824 INFO TEST-INFO | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | must wait for focus 9825 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: Pointer should be locked even when pointer hovers over a child iframe 9826 INFO TEST-PASS | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: MovementX of first move to childDiv should be equal to movementX of second move to child div - false should equal false 9827 INFO TEST-PASS | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | file_childIframe.html: MovementY of first move to childDiv should be equal to movementY of second move to child div - false should equal false [POINTERLOCK] Finishing file_childIframe.html That test shouldn't have been run twice, nor should it have failed. Or: 9897 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/pointerlock/test_pointerlock-api.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run. Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0 > I got to the same point and was *this* close to disabling the tests on Lion > if the last ditch timeout didn't work. My vote is just todo them. For all > intents & purposes Lion FS isn't really special - it's just a bigger window. If Olli et al are OK with this, that is what I'd like to do as well, perhaps with a bug filed to investigate how to re-enable them when this is sorted out. I really don't want to maintain this patch for much longer when it's only test env failures I'm chasing and we could be testing it for real cases in nightlies.

(In reply to David Humphrey (:humph) from comment #128) > (In reply to Paul O'Shannessy [:zpao] from comment #127) > > The FS tests are (afaik) running pretty well, with the exception of the > > plugin test (but I want to blame plugins). > > Really? Fair. Those tests didn't have a good track record before I landed my stuff. I meant that apart from the one exception, I haven't seen any Lion-specific issues pop up and I don't think I made anything worse (not that status quo should necessarily be a goal, but I didn't go in looking to fix all orange). > What about bugs 690989 Ok, that one has been an issue for longer than Lion FS and AFAICT it hasn't failed on Lion. > 726540 Linux only(?), predates. > 730516 predates > 730583 predates, but it does look like it picked up for a bit after Lion FS landed, then stopped again > 738275, 738764? Those do look to be new, though luckily infrequent. They don't seem to be happening on 10.7 (but 10.7 debug M1 is hidden so maybe they are) (Sorry for getting defensive!) > > The only difference that I > > noticed was I did waitForFocus(waitForFocus(testWin.start, testWin), window) > > to bounce focus back & forth. I don't remember if _that_ was some part of > > making it work or not but I wasn't going to mess with it anymore. > > I did that earlier, and interestingly, it made it worse on non-Mac, and > especially Linux. It ends up causing cases where the waitForFocus calls get > unbalanced, or tests get run more than once: Just to make sure, did you also remove your usage of addLoadEvent from pointerlock_utils.js? Both of those problems seem like they could be connected to that. > I really don't want to maintain this patch for much longer when it's only > test env failures I'm chasing and we could be testing it for real cases in > nightlies. I completely agree. I think we need to be pragmatic and just land things like this.

(In reply to Paul O'Shannessy [:zpao] from comment #129) > (In reply to David Humphrey (:humph) from comment #128) > > (In reply to Paul O'Shannessy [:zpao] from comment #127) > > > The FS tests are (afaik) running pretty well, with the exception of the > > > plugin test (but I want to blame plugins). NOTE: I'm talking about fullscreen in general vs. Lion fullscreen in particular. > > What about bugs 690989 > Ok, that one has been an issue for longer than Lion FS and AFAICT it hasn't > failed on Lion. > > 726540 > Linux only(?), predates. > > 730516 > predates > > 730583 > predates, but it does look like it picked up for a bit after Lion FS landed, > then stopped again > > 738275, 738764? > Those do look to be new, though luckily infrequent. They don't seem to be > happening on 10.7 (but 10.7 debug M1 is hidden so maybe they are) My point is that fullscreen is turning out to be hard to test dependably. > > (Sorry for getting defensive!) No, I understand, and I apologize for being so frustrated. I'm not blaming you for sure, nor anyone else. I'm just sick of these tests and looking for solutions. > > > The only difference that I > > > noticed was I did waitForFocus(waitForFocus(testWin.start, testWin), window) > > > to bounce focus back & forth. I don't remember if _that_ was some part of > > > making it work or not but I wasn't going to mess with it anymore. > > > > I did that earlier, and interestingly, it made it worse on non-Mac, and > > especially Linux. It ends up causing cases where the waitForFocus calls get > > unbalanced, or tests get run more than once: > > Just to make sure, did you also remove your usage of addLoadEvent from > pointerlock_utils.js? Both of those problems seem like they could be > connected to that. A good question, and yes, I removed the load event when changing the flow to do what you did. It didn't help. > > I really don't want to maintain this patch for much longer when it's only > > test env failures I'm chasing and we could be testing it for real cases in > > nightlies. > > I completely agree. I think we need to be pragmatic and just land things > like this. +1

I'd rather not disable all fullscreen tests, we're liable to regress something if we do. We need to do something to make them more reliable, and I don't want to block Humph's work due to crappy tests. Are the failures at least in part caused by our requirement that fullscreen requests are only granted in the currently focused document? Then we wouldn't need all that waitForFocus junk. Perhaps we'll increase our reliability if we disable that requirement while running the fullscreen tests (via a pref). We should then create a new test which tests only and specifically that requirement. This should isolate our random failures to that new test.

(In reply to Chris Pearce (:cpearce) from comment #131) > I'd rather not disable all fullscreen tests, we're liable to regress > something if we do. Absolutely, we can't do that. > We need to do something to make them more reliable, and I don't want to > block Humph's work due to crappy tests. > > Are the failures at least in part caused by our requirement that fullscreen > requests are only granted in the currently focused document? Then we > wouldn't need all that waitForFocus junk. Perhaps we'll increase our > reliability if we disable that requirement while running the fullscreen > tests (via a pref). We should then create a new test which tests only and > specifically that requirement. This should isolate our random failures to > that new test. What I want to do is follow what we've done for WinXP with fullscreen tests. From http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_fullscreen-api.html if (isWinXP) { todo(false, "Can't reliably run full-screen tests on Windows XP due to bug 704010"); SimpleTest.finish(); return; } I'm suggesting: 1) File a new bug, "Investigate Pointer Lock + Fullscreen failures on Lion" 2) Do an isOSXLion check and bail with a todo() listing the bug from 1) 3) Land this with all other platforms/tests enabled 4) Continue to figure out what is causing issues on Lion in that other bug from 1) Is something like this possible? Trying to solve 4) and not land this means I have to carry the patch when it works everywhere else (and works on Lion too), but in the Lion-test case.

Comment on attachment 612416 [details] [diff] [review] Patch v10j (r=roc, smaug) The problems with fullscreen on Lion shouldn't block this. We may need to mark some tests todo() for now, and enable them properly when fullscreen on Lion works ok.

...but someone really needs to work on to get fullscreen tests reliable. The current situation isn't good.

Ok, let's run with Humph's suggested plan above.

I've added a special case for OS X Lion so tests won't fail (they are now todo() like Windows XP). I've filed bug 744125 to get them fixed/enabled. This patch looks good on try (none of the orange there is related to this) - https://tbpl.mozilla.org/?tree=Try&rev=457656a63a6b This is ready to land again, but I'll give Olli the benefit of agreeing/disagreeing before I request it.

Olli, the relevant change here is the special case in nextTest(): + function nextTest() { + if (isWinXP) { + todo(false, "Can't reliably run full-screen tests on Windows XP due to bug 704010"); + SimpleTest.finish(); + return; + } + if (isOSXLion) { + todo(false, "Can't reliably run full-screen tests on OS X Lion, see bug 744125"); + SimpleTest.finish(); + return; + } The rest is updates to trunk, which are breaking interdiff for some reason.

Thank you. When someone has a moment, this needs to land.

Round 2! https://hg.mozilla.org/integration/mozilla-inbound/rev/637a5d7228be

Backed out whole merge for bustage per Yoric (Bug 743574): https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321

Disregard that; PEBKAC. Did not get backed out. I misread TBPL.

Congrats guys! :-) https://hg.mozilla.org/mozilla-central/rev/637a5d7228be

I've updated the docs at https://developer.mozilla.org/en/API/Pointer_Lock_API to reflect the new spec and implementation. Someone more familiar with the new MDN styling might want to review and make sure they're happy with what I've done.

In full screen the following demo displays on a quarter of the screen. Would this be expected (demo issue or Firefox known issue)? http://media.tojicode.com/q3bsp/?tesselate=2 Is the demo supposed to work now in F14 with mouse lock following feature implementation? Should double clicking with the demo in full screen revert to mouse movement?

Is this supposed to be working under Linux on the 2012-04-24 build of Aurora 14.0a2? I've tried various demos and the demo fullscreens properly (filling one of my two monitors) but the mouse never locks.

(In reply to Stephan Sokolow from comment #145) > Is this supposed to be working under Linux on the 2012-04-24 build of Aurora > 14.0a2? > > I've tried various demos and the demo fullscreens properly (filling one of > my two monitors) but the mouse never locks. Try http://media.tojicode.com/q3bsp/ which is working now (was broken due to demo using old API). I suspect that most if not all of the current demos out there are broken, given the move from callbacks to events, and other API changes. However, it does work on Linux, yes. Please file new bugs if you find this to be incorrect.

(In reply to David Humphrey (:humph) from comment #146) > (In reply to Stephan Sokolow from comment #145) > > Is this supposed to be working under Linux on the 2012-04-24 build of Aurora > > 14.0a2? > > > > I've tried various demos and the demo fullscreens properly (filling one of > > my two monitors) but the mouse never locks. > > Try http://media.tojicode.com/q3bsp/ which is working now (was broken due to > demo using old API). I suspect that most if not all of the current demos > out there are broken, given the move from callbacks to events, and other API > changes. However, it does work on Linux, yes. Please file new bugs if you > find this to be incorrect. Ahh. That's working. The framerate's terrible, but I think that's just the addon memory leaks I already knew about and haven't had time to track down.

Not blocking Basecamp - we could ship the release without this feature.

Note that when trying http://media.tojicode.com/q3bsp/, you have to click on fullscreen. I marked this as supported since Firefox 14 on https://developer.mozilla.org/en-US/docs/API/Pointer_Lock_API instead of "targetting" and unsupported.

FYI, we have very little test coverage of this code. Tests are currently disabled on every platform except one rev of OSX, and those tests will be disabled soon (bug 931445, bug 1121487).

Martin, who handles games work these days? We need to get pointer lock test coverage on their radar.

Should anyone have cycles to build out tests, it would be great to have contributions to https://github.com/w3c/web-platform-tests/tree/master/pointerlock.

Jim, I'll get this on our roadmap. Vincent, I'll be sure to point them towards that repo.