1284785 - Display error message in console when pointer lock request is rejected

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

Currently we use NS_WARNING to record rejection of pointer lock request, which doesn't make sense, because web developers do not care about command line output at all. Those information should be posted to the web console.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284785 part 1 - Make ShouldLockPointer have internal linkage rather than being a method of nsDocument.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284785 part 2 - Report reason to console if a pointer lock request is denied.

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

Comment 3

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

Comment on attachment 8769568 [details]
Bug 1284785 part 1 - Make ShouldLockPointer have internal linkage rather than being a method of nsDocument.

https://reviewboard.mozilla.org/r/63416/#review60336

(not sure why this patch, but no reason why not)

Comment 4

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

Comment on attachment 8769569 [details]
Bug 1284785 part 2 - Report reason to console if a pointer lock request is denied.

https://reviewboard.mozilla.org/r/63418/#review60338

identifier for uncomposeddoc case and the warning message for it fixed, r+.
Feel free to ask another review if you're not sure about the message.

::: dom/base/nsDocument.cpp:12369
(Diff revision 1)
>    nsDocument* d = static_cast<nsDocument*>(doc.get());
> -  if (!e || !d || e->GetUncomposedDoc() != d) {
> -    DispatchPointerLockError(d);
> +  const char* error = nullptr;
> +  if (!e || !d) {
> +    error = "PointerLockDeniedNotInDocument";
> +  } else if (e->GetUncomposedDoc() != d) {
> +    error = "PointerLockDeniedMovedDocument";

Really annoying to have error handling in many places, but I guess that is how it needs to be

::: dom/base/nsDocument.cpp:12370
(Diff revision 1)
> -  if (!e || !d || e->GetUncomposedDoc() != d) {
> -    DispatchPointerLockError(d);
> +  const char* error = nullptr;
> +  if (!e || !d) {
> +    error = "PointerLockDeniedNotInDocument";
> +  } else if (e->GetUncomposedDoc() != d) {
> +    error = "PointerLockDeniedMovedDocument";
> +  }

GetUncomposedDoc() returns null for shadow elements, so nothing is then moved.
So, "MovedDocument" sounds a bit wrong.

::: dom/locales/en-US/chrome/dom/dom.properties:87
(Diff revision 1)
> +PointerLockDeniedOnUse=Request for pointer lock was denied because the pointer is currently controlled by a different document.
> +PointerLockDeniedNotInDocument=Request for pointer lock was denied because requesting element is not in a document.
> +PointerLockDeniedSandboxed=Request for pointer lock was denied because Pointer Lock API is restricted via sandbox.
> +PointerLockDeniedHidden=Request for pointer lock was denied because the document is not visible.
> +PointerLockDeniedNotFocused=Request for pointer lock was denied because the document is not focused.
> +PointerLockDeniedMovedDocument=Request for pointer lock was denied because requesting element has moved document.

So this is wrong. It needs to say something about element not being in document.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8769568 [details]
Bug 1284785 part 1 - Make ShouldLockPointer have internal linkage rather than being a method of nsDocument.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8769569 [details]
Bug 1284785 part 2 - Report reason to console if a pointer lock request is denied.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/63418/#review60338

> GetUncomposedDoc() returns null for shadow elements, so nothing is then moved.
> So, "MovedDocument" sounds a bit wrong.

The new patch explicitly checks whether GetUncomposedDoc() returns null, and reports "not in document" for that case. Do you think this is fine?

Comment 8

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

Comment on attachment 8769569 [details]
Bug 1284785 part 2 - Report reason to console if a pointer lock request is denied.

https://reviewboard.mozilla.org/r/63418/#review60616

Feel free to ask feedback from some native English speaker too :)

::: dom/base/nsDocument.cpp:12333
(Diff revision 2)
> -    return false;
>    }
>  
>    nsCOMPtr<nsIDocument> ownerDoc = aElement->OwnerDoc();
>    if (aCurrentLock && aCurrentLock->OwnerDoc() != ownerDoc) {
> -    NS_WARNING("ShouldLockPointer(): Existing pointer lock element in a different document");
> +    return "PointerLockDeniedOnUse";

Is it 'on use' or 'in use'?

::: dom/locales/en-US/chrome/dom/dom.properties:87
(Diff revision 2)
> +PointerLockDeniedOnUse=Request for pointer lock was denied because the pointer is currently controlled by a different document.
> +PointerLockDeniedNotInDocument=Request for pointer lock was denied because requesting element is not in a document.
> +PointerLockDeniedSandboxed=Request for pointer lock was denied because Pointer Lock API is restricted via sandbox.
> +PointerLockDeniedHidden=Request for pointer lock was denied because the document is not visible.
> +PointerLockDeniedNotFocused=Request for pointer lock was denied because the document is not focused.
> +PointerLockDeniedMovedDocument=Request for pointer lock was denied because requesting element has moved document.

I think 'the requesting element'

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc858a13aa9c
part 1 - Make ShouldLockPointer have internal linkage rather than being a method of nsDocument. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e536d6ced0
part 2 - Report reason to console if a pointer lock request is denied. r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc858a13aa9c
https://hg.mozilla.org/mozilla-central/rev/32e536d6ced0

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Olli Pettay [:smaug] from comment #4)
> > +PointerLockDeniedMovedDocument=Request for pointer lock was denied because requesting element has moved document.
> 
> So this is wrong. It needs to say something about element not being in
> document.

I've been staring at this string for a while failing to understanding it. I assume this should have been addressed before landing.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

I choose the same expression as FullscreenDeniedMovedDocument here... I think the meaning is that "the requesting element has been moved to another document".

If that expression is not clear, probably we should fix it for fullscreen strings as well?

Comment 13

[Francesco Lodolo [:flod]]

"has moved document" is definitely confusing to me as a non native speaker, I would prefer "has moved to a different document" if that's the meaning.

I'd suggest to get a native speaker to give feedback on this.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

mheubusch, could you provide some suggestion on wording here? The current content is
> Request for pointer lock was denied because the requesting element has moved document.
and some people (including me) find it confusing. The meaning is actually something like
> ... because the requesting element has been moved to a different document.

It doesn't seem to me "move" can be used as transitive verb the way in the first sentence actually...

Comment 15

[mheubusch]

Hello :xidorn - I'd love to help.  I'm trying to read up a bit on this, but need a bit of context from you.  

Does the error appear to all users or is this something that appears only in Tools>Web Developer>Console? Would all the people who are likely to see this error understand what "requesting element" is?  

Does the error mean:
We can't lock your pointer because you moved your pointer out of this application window and we don't know where it is.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

(In reply to mheubusch from comment #15)
> Hello :xidorn - I'd love to help.  I'm trying to read up a bit on this, but
> need a bit of context from you.  
> 
> Does the error appear to all users or is this something that appears only in
> Tools>Web Developer>Console? Would all the people who are likely to see this
> error understand what "requesting element" is?  

It only appears in the console, so not for general user. Developers who care about this message should understand what "requesting element" is I think.

> Does the error mean:
> We can't lock your pointer because you moved your pointer out of this
> application window and we don't know where it is.

No, it means that the HTML element (imagine that as an area of the page) who wants to lock the pointer in no longer stays in the original page. The original page is the page the element is in when it requests that permission. (I guess I should probably ask a developer for advice for messages targeting developers?)