Pointer Lock implementation is being done in phases. This bug represents the work needed to correctly handle pointer lock target elements that are removed from the DOM tree.
Created attachment 152106 [details] Patch
Comment on attachment 152106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152106&action=review > Source/WebCore/page/PointerLockController.cpp:52 > + if (m_documentOfRemovedElementWhileWaitingForUnlock) > + return; Does this line mean that if you removed a locked element from a DOM, then checked the webkitPointerLockElement which would be empty (implying nothing was locked), and then immediately tried to lock some other element, then locking would fail silently?
Comment on attachment 152106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152106&action=review > Source/WebCore/page/PointerLockController.cpp:51 > + if (m_documentOfRemovedElementWhileWaitingForUnlock) r-, I forgot to sent an error event here. >> Source/WebCore/page/PointerLockController.cpp:52 >> + return; > > Does this line mean that if you removed a locked element from a DOM, then checked the webkitPointerLockElement which would be empty (implying nothing was locked), and then immediately tried to lock some other element, then locking would fail silently? Yes. The element is removed form DOM, it must be unlocked and not have mouse events sent to it. However, the user agent must still release the system cursor. In the case of chromium that will be an asynchronous operation, and a lock request will be rejected. We could ignore this logic in WebKit, but I believe it's preferable to handle it here to reduce the chance or logic error outside of this code. A reliable application would do the following: - lock pointer - remove element from dom - handle pointer lock change event - lock pointer This patch is attentive to not sent the pointer lock change until the unlock is complete. Applications will have other scenarios as well when the pointerLockElement is null but requesting pointer lock is rejected (users not opting in to a lock, attempting multiple locks before any are accepted). Thus, I think they'll need to be made robust to rejected requests.
Created attachment 152291 [details] Patch
(In reply to comment #3) > (From update of attachment 152106 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152106&action=review > > > Source/WebCore/page/PointerLockController.cpp:51 > > + if (m_documentOfRemovedElementWhileWaitingForUnlock) > > r-, I forgot to sent an error event here. Fixed and included in test that error sent. > > >> Source/WebCore/page/PointerLockController.cpp:52 > >> + return; > > > > Does this line mean that if you removed a locked element from a DOM, then checked the webkitPointerLockElement which would be empty (implying nothing was locked), and then immediately tried to lock some other element, then locking would fail silently? > > Yes. > > The element is removed form DOM, it must be unlocked and not have mouse events sent to it. However, the user agent must still release the system cursor. In the case of chromium that will be an asynchronous operation, and a lock request will be rejected. We could ignore this logic in WebKit, but I believe it's preferable to handle it here to reduce the chance or logic error outside of this code. > > A reliable application would do the following: > - lock pointer > - remove element from dom > - handle pointer lock change event > - lock pointer > > This patch is attentive to not sent the pointer lock change until the unlock is complete. > > Applications will have other scenarios as well when the pointerLockElement is null but requesting pointer lock is rejected (users not opting in to a lock, attempting multiple locks before any are accepted). Thus, I think they'll need to be made robust to rejected requests.
Comment on attachment 152291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152291&action=review > Source/WebCore/page/PointerLockController.cpp:95 > + // Set element null immediatly to block any future interaction with it :%s/immediatly/immediately/g > Source/WebCore/page/PointerLockController.cpp:97 > + m_element = 0; This is more a spec question for you and is just a corner case, but is it ok that in this case webkitPointerLockElement is null even though it is still technically locked?
Created attachment 152304 [details] Patch
Comment on attachment 152291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152291&action=review >> Source/WebCore/page/PointerLockController.cpp:95 >> + // Set element null immediatly to block any future interaction with it > > :%s/immediatly/immediately/g Done. >> Source/WebCore/page/PointerLockController.cpp:97 >> + m_element = 0; > > This is more a spec question for you and is just a corner case, but is it ok that in this case webkitPointerLockElement is null even though it is still technically locked? Spec wise we're a bit constrained as the desire is to stay similar to fullscreen and also keep it simple. I think it's OK to return null even though the mouse is momentarily not sending any events to the page. In the other transition, I've filed a bug 91186, to not return an element until the user agent has completed the lock.
Comment on attachment 152304 [details] Patch R=me. Thanks for the explanation.
Comment on attachment 152291 [details] Patch Attachment 152291 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13242172 New failing tests: pointer-lock/locked-element-iframe-removed-from-dom.html
Created attachment 152308 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Committed r122626: <http://trac.webkit.org/changeset/122626>