Changes

Jump to: navigation, search

Implementing the Mouse Lock API in Firefox

12,000 bytes added, 00:09, 15 February 2012
Review Issues
## [http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Reference/Quartz_Services_Ref/Reference/reference.html CGAssociateMouseAndMouseCursorPosition and CGGetLastMouseDelta] on OS X
## [http://www.x.org/archive/X11R6.8.2/doc/XGrabPointer.3.html XGrabPointer] on Linux
 
====Review Issues====
 
* <s>Rewrite DOM node removal unlocking logic to use nsIMutatationObserver</s> humph, diogogmt
* <s>Space, bracing, tabs, etc style nits</s> humph
* <s>clientX, clientY, screenX, screenY values should follow spec as per [https://bugzilla.mozilla.org/show_bug.cgi?id=633602#c55 scheib's comment]</s> diogogmt
* <s>Confirm |aEvent->lastRefPoint = nsIntPoint(bounds.width/2, bounds.height/2);| that / 2 is right</s> diogogmt
* <s>"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." Probably wants a test for this case, too.</s> diogogmt
* <s>"nsDOMMouseLockable::ShouldLock...QI to nsINode and check IsInDoc()"</s> diogogmt
* <s>"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));"</s> - humph
* <s>"nsDOMMouseLockable looks like it should be cycle collectable"</s> humph
* <s>Break .pointer out of Navigator, and add nsIDOMMozNavigatorPointer (not sure if this name is best) as a new interface to Navigator, similar to nsIDOMMozNavigatorBattery.</s> - humph, diogogmt
* <s>Rename for moz* prefix. The API should be probably prefixed. So, navigator.mozPointer and all the interfaces should start with nsIDOMMoz. Similar to what webkit* is doing:</s> - humph
** <s>navigator.webkitPointer.unlock();</s>
** <s>navigator.webkitPointer.lock();</s>
** <s>navigator.webkitPointer.isLocked();</s>
** <s>document.body.addEventListener("webkitpointerlocklost", ...</s>
** <s>mouseMoveEvent.webkitMovementX</s>
** <s>mouseMoveEvent.webkitMovementY</s>
* Follow discussion unfolding here for other changes: http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/1558.html
* Finish tests, updated to changes above.
* <s>Update patch to trunk</s> - humph
* 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()
 
* Maybe use nsStubMutationObserver instead of nsIMutationObserver?
 
* Extensions to the Document Interface. Add pointerLockElement attribute and exitPointerLock method - diogogmt
 
* Update Element interface to have requestPointerLock - diogogmt
 
* <s>Dispatch pointerlockchange or pointerlockerror events instead of firing a callback when pointer gets locked</s> - diogogmt
 
* Update mochitests to use new pointerlock API
 
* Add expect number of tests to mochitests
* Update patch to trunk
===Tests===
|<s>islocked() returns true if mouse is locked, false if not locked</s>
|<strong>JSilver999</strong>
|'''test_fullscreenfile_fullscreen.html'''
|-
|7
|<s>lock(target) expects a DOM element, and takes two optional callbacks: successcallback, failurecallback.</s>
|<strong>JSilver999</strong>
|'''test_fullscreenfile_fullscreen.html'''
|-
|8
|<s>lock() should return immediately and call callbacks when lock succeeds or fails</s>
|<strong>JSilver999</strong>
|'''test_fullscreenfile_fullscreen.html'''
|-
|9
|<s>"Mouse lock must succeed only if the window is in focus"|'''johnno'''</s>|'''test_UserAgentIsActive.html'''
|-
|10
|<s>"Mouse lock must succeed only if...the user-agent is the active application of the operating system"</s>
|'''johnno'''
|'''test_UserAgentIsActive.html'''
|-
|11
|<s>"The unlock method cancels the mouse lock state"</s>
|'''abhatnagar1'''
|No test written for this case specifically. but '''test_fullscreenfile_fullscreen.html''' should cover covers it.
|-
|22
# Prefer <code>i++;</code> to <code>i+=1;</code>
# Many tests require the harness and focus, ensure that those tests are correctly written ie:
#* For Harness: See [[#Using the Test Harness|here]]#* For Focus: Ensure See [[#Waiting For Focus]] One or more tests have to deal with switching the test resembles lock between multiple elements, and the following format-<pre>&lt;!-- snip --&gt;&lt;body onload="runTests();"&gt;[[Changes to Fullscreen Unlock|spec is changing]] on that front.
&lt;!-- other tags that ====New Test Reviews [Updated 1/19/2012]====This is a new list of test. The following tests will need to confirm to the new prefixes in accordance to Patchv3 of [https://bugzilla.mozilla.org/show_bug.cgi?id=633602 Bug 633602]. Please add your name beside each test(s) you may need go here --&gt;are assessing/reviewing/fixing/peer reviewing.Once the cause for rejection is clear, indicate it by placing an X in any of the columns and optionally filling out the comments column. Indicate the action taken to resolve the issue in the action taken column. Example actions include:# '''DELETED'''# '''FIXED'''
&lt;script&gt;Other actions are also acceptable where appropriate. function runTests() { SimpleTestOnce fixed, please pull request to rhung's [https://github.com/rhung/mozilla-central/tree/mouselock-tests/dom/tests/mochitest/mouselockrhung's mouselock-tests] branch and indicate that you've done so in the pull requested column.waitForExplicitFinish(); SimpleTestEnsure that you add '''[POST-SUBMIT]''' to the name of your pull request.waitForFocus(function() {
// {| border=1 style="border: 1px solid darkgray;"| Num| Name| Primary Reviewer| Timing issues?| Needs Harness?| Needs focus?| Formatting issues?| Other? (Indicate in comments)| Peer Reviewers| Peer reviewed?| Action Taken| Pull requested?| bgcolor="red" style="text-align:center;" | Updated with Patchv3 | Comments|- | colspan=14 bgcolor="yellow" style="text-align:center;" | '''Ensure you have actually tested your test before sending a pull request'''|-| 1| test_FullScreenHarness.html| Anachid| X (no action)| X (no action)| X (no action)| X (no action)| -| | | '''FIXED'''| | X| |-| 2| file_DOMtree.html| Anachid| X (fixed)| X (fixed)| X (fixed)| X (no action)| -| | | '''FIXED''' | | X| |-| 3| file_nestedFullScreen.html| Anachid| X (fixed)| X (fixed)| X (fixed)| X (no action)| -| | | '''FIXED''' | | X| |-| 4| file_differentDOM.html| Anachid| X (no action)| X (no action)| X (no action)| X (no action)| -| | | '''FIXED''' | | X| |-| 5| file_fullscreen.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| Test code goes hereneeds to completely re-done. Cases like <br>* ok(false, "Mouse locked without fullscreen mode"); will always fail!* Flow of the test will causes scope problems.* Currently fixing.|-| 6| file_doubleLockCallBack.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 7| file_unlockOnUnload.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 8| file_exitMouseLockOnLoseFocus.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 9| test_mousePos.html| rhung||||| Doesn't seem like it can be done via mochitest||| '''REMOVED'''||| As mentioned in the previous comments, synthesizeMouse cannot be moved beyond the browser. It might need to be a litmus test.|-| 10| file_mouselocklost.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| Removed some test that were repeating. Otherwise, it works|-| 11| file_movementXY.html| rhung| X| X| X| X| -| | | '''Fixing''' | | X||-| 12| file_targetOutOfFocus.html| rhung| X| X| X| X (fixed)| -| | | '''FIXED''' | | X| Minor changes, mostly styling.|-| 13| test_mouseLockable.html| rhung| X| X| X| X| -| | | '''FIXED & RENAMED''' | | X| Updated to test_MozPointerLock.html|-| 14| file_MouseEvents.html| rhung| X| X| X| X| Leak issue might be platform specific.| | | '''FIXED?''' | | X| Works without chance of hanging or random success and failures.|-| 15| file_limitlessScroll.html| rhung|||||| | | '''MERGED WITH file_movementXY''' | |||-| 16| file_defaultUnlock.html| rhung|||||| | | '''FIXED''' | | X||-| 17| file_userPref.html| rhung|||||| | | '''FIXED''' || X||-| 18| file_constantXY.html| rhung|||||| | | '''FIXING''' || X| Needs proper error messages, perhaps formatting issues. Test runs fine though.|-|}
});
}
&lt;/script&gt;
&lt;/body&gt;
&lt;!-- /snip --&gt;</pre>
One or more tests New Tests (not sure if it will work on mochitest, and the test will possibly fail as dave is working on this patch)# When there is a parent element with multiple child element, have a test to deal with switching check that all mouse events fired inside the lock between multiple elements, child element are suppressed and is captured by the [[Changes parent element. (this may also include the scenario where there is an iframe in the parent element)# A Test to check that repeated move of the mouse to Fullscreen Unlock|spec is changing]] on a certain direction that frontwill exceed the screen size does not break the mouselock (this may conflict with file_movementXY.html and we may need to add more test cases)
====Changes To Be Applied To Submitted Tests[Deprecated]====
The following table is used to maintain a small note on the task for each test. It will include note from the review by Mozilla and issue we overlooked.
|}
====Remaining Test Reviews[Deprecated]====
The following tests need investigation. Please add your name beside each test(s) you are assessing/reviewing/fixing/peer reviewing.
Once the cause for rejection is clear, indicate it by placing an X in any of the columns and optionally filling out the comments column. Indicate the action taken to resolve the issue in the action taken column. Example actions include:
| Action Taken
| Pull requested?
| bgcolor="red" style="text-align:center;" | Updated with Patchv3
| Comments
|-
| colspan=13 14 bgcolor="yellow" style="text-align:center;" | '''Ensure you have actually tested your test before sending a pull request'''
|-
|-
|<s>106</s>|<s>test_UserAgentIsActive</s>|<s>diogogmt</s>
|
|<s>X||X|X|northwind</s>
|
|<s>X</s>
|<s>X</s>
|<s>northwind</s>
|
|<s><b>IMPOSSIBLE IN MOCHITEST</b></s>
|
|
|<s>
* northWind Comments prior to diogo re-writing user agent test:
** navigator.userAgent does not indicate focus status of browser.
** test should be testing for full screen if user agent is in focus then testing that full screen is unavailable when user agent is not in focus.
** test uses tabs, should use spaces.
</s>
|-
|<s>109</s>|<s>file_doubleLockCallBack.html</s>|<s>northWind</s>
|
|X
|X
|
|<s>rhung</s>
|X
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|X
|
|* <s>For Peer Reviewer: https://github.com/rhung/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/mouselock/file_doubleLockCallBack.html</s>
|-
|<s>110</s>|<s>test_exitMouselockOnLoseFocus.html</s>||||||<s>diogogmt</s>
|
|<s>X</s>
|<s>X</s>
|<s>X</s>
|
|<s>northWind</s>
|<s>X</s>
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|114
|test_mousePos.html
|diogogmt
|
|
|
|
|A summary of the test: *Move the mouse outside the browser boundaries. *Move the mouse back inside the browser. *Checks if movementXY were set to zero, since the last refPoint is unknownThe problem is that synthesizeMouse doesn't move the mouse outside the boundaries of the specified element. If the X/Y coords passed to synthesizeMouse are bigger than the X/Y of the element receiving the event, it won't move the cursor. I'm logging all the screen/client/movementXY values, and if the coords passed to synthesizeMouse are bigger than the boundaries of the element receiving the mousemove, the test will break, since the mouse is never moved.   [http://pastebin.com/MhP62BtC code]  
|-
|<s>115</s>|<s>file_mouselocklost.html</s>|||<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|<s><b>PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|<s>Original file didn't need any changes. Passed all test criterias</s>
<s>Awaiting peer review to cross it off the list</s>
|-
|<s>119</s>|<s>file_fullscreen.html</s>|<s>northWind</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>rhung</s>
|
||<s><b>FIXED, AWAITING PEER REVIEWREVIEWED, AND ADDED TO REPOSITORY</b></s>
|X
|
* For Peer Reviewer: https:|<s>Lines</s> <s>49</githubs>, <s>66</s><s>, and 76 are too long.com76 in particular.</rhung/mozilla-centrals> <s>Needs comment at the beginning to describe what the test does.</blob/mouselock-tests/dom/tests/mochitest/mouselock/file_fullscreen.htmls>
|-
|<s>107</s>|file_constantxy<s>file_screenXYclientXYConst.html</s>|<s>diogogmt</s>|<s>X</s>|<s>X</s>
|
|
|
|<s>anachid</s>
|
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|<s>[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests-constantXY/dom/tests/mochitest/mouselock/file_constantXY.html file_constantXY]</s>|<bs>FIXING[https://github.com/diogogmt/mozilla-central/tree/mouselock-tests-constantXY branch]</bs>||<s>* Anachid (formatting review on file commit version '''1f5fa3e0a5f7730f33eb5d3f7c51105fac4cb93d'''):</s><s>** Line 8 exceeds 80 character limit by 7 characters.</s><s>** Remove line 99 [ console.log("start"); ]</s><s>** Line 100 and 103 could be re-done to document.getElementById("div") instead of having a div variable, thereby removing variable space and risk of a bug</s><s>** Extra line breaks (should be removed) on line 74,77,78</s>
|-
|108
|file_defaultUnlock.html
|diogogmt
|
|
|
|-
|<s>103</s>|<s>file_DOMtree.html</s>||<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|
|'''FIXING'''
|
|
|
|X
|
|X
|
|
|northWind
|
|'''PEER REVIEWED, SEE COMMENTS'''
|
|
|'''FIXED, AWAITING PEER REVIEW'''
|
|
* northWind:
** hangs intermittently, most likely requires focus, see [[#Waiting For Focus|here]]
** 39,41: not required, handled by harness
** 51: remove line, setup being called at better time @343
** 60: should use === for identity as opposed to ==
** 60: should add document.mozFullScreen && to condition
** 61, 69: needs failure callback with is(false, ...) and SimpleTest.finish()
** 68: should add && document.mozFullScreenElement === outer to condition
** 68: whitespace at end of line
** <s>78: looked at this briefly, are we sure that synthMs will cause the cursor to cross over the 'one' div?</s>
** 172: Col 45: "the" should be "a Left"
** 183-214: Why not testing for mousemove?
|-
|116
|file_movementfile_movementXY.html|||diogogmt
|
|
|
|
| mjschranz
|
|<b>REVIEWED, READY FOR PULL REQUEST</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_movementXY.html file_movementXY]
* mjschranz
** As far as I can tell, there doesn't appear to be any styling errors. The tests run just fine and based on what I understand about it and what is listed in the comments the tests appear to function as intended.
|-
|<s>120</s>|<s>file_syntheticMouseEvent.html</s>||<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|
|
|
|<s>Merged with MouseEvents test</s>
|-
|105
|file_targetOutOfFocus.html
|diogogmt
|
|
|
|
| mjschranz
|
|<b>REVIEWED, SEE COMMENTS</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_targetOutOfFocus.html file_targetOutOfFocus]|* mjschranz|** Line 41: the word focus is surrounded by single quotation marks, should be double ( "" ).** No other styling issues seen. Tests run and appear to be functioning as per the specs.
|-
|118
|
|
|X
|
|X
|
|
|121
|test_mouseLockable.html
(rename to file_mozPointerLock.html)
|
|
|X
|X
|X
|X
|
|
|
|
|-
|122
|file_nestedFullScreen.html
|diogogmt
|
|
|
|X
|
| mjschranz
|
|<b>REVIEWED, SEE COMMENTS</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_nestedFullScreen.html file_nestedFullScreen]
* mjschranz
** Line 48: isLocked method call is missing opening/closing brackets. The test still passes when FullScreenHarness is run however we obviously need to fix that and ensure it's still locked as intended.
** No other styling issues seen. Fix previous error and double check all tests still pass when harness is run.
|-
|122<s>123</s>|file_nestedFullScreen<s>file_unlockOnUnload.html</s>|<s>northWind</s>
|
|
|
|X
|
|<s>rhung</s>
|
|<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|
|
|-
|<s>120</s>
|<s>file_differentDOM.html</s>
|<s>diogogmt</s>
|
|
|
|-
|123
|file_unlockOnUnload.html
|northWind
|
|
|<s>rhung</s>
|
|X<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|rhung
|
|'''NEEDS CHANGES READ COMMENT'''
|
|Lines 49, 66, and 76 are too long. 76 in particular. Needs comment at the beginning to describe what the test does.
|-
|}
<!--
====Test Reviews[DEPRECATED]====
# <s>test_syntheticMouseEvent.html</s>
# <s>test_userPref.html</s> - mjschranz
-->
 
====Waiting For Focus====
Many tests will intermittently hang if they don't wait for focus. To have a test wait for focus, ensure that the test incorporates the following template:
<pre>
&lt;!-- snip --&gt;
&lt;body onload="runTests();"&gt;
 
&lt;!-- other tags that you may need go here --&gt;
 
&lt;script&gt;
function runTests() {
SimpleTest.waitForExplicitFinish();
SimpleTest.waitForFocus(function() {
 
// Test code goes here
 
});
}
&lt;/script&gt;
&lt;/body&gt;
&lt;!-- /snip --&gt;</pre>
 
 
====Using the Test Harness====
1
edit

Navigation menu