Display error message in console when pointer lock request is rejected

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.
Depends on: 1285069
Assignee: nobody → xidorn+moz
Created attachment 8769568 [details]
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/
Attachment #8769568 - Flags: review?(bugs)
Attachment #8769569 - Flags: review?(bugs)
Created attachment 8769569 [details]
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 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)
Attachment #8769568 - Flags: review?(bugs) → review+
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.
Attachment #8769569 - Flags: review?(bugs) → review+
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 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/
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?
Attachment #8769569 - Flags: review+ → review?(bugs)

Updated

a year ago
Attachment #8769569 - Flags: review?(bugs) → review+
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

a year ago
Pushed by xquan@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc858a13aa9c
https://hg.mozilla.org/mozilla-central/rev/32e536d6ced0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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.
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(xidorn+moz)
"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.
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...
Flags: needinfo?(mheubusch)

Comment 15

a year ago
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.
Flags: needinfo?(mheubusch) → needinfo?(xidorn+moz)
(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?)
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.