Closed
Bug 1284785
Opened 8 years ago
Closed 8 years ago
Display error message in console when pointer lock request is rejected
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(2 files)
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63418/
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #8769569 -
Flags: review+ → review?(bugs)
Updated•8 years ago
|
Attachment #8769569 -
Flags: review?(bugs) → review+
Comment 8•8 years ago
|
||
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'
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dc858a13aa9c
https://hg.mozilla.org/mozilla-central/rev/32e536d6ced0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
"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.
Assignee | ||
Comment 14•8 years ago
|
||
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•8 years 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)
Assignee | ||
Comment 16•8 years ago
|
||
(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)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•