Closed Bug 1284788 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

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.
Depends on: 1285069
P2 (~"in the next few months") since it seems like you're working on this.
Priority: -- → P2
Assignee: nobody → xidorn+moz
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()
Attachment #8772693 - Flags: review?(bugs) → review+
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?
ChangePointerLockedElement is fine.
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/
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/d5407d5642bf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.