- From: Domenic Denicola <notifications@github.com>
- Date: Mon, 10 Aug 2020 12:56:35 -0700
- To: w3c/pointerlock <pointerlock@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/pointerlock/pull/49/review/464516639@github.com>
@domenic commented on this pull request.
> - document=].
- </p>
- <p class="note">
- Magnification software increases the size of content on the screen. It
- uses the mouse to move the magnified point of focus around. When a
- pointer lock is initiated, the magnification software needs to switch
- to using the keyboard to move the magnified point of focus around
- instead. When a pointerlockchange event is fired, web browsers
- therefore need to make sure the event is communicated to assistive
- technologies like screen magnifiers.
- </p>
+ <section>
+ <h3>
+ The <dfn>Pointer Lock State</dfn> Definition
+ </h3>
+ <p><a>Pointer Lock State</a> is the state where a single DOM element, which
It's a bit unusual to capitalize all the words in the linked references; "Pointer lock state" at the start of a sentence (and "pointer lock state" mid-sentence) is more typical.
> + <section>
+ <h3>
+ The <dfn>Pointer Lock State</dfn> Definition
+ </h3>
+ <p><a>Pointer Lock State</a> is the state where a single DOM element, which
+ we will call the <dfn>pointer-lock target</dfn>, receives all mouse events and the cursor
+ is hidden.</p>
+ <p>
+ Once in the <a>Pointer Lock State</a> the <a>user agent</a> has a
+ <a>pointer-lock target</a> and a set of <a>PointerLockOptions</a>. The
+ <a>pointer-lock target</a> must receive all relevant user generated
+ {{MouseEvent}} events (for example: `mousemove`, `mousedown`, `mouseup`,
+ `click`, and `wheel`) [[ui-events]]. No other elements may receive these
+ events while in <a>Pointer Lock State</a>. Events that require the concept of
+ a mouse cursor must not be dispatched (for example: `mouseover`, `mouseout`,
+ `drag`, and `drop`).
Would it be possible to list these events normatively, instead of only giving some examples?
I think the current text is OK, because the lack of precision here is just a symptom of the larger lack of precision in how mouse events are specified in general. Some progress is being made there, in https://github.com/w3c/uievents/issues/200, and if that lands then eventually this paragraph can move into the UI events spec as an actual change to the processing model.
But in the meantime, it would help achieve interoperability if you were precise about the list of events.
For example, it's not clear to me whether pointer events are impacted by this change.
> - pointer lock is initiated, the magnification software needs to switch
- to using the keyboard to move the magnified point of focus around
- instead. When a pointerlockchange event is fired, web browsers
- therefore need to make sure the event is communicated to assistive
- technologies like screen magnifiers.
- </p>
+ <section>
+ <h3>
+ The <dfn>Pointer Lock State</dfn> Definition
+ </h3>
+ <p><a>Pointer Lock State</a> is the state where a single DOM element, which
+ we will call the <dfn>pointer-lock target</dfn>, receives all mouse events and the cursor
+ is hidden.</p>
+ <p>
+ Once in the <a>Pointer Lock State</a> the <a>user agent</a> has a
+ <a>pointer-lock target</a> and a set of <a>PointerLockOptions</a>. The
I think it would be better to define a specific field for the options, like you did for pointer-lock target. E.g. `and a <dfn>pointer-lock options</dfn>, which is a <a>PointerLockOptions</a>`. Then you could reference "pointer-lock options" in the paragraph below.
> + </dd>
+ <dt>
+ <dfn>unadjustedMovement</dfn> member
+ </dt>
+ <dd>
+ <p>
+ If this value is set to `true`, then the pointer movements will
+ not be affected by the underlying platform modications such as
+ mouse accelaration.
+ </p>
+ </dd>
+ </dl>
+ </section>
+ <section>
+ <h3>
+ `"pointerlockchange"` and `"pointerlockerror"` Events
Nit: traditionally, for some reason, we don't include the quotes when talking about event names. I agree quotes make more sense, but for consistency it'd be better to remove them.
> + <dfn>unadjustedMovement</dfn> member
+ </dt>
+ <dd>
+ <p>
+ If this value is set to `true`, then the pointer movements will
+ not be affected by the underlying platform modications such as
+ mouse accelaration.
+ </p>
+ </dd>
+ </dl>
+ </section>
+ <section>
+ <h3>
+ `"pointerlockchange"` and `"pointerlockerror"` Events
+ </h3>
+ <p class="note">
This shouldn't be a note; it's a useful statement of fact.
> + instead. When a pointerlockchange event is fired, web browsers
+ therefore need to make sure the event is communicated to assistive
+ technologies like screen magnifiers.
+ </p>
+ </section>
+ <section>
+ <h3><dfn>Exit Pointer Lock</dfn></h3>
+ <p>
+ The process of exiting pointer lock is as follows:
+ <ol>
+ <li>
+ <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockchange</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a> on the
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#user-interaction-task-source">
+ User Interaction task source</a>
You don't fire an event on a task source; you queue a task on a task source. So I think the task source thing needs to move down to the exitPointerLock() method definition.
Finally, there is no "this" in this algorithm, since it is not a Web IDL method definition. You need to pass in a document, or an element, from the calling site.
> + <h3><dfn>Exit Pointer Lock</dfn></h3>
+ <p>
+ The process of exiting pointer lock is as follows:
+ <ol>
+ <li>
+ <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockchange</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a> on the
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#user-interaction-task-source">
+ User Interaction task source</a>
+ </li>
+ <li>
+ The system mouse cursor must be displayed again and positioned at
+ the same location that it was when pointer lock was entered (the
+ same location that is reported in <code>screenX</code>,
+ <code>screenY</code>, when the pointer is locked).
This is duplicated in the definition of exitPointerLock().
Additionally, this would be best stored as an additional piece of state on the user agent, like pointer-lock target and pointer-lock options.
> + The process of exiting pointer lock is as follows:
+ <ol>
+ <li>
+ <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockchange</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a> on the
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#user-interaction-task-source">
+ User Interaction task source</a>
+ </li>
+ <li>
+ The system mouse cursor must be displayed again and positioned at
+ the same location that it was when pointer lock was entered (the
+ same location that is reported in <code>screenX</code>,
+ <code>screenY</code>, when the pointer is locked).
+ </li>
+ <li>
This should not be an algorithm step. Instead, there should be an explicit algorithm step that sets the user agent's pointer-lock target and pointer-lock options to null, and exits the pointer lock state. Then, these things will become automatic side effects of exiting pointer lock state, per the requirements in https://pr-preview.s3.amazonaws.com/w3c/pointerlock/pull/49.html#the-pointer-lock-state-definition.
> - relevant user generated {{MouseEvent}} events (for example:
- `mousemove`, `mousedown`, `mouseup`, `click`, and `wheel`)
- [[ui-events]] to the <a>target</a> of pointer lock, and not fire
- mouse events to other elements. Events that require the concept of
- a mouse cursor must not be dispatched (for example: `mouseover`,
- `mouseout`, `drag`, and `drop`).
- </p>
- <p>
- In the locked state the system mouse cursor must be hidden.
- Movement and button presses of the mouse must not cause the window
- to lose focus.
- </p>
- <p>
- Synthetic mouse events created by application script act the same
- regardless of lock state.
+ RequestPointerLock() will perform the following steps:
```suggestion
Perform the following steps:
```
> - <p>
- Synthetic mouse events created by application script act the same
- regardless of lock state.
+ RequestPointerLock() will perform the following steps:
+ <ol>
+ <li>
+ If the <a href="https://heycam.github.io/webidl/#this">this</a>'s
+ <a>shadow-including root</a> is the <a>active document</a> of a
+ [=Document/browsing context=] which is (or has an <a>ancestor
+ browsing context</a> which is) in focus by a window which is in
+ focus by the operating system's window manager:
+ <ol>
+ 1. <a href="https://html.spec.whatwg.org/multipage/webappapis.html#queue-an-element-task">
+ Queue an element task</a> on the
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#user-interaction-task-source">
+ User Interaction task source</a>, given this, to perform the following steps:
```suggestion
user Interaction task source</a>, given this, to perform the following steps:
```
> + [=Document/browsing context=] which is (or has an <a>ancestor
+ browsing context</a> which is) in focus by a window which is in
+ focus by the operating system's window manager:
+ <ol>
+ 1. <a href="https://html.spec.whatwg.org/multipage/webappapis.html#queue-an-element-task">
+ Queue an element task</a> on the
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#user-interaction-task-source">
+ User Interaction task source</a>, given this, to perform the following steps:
+ <ol>
+ <li>
+ <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockerror</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ Return <a>a promise rejected with</a> a "{{WrongDocumentError}}" {{DOMException}}.
You can't return a promise from inside a queued task. What you need to do is create the promise in step 1, before queuing the task, then reject it from inside here, and then return it as the last step of the algorithm.
> + <a>pointerlockerror</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ Return <a>a promise rejected with</a> a "{{InvalidStateError}}" {{DOMException}}.
+ </li>
+ </ol>
+ </ol>
+ </li>
+ <li>
+ If the user agent's <a>pointer-lock target</a>'s <a>shadow-including
+ root</a> is equal to <a>this</a>'s <a>shadow-including root</a> follow these steps:
+ <ol>
+ <li>
+ If this request's <a>pointerLockOptions</a> are equivalent to the user agent's current
+ <a>pointerLockOptions</a> then the <a>pointer-lock target</a>
"this request's pointerLockOptions" should be just `<var>options</var>`. And maybe line 322 can be modified to include `options`, although I'm not sure how ReSpec works and that might break something.
> + </ol>
+ </ol>
+ </li>
+ <li>
+ If the user agent's <a>pointer-lock target</a>'s <a>shadow-including
+ root</a> is equal to <a>this</a>'s <a>shadow-including root</a> follow these steps:
+ <ol>
+ <li>
+ If this request's <a>pointerLockOptions</a> are equivalent to the user agent's current
+ <a>pointerLockOptions</a> then the <a>pointer-lock target</a>
+ must be updated to this element, a <a>pointerlockchange</a>
+ event sent, and the promise must <a>resolve</a>.
+ </li>
+ <li>
+ If this request's <a>pointerLockOptions</a> are not equivalent to the user agent's current
+ <a>pointerLockOptions</a> and this request's <a>pointerLockOptions</a> are not supported:
"current pointerLockOptions" should refer to the user agent field discussed in "I think it would be better to define a specific field for the options..."
> + <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockerror</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ Return <a>a promise rejected with</a> a "{{NotSupportedError}}" {{DOMException}}.
+ </li>
+ </ol>
+ </ol>
+ </li>
+ <li>
+ If this request's <a>pointerLockOptions</a> are not equivalent to the user agent's current
+ <a>pointerLockOptions</a> and this request's <a>pointerLockOptions</a> are supported the
+ user agent's <a>pointer-lock target</a> must be updated to this element, the user agent's
+ <a>pointerLockOptions</a> must be updated to this request's <a>pointerLockOptions</a>, a
+ <a>pointerlockchange</a> event sent, and the promise must <a>resolve</a>.
This passive voice and one big long sentence is weird. The structure of the sibling bullets is better. And e.g. just say "Resolve _promise_" (assuming that, per the above, you create a _promise_ variable in step 1) instead of "The promise must resolve".
> + <a>pointerLockOptions</a> must be updated to this request's <a>pointerLockOptions</a>, a
+ <a>pointerlockchange</a> event sent, and the promise must <a>resolve</a>.
+ </li>
+ </ol>
+ </li>
+ <li>
+ If any element (including this one), whose <a>shadow-including
+ root</a> is same as this element's <a>shadow-including root</a>, is
+ already locked (or pending lock) the pointer lock <a>pointer-lock target</a>
+ must be updated to this element, a <a>pointerlockchange</a>
+ event sent and the promise must <a>Resolve</a>.
+ </li>
+ <li>
+ If the request was not started from an <a>engagement gesture</a> and
+ the Document has not previously released a successful Pointer Lock with
+ {{Document/exitPointerLock()}}:
Could this be made precise by checking if the user agent's pointer-lock target is non-null?
> + <ol type='i'>
+ <li>
+ <a href=https://dom.spec.whatwg.org/#concept-event-fire">Fire an event</a> named
+ <a>pointerlockerror</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ Return <a>a promise rejected with</a> a "{{SecurityError}}" {{DOMException}}.
+ </li>
+ </ol>
+ </ol>
+ </li>
+ <li>
+ <dl data-dfn-for="PointerLockOptions" data-link-for=
+ "PointerLockOptions">
+ If if <a>unadjustedMovement</a> is true and the platform does not
```suggestion
If <var>options</var>["<a>unadjustedMovement</a>"] is true and the platform does not
```
> + Return <a>a promise rejected with</a> a "{{NotSupportedError}}" {{DOMException}}.
+ </li>
+ </ol>
+ </ol>
+ </dl>
+ <aside class="note" data-link-for="Document">
+ "{{NotSupportedError}}" {{DOMException}} should be reserved for
+ unsupported options in future iterations of this spec. That is,
+ no other error conditions should use "{{NotSupportedError}}", so
+ that web developers can use it for feature testing.
+ </aside>
+ </li>
+ <li>
+ <dl data-dfn-for="PointerLockOptions" data-link-for=
+ "PointerLockOptions">
+ If none of the above errors have occured the user agent will enter
"the user agent will" is strange passive voice phrasing. You're the spec; you say what to do! So just say "1. Enter pointer lock state. 2. Set the user agent's pointer-lock target to this. 3. Set the user agent's pointer-lock options to |options|. 4. Queue an element task..."
> + Two events are used to communicate pointer lock state change or an
+ error in changing state. They are named <dfn>pointerlockchange</dfn>
+ and <dfn>pointerlockerror</dfn>.
+ </p>
+ <p class="note">
+ Magnification software increases the size of content on the screen.
+ It uses the mouse to move the magnified point of focus around. When a
+ pointer lock is initiated, the magnification software needs to switch
+ to using the keyboard to move the magnified point of focus around
+ instead. When a pointerlockchange event is fired, web browsers
+ therefore need to make sure the event is communicated to assistive
+ technologies like screen magnifiers.
+ </p>
+ </section>
+ <section>
+ <h3><dfn>Exit Pointer Lock</dfn></h3>
This algorithm is only referenced once in the document. Should it be called from more places than just exitPointerLock()? E.g., on document unloading?
> + error in changing state. They are named <dfn>pointerlockchange</dfn>
+ and <dfn>pointerlockerror</dfn>. If pointer lock is entered or exited
+ for any reason a <a>pointerlockchange</a> event must be sent.
+ </p>
+ <p>
+ <a>User agents</a> must deliver these events by <a data-lt=
+ "queue a task">queuing a task</a> to <a>fire an event</a> of the
+ appropriate name with its <code>bubbles</code> attribute [[DOM]] set to
+ false to the pointer lock <a>target</a> element's <a>node document</a>.
+ </p>
+ <p class="note">
+ Magnification software increases the size of content on the screen. It
+ uses the mouse to move the magnified point of focus around. When a
+ pointer lock is initiated, the magnification software needs to switch
+ to using the keyboard to move the magnified point of focus around
+ instead. When a pointerlockchange event is fired, web browsers
Marcos is saying to not put it in a note, since notes cannot affect implementation behavior.
> @@ -332,7 +454,7 @@ <h2>
</dt>
<dd>
<p>
- Initiates an exit from pointer lock state if currently locked to a
+ Initiates an exit from <a>pointer lock state</a> if currently locked to a
Looks pretty good, but now you can just delete all the non-step paragraphs.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/pointerlock/pull/49#pullrequestreview-464516639
Received on Monday, 10 August 2020 19:56:50 UTC