- From: Domenic Denicola <notifications@github.com>
- Date: Wed, 22 Jan 2020 10:46:41 -0800
- To: w3c/pointerlock <pointerlock@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/pointerlock/pull/49/review/346794415@github.com>
domenic requested changes on this pull request.
Really great to see this firming up! I left a first round of feedback that should get us started.
> + we will call the <dfn>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> must fire all
+ 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>
+ <dl data-dfn-for="PointerLockOptions" data-link-for=
+ "PointerLockOptions">
+ While in the <a>Pointer Lock State</a> if <a>unadjustedMovement</a> is
+ <code>true</code> the event coordinates must not be affected by the
I think this section shouldn't reference API objects like unadjustedMovement. Instead it should reference concepts like "target" and "unadjusted movement", and then the requestPointerLock() method should translate between API and concepts.
So you could say something like
> If the Pointer Lock State is requested with **unadjusted movement**, then .... Otherwise, the UA must rely...
> + While in the <a>Pointer Lock State</a> if <a>unadjustedMovement</a> is
+ <code>true</code> the event coordinates must not be affected by the
+ underlying platform behaviors such as mouse acceleration. In other
+ words, UA must use the APIs provided by the underlying platform to
+ guaranteee getting the raw events. If <a>unadjustedMovement</a>
+ is <code>undefined</code> or <code>false</code> UA must rely on the
+ default behavior of the underlying platform regarding the mouse
+ acceleration.
+ </dl>
+ </p>
+ <p>
+ In the <a>Pointer Lock State</a> the system mouse cursor must be hidden.
+ Movement and button presses of the mouse must not cause the window
+ to lose focus.
+ </p>
+ <p>
```suggestion
<p class="note">
```
Since this is kind of a side-note that is about something related, but not part of the normative definition of the Pointer Lock State itself.
> + 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>
+ 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>. If pointer lock is entered or exited
+ for any reason a <a>pointerlockchange</a> event must be sent.
I think this last sentence should be removed, or transformed into a statement of fact like "When pointer lock is entered or exited, a pointerlockchange event will be sent." The normative text in the algorithms should take care of this.
> + </p>
+ </dd>
+ </dl>
+ </section>
+ <section>
+ <h3>
+ `"pointerlockchange"` and `"pointerlockerror"` Events
+ </h3>
+ <p>
+ 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>. 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=
This should be removed; it will be taken care of in the algorithm.
> - 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 follow these steps
```suggestion
Perform the following steps:
```
> - 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 follow these steps
+ <ol>
+ <li>
+ If the the <a>target</a>'s
This should be something like "If [this](https://heycam.github.io/webidl/#this)'s shadow-including root
> - <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 follow these steps
+ <ol>
+ <li>
+ If the the <a>target</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 queue <a>pointerlockerror</a>
So, for all these error cases, the following is the more precise phrasing:
1. [Queue an element task](https://html.spec.whatwg.org/multipage/webappapis.html#queue-an-element-task) on the ??? task source, given [this](https://heycam.github.io/webidl/#this), to perform the following steps:
1. [Fire an event](https://dom.spec.whatwg.org/#concept-event-fire) named `pointerlockerror` at [this](https://heycam.github.io/webidl/#this)'s [node document](https://dom.spec.whatwg.org/#concept-node-document).
1. Return [a promise rejected with](https://heycam.github.io/webidl/#a-promise-rejected-with) a "[`WrongDocumentError`](https://heycam.github.io/webidl/#wrongdocumenterror)" [`DOMException`](https://heycam.github.io/webidl/#idl-DOMException).
As for the task source, it may be worth checking implementations (e.g. just inspecting the Chromium source) to see what task source is in use. It might be a dedicated new one, in which case somewhere in the document you'll want to define the **pointer lock task source** and reference that. ([Example of another spec](https://wicg.github.io/portals/#portal-task-source).)
> + "{{WrongDocumentError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
+ </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) follow these steps:
Hmm. This is not quite as explicit as would be ideal. I think the best way to do this would be something like:
- Previously, where you state that the user agent has a (pointer-lock) target, also state that it has a **current pointer-lock options** which is a `PointerLockOptions` dictionary or null.
- Be sure that when you lock/unlock and manage the pointer-lock target, you also set the user agent's pointer-lock options.
- Update the check here to "If the user agent's pointer-lock target's shadow-including root is equal to this's shadow-including root".
... Well, I think there's more cleanup in general, but that's a start.
> - </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
+ we will call the <dfn>target</dfn>, receives all mouse events and the cursor
This should state that the [user agent](https://infra.spec.whatwg.org/#user-agent) has a pointer-lock target, to be clear this is scoped to the entire UA and not, e.g., to each Document.
And it should be stated that the target is either an `Element` or null.
I think the name "pointer-lock target" is also a bit clearer than just "target".
Finally, you need to improve the spec text so that everything which locks/unlocks explicitly changes the user agent's pointer-lock target, either to an element or to null. That includes requestPointerLock(), exitPointerLock(), and the user dismissing the pointer lock with no API calls. To do this, it may help to have a central definition for **enter pointer lock** and **exit pointer lock** which: put the UA in/out of the pointer lock state, then queue a task to manage the pointer-lock target and fire the pointerlockchange event at the target's node document.
> - <p>
- Synthetic mouse events created by application script act the same
- regardless of lock state.
+ RequestPointerLock() will follow these steps
+ <ol>
+ <li>
+ If the the <a>target</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 queue <a>pointerlockerror</a>
+ and <a href="https://heycam.github.io/webidl/#reject">reject</a> the promise with a
+ "{{WrongDocumentError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
This should be made precise, building on top of the previous more-precise definition of pointer-lock target, using something like:
> If the user agent's pointer-lock target is an element whose shadow-including root is not equal to this's shadow-including root, then:
> - regardless of lock state.
+ RequestPointerLock() will follow these steps
+ <ol>
+ <li>
+ If the the <a>target</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 queue <a>pointerlockerror</a>
+ and <a href="https://heycam.github.io/webidl/#reject">reject</a> the promise with a
+ "{{WrongDocumentError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
This is not a good error for this case. Maybe InvalidStateError or NotAllowedError?
> + <li>
+ If the the <a>target</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 queue <a>pointerlockerror</a>
+ and <a href="https://heycam.github.io/webidl/#reject">reject</a> the promise with a
+ "{{WrongDocumentError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
This step seems like a dupe of the previous one?
> + If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
+ </li>
+ <li>
+ If the pointer is already locked by an element whose <a>shadow-including root</a> is a different
+ document queue <a>pointerlockerror</a> and <a href="https://heycam.github.io/webidl/#reject">reject</a>
+ the promise with a "{{InUseAttributeError}}" {{DOMException}}.
+ </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) follow these steps:
+ <ol>
+ <li>
+ If the <a>pointerLockOptions</a> have not changed the pointer lock <a>target</a>
So, I think the flow of this algorithm should be: check all error conditions first, then, lock and/or update options. There should only be one success step.
Right now as-written it seems like you can succeed without a user gesture because this success case is too early.
I'll skip reviewing these sub-steps, as well as all other success substeps, until we have this restructured a bit.
> + </li>
+ <li>
+ If the options have changed and the new options are supported the pointer lock <a>target</a>
+ must be updated to this element, the new options must be used, 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>target</a>
+ must be updated to this element, a <a>pointerlockchange</a>
+ event sent and the promise must <a>Resolve</a>.
+ </li>
+ <li data-link-for="Document">
I find it a bit nicer to use the syntax `{{Document/exitPointerLock()}}` and remove these `data-link-for="Document"` additions. (If that doesn't work, then my respec knowledge is bad, so please ignore.)
> + <li>
+ If the options have changed and the new options are supported the pointer lock <a>target</a>
+ must be updated to this element, the new options must be used, 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>target</a>
+ must be updated to this element, a <a>pointerlockchange</a>
+ event sent and the promise must <a>Resolve</a>.
+ </li>
+ <li data-link-for="Document">
+ If the request was not started from an <a>engagement gesture</a> and
We should clean up the "engagement gesture" wording to use https://html.spec.whatwg.org/multipage/interaction.html#tracking-user-activation. See also [this guidance](https://docs.google.com/document/d/14wT89JZ0qeRehXGkcn3_meXxjvlHKgM9d7aJj80kQcQ/edit). But, I think it's probably better to defer that to a future PR.
> + </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>target</a>
+ must be updated to this element, a <a>pointerlockchange</a>
+ event sent and the promise must <a>Resolve</a>.
+ </li>
+ <li data-link-for="Document">
+ If the request was not started from an <a>engagement gesture</a> and
+ the Document has not previously released a successful Pointer Lock with
+ {{exitPointerLock()}} queue <a>pointerlockerror</a> and
+ <a href="https://heycam.github.io/webidl/#reject">reject</a> the promise
+ with a "{{ConstraintError}}" {{DOMException}}.
We've been using NotAllowedError for user activation failures, previously.
> + <p>
+ Conversely, if pointer lock is exited via {{exitPointerLock()}}
+ no <a>engagement gesture</a> is required to reenter pointer lock.
+ This enables applications that frequently move between
+ interaction modes, and ones that may do so based on a timer or
+ remote network activity.
+ </p>
+ <p data-link-for="Element">
+ Pointer lock is commonly combined with fullscreen [[FULLSCREEN]],
+ and if an <a>engagement gesture</a> is used to enter fullscreen
+ it is sufficient for a subsequent {{requestPointerLock()}}.
+ </p>
+ </aside>
+ </li>
+ <li>
+ if the document object's
This should be "If this's node document's"
> + sandboxing flag set</a> has the <a>sandboxed pointer lock browsing
+ context flag</a> set queue <a>pointerlockerror</a> and
+ <a href="https://heycam.github.io/webidl/#reject">reject</a> the
+ promise with a "{{SecurityError}}" {{DOMException}}.
+ </li>
+ <li>
+ <dl data-dfn-for="PointerLockOptions" data-link-for=
+ "PointerLockOptions">
+ If if <a>unadjustedMovement</a> is true and the platform does not
+ support <a>unadjustedMovement</a> queue <a>pointerlockerror</a> and
+ <a href="https://heycam.github.io/webidl/#reject">reject</a> the
+ promise with a "{{NotSupportedError}}" {{DOMException}}.
+ </dl>
+ <aside class="note" data-link-for="Document">
+ "{{NotSupportedError}}" {{DOMException}} should be reserved for
+ unsupported options in future iterations of this spec.
I'd be a bit more explicit, e.g. "That is, no other error conditions should use "NotSupportedError", so that web developers can use it for feature testing"
> @@ -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
We should turn this into an algorithm in the same way. It looks like it'll have two steps, if you take the above advice about centralizing the definition of "exit the pointer lock state":
1. If shadow-including root test fails, return.
1. Queue a task to exit pointer lock.
Then you'd move the text about the mouse cursor position into the "exit pointer lock" definition.
--
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-346794415
Received on Wednesday, 22 January 2020 18:46:44 UTC