- From: Domenic Denicola <notifications@github.com>
- Date: Mon, 26 Oct 2020 12:33:38 -0700
- To: w3c/pointerlock <pointerlock@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/pointerlock/pull/49/review/517104355@github.com>
@domenic commented on this pull request.
> - 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.
+ Perform the following steps:
+ <ol>
+ <li>
+ Let <dfn>promise</dfn> be <a href="https://heycam.github.io/webidl/#a-new-promise">a new promise</a>.</li>
Nit: typically we use `<var>` for variables, instead of `<dfn>` for declarations and `<a>` for references. Your version makes sense, but is unusual.
> + <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>
+ <a>Reject</a> <a>promise</a> with a "{{WrongDocumentError}}" {{DOMException}}.
+ </li>
+ <li>
+ Return <a>promise</a>.
This needs to be dedented. I.e., you can't return from inside a queued task. The same for all other instances.
> - to lose focus.
- </p>
- <p>
- Synthetic mouse events created by application script act the same
- regardless of lock state.
+ Perform the following steps:
+ <ol>
+ <li>
+ Let <dfn>promise</dfn> be <a href="https://heycam.github.io/webidl/#a-new-promise">a new promise</a>.</li>
+ </li>
+ <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:
Should this be negated? I.e. "is not, or has an ancestor browsing context which is not"?
> + <a>Reject</a> <a>promise</a> with a "{{InvalidStateError}}" {{DOMException}}.
+ </li>
+ <li>
+ Return <a>promise</a>.
+ </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>, then:
+ <ol>
+ <li>
+ If <a>options</a> are equivalent to the current <a>pointer-lock options</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>.
This needs to use a similar style to the error cases:
1. If options are equivalent to the current pointer-lock options, then:
1. Queue an element task on the user interaction task source, given this, to perform the following steps:
1. Set the pointer-lock target to this.
1. Fire an event named pointerlockchange at this's node document.
1. Resolve promise.
1. Return promise.
> + <li>
+ If <a>options</a> are not equivalent to the user agent's current
+ <a>pointer-lock options</a> and <a>options</a> are not supported:
+ <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 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>
+ <a href="https://html.spec.whatwg.org/multipage/webappapis.html#queue-an-element-task">
This looks incorrect; you're nesting two task queuings with two event firings.
> + <ol>
+ <li>
+ Update the user agent's <a>pointer-lock target</a> to this element.
+ </li>
+ <li>
+ Update the user agent's <a>pointer-lock options</a> to <a>options</a>
+ </li>
+ <li>
+ <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 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
Is error correct here?
> + </li>
+ </ol>
+ </li>
+ </ol>
+ </li>
+ </ol>
+ </li>
+ <li>
+ If the user agent's current pointer-lock target's shadow-including root is the
+ same as this element's shadow-including root, then:
+ <ol type='i'>
+ <li>
+ The user agent's <a>pointer-lock target</a> must be updated to this element
+ </li>
+ <li>
+ a <a>pointerlockchange</a> event must be sent
This needs to use the "fire an event" phrasing.
> + </li>
+ </ol>
+ </li>
+ <li>
+ If the user agent's current pointer-lock target's shadow-including root is the
+ same as this element's shadow-including root, then:
+ <ol type='i'>
+ <li>
+ The user agent's <a>pointer-lock target</a> must be updated to this element
+ </li>
+ <li>
+ a <a>pointerlockchange</a> event must be sent
+ </li>
+ <li>
+ the promise must <a>resolve</a>
+ </li>
You probably want to return promise from this branch, instead of continuing onward.
> + If the user agent's current pointer-lock target's shadow-including root is the
+ same as this element's shadow-including root, then:
+ <ol type='i'>
+ <li>
+ The user agent's <a>pointer-lock target</a> must be updated to this element
+ </li>
+ <li>
+ a <a>pointerlockchange</a> event must be sent
+ </li>
+ <li>
+ the promise must <a>resolve</a>
+ </li>
+ </ol>
+ </li>
+ <li>
+ If the request was not started from an <a>engagement gesture</a> and
Should this error condition check come before all the success conditions? The same for the below ones. Or is the idea that it's OK to ignore the sandboxing flag, etc. if the shadow-including root is the same?
> + <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ <a>Reject</a> <a>promise</a> with a "{{SecurityError}}" {{DOMException}}.
+ </li>
+ <li>
+ Return <a>promise</a>.
+ </li>
+ </ol>
+ </ol>
+ </li>
+ <li>
+ <dl data-dfn-for="PointerLockOptions" data-link-for=
+ "PointerLockOptions">
+ If <a>options</a>["<a>unadjustedMovement</a>"] is true and the platform does not
+ support <a>unadjustedMovement</a>:
Is this redundant with step 4.2? Maybe this should move before step 4, and then step 4.2 could be eliminated?
> + <a>pointerlockerror</a> at <a>this</a>'s
+ <a href="https://dom.spec.whatwg.org/#concept-node-document">node document</a>.
+ </li>
+ <li>
+ <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 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>
+ The <a>promise</a> must be resolved.
Why would you resolve the promise in an error condition (not supported options)?
--
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-517104355
Received on Monday, 26 October 2020 19:33:52 UTC