1284788 - Changing target of pointer lock should be allowed when the pointer has been locked

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+11]

When the pointer is locked, the document should be allowed to change the target without user gesture, per the spec. Currently, Gecko doesn't allow that.

Comment 1

[Andrew Overholt [:overholt]]

P2 (~"in the next few months") since it seems like you're working on this.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284788 - Allow change target of pointer lock when the pointer has been locked in the document.

Review commit: https://reviewboard.mozilla.org/r/65436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65436/

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8772693 [details]
Bug 1284788 - Allow change target of pointer lock when the pointer has been locked in the document.

https://reviewboard.mozilla.org/r/65436/#review62506

::: dom/base/nsDocument.cpp:12372
(Diff revision 1)
>  
>    return nullptr;
>  }
>  
> +static void
> +SetPointerLockOnElement(Element* aElement, nsIDocument* aDocument,

It is unclear to me what aDocument is supposed to be.
And why it is needed, if GetUncomposedDoc() could be used.

Is aDocument only for event dispatching, and needed only in the case when pointer locked element has been moved to another document?
This could use some comment.

::: dom/base/nsDocument.cpp:12379
(Diff revision 1)
> +{
> +  MOZ_ASSERT(aDocument);
> +  MOZ_ASSERT(aElement != aPointerLockedElement);
> +  if (aPointerLockedElement) {
> +    MOZ_ASSERT(aPointerLockedElement->GetUncomposedDoc() == aDocument);
> +    aPointerLockedElement->ClearPointerLock();

Ahaa, so SetPointerLockOnElement may actually just clear the lock. Call the method SetOrClearPointerLock?

::: dom/tests/mochitest/pointerlock/file_changeLockElement.html:53
(Diff revision 1)
> +          callback(e);
> +        }
> +      };
> +
> +      var tester = new ClickTester(block3);
> +      function start() {

Please add a comment what ends up calling start()

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/65436/#review62506

> It is unclear to me what aDocument is supposed to be.
> And why it is needed, if GetUncomposedDoc() could be used.
> 
> Is aDocument only for event dispatching, and needed only in the case when pointer locked element has been moved to another document?
> This could use some comment.

It is not strictly needed. It is for event dispatching, and needed when either aElement is null or aPointerLockedElement is null. Because the caller must have known what the document is, so I think passing it in is better than trying getting it from one of the elements.

But you remind me that it is possible that this function can be called when unbinding an element from the document... I probably should check what would happen then, and ensure the assertions here actually make sense.

> Ahaa, so SetPointerLockOnElement may actually just clear the lock. Call the method SetOrClearPointerLock?

It means to unify the logic of set/clear/transfer pointer lock (transfer is basically clear+set with some steps skipped). Probably call it ChangePointerLockedElement?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

ChangePointerLockedElement is fine.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f747a8ad379c

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8772693 [details]
Bug 1284788 - Allow change target of pointer lock when the pointer has been locked in the document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65436/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5407d5642bf
Allow change target of pointer lock when the pointer has been locked in the document. r=smaug

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5407d5642bf