Bug 77029 – Pointer Lock handles disconnected DOM elements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vincent Scheib]

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.

[Vincent Scheib]

Created attachment 152106 [details]
Patch

[Adrienne Walker]

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?

[Vincent Scheib]

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.

[Vincent Scheib]

Created attachment 152291 [details]
Patch

[Vincent Scheib]

(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.

[Adrienne Walker]

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?

[Vincent Scheib]

Created attachment 152304 [details]
Patch

[Vincent Scheib]

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.

[Adrienne Walker]

Comment on attachment 152304 [details]
Patch

R=me.  Thanks for the explanation.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Vincent Scheib]

Committed r122626: <http://trac.webkit.org/changeset/122626>