Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

got/lostpointercapture events: attributes and immediate firing on implicit release #122

Merged
merged 9 commits into from
Aug 29, 2016

Conversation

NavidZ
Copy link
Member

@NavidZ NavidZ commented Jul 28, 2016

Closes #113 and #32.

@NavidZ NavidZ force-pushed the got-lost-pointer-capture-attributes branch from 3cbe5b9 to 8585da3 Compare July 28, 2016 20:49
@@ -423,6 +423,8 @@
<li><code>pointerout</code></li>
</ul>

<p>Run <a href="#process-pending-pointer-capture">Process Pending Pointer Capture</a> steps for this <code>PointerEvent</code> if it is not <code>gotpointercapture</code> or <code>lostpointercapture</code>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reverse the sentence "If the event is not ..., run ..."

@RByers
Copy link
Contributor

RByers commented Jul 29, 2016

Big picture looks good to me, thanks! Just a couple minor style / editorial suggestions.

@NavidZ
Copy link
Member Author

NavidZ commented Jul 29, 2016

I moved the table to the other section and removed the "Composed" column from it as we do mention setting composed attribute to true for all the pointer events at first.
Regarding the attributes of got/lostpointercapture, I'm not sure if we split the attributes values into two sections makes things clearer. Right now they are all in this "Firing event.." section. Regarding that boundary events I believe we would be able to remove them all from that section in future PRs. Right?

@RByers
Copy link
Contributor

RByers commented Jul 29, 2016

Personally I think the table makes more sense in 5.2 as a summary of that section (listing all the events). Having a line in the firing algorithm saying "initialize the properties according to this table" (link) seems adequate to me. But I don't have a strong opinion, this is all just editorial. @patrickhlauke WDYT?

@patrickhlauke
Copy link
Member

Personally I think the table makes more sense in 5.2 as a summary of that section (listing all the events). Having a line in the firing algorithm saying "initialize the properties according to this table" (link) seems adequate to me.

yeah, i'd agree with that

@NavidZ NavidZ force-pushed the got-lost-pointer-capture-attributes branch from 0315d08 to 578e45e Compare July 29, 2016 19:42
<td>None</td>
</tr>
<tr>
<td><code>lostpointercapture</code></td>
<td>Sync/Async</td>
<td>Yes</td>
<td>No</td>
<td>Yes</td>
<td>None</td>
</tr>
</tbody>
</table>
<p>In the case of the <a>primary pointer</a>, these events (with the exception of <code>gotpointercapture</code>, and <code>lostpointercapture</code>) may also fire <a>compatibility mouse events</a>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get rid of the comma before "and lostpointercamture...".

@mustaqahmed mustaqahmed changed the title Attributes of got/lostpointercapture events Aug 2, 2016
@NavidZ
Copy link
Member Author

NavidZ commented Aug 15, 2016

Anyone else has any comment about this PR?

@mustaqahmed
Copy link
Member

Am I the only one to think that the mix of "attribute setting" and "event firing" between section headers 5.1.3 and 5.1.3.1 could be made cleaner?

@NavidZ
Copy link
Member Author

NavidZ commented Aug 15, 2016

I personally agree with you. Do you think just moving all attribute values to 5.2 along with that table and reference that section for all attributes be better? Regarding firing events in 5.1.3 vs 5.1.3.1, they are a bit different I believe. They are 2 different routines that call into each other in some situations. WDTY?

@mustaqahmed
Copy link
Member

I personally agree with you. Do you think just moving all attribute values to 5.2 along with that table and reference that section for all attributes be better?

Yes, that should be cleaner than referencing that table twice before it appears.

Regarding firing events in 5.1.3 vs 5.1.3.1, they are a bit different I believe. They are 2 different routines that call into each other in some situations. WDTY?

The mix of "setting attributes" and "firing details" looked a bit odd to me. Your suggestion above would reduce the "setting" part in 5.1.3 into a one-line reference to 5.2, which seems better. So, please don't split 5.1.3.

@NavidZ
Copy link
Member Author

NavidZ commented Aug 24, 2016

Does this look okay?


<p>For any pointer event initialize the <code>composed</code> ([[!WHATWG-DOM]]) attribute to <code>true</code> and <a href="https://www.w3.org/TR/uievents/#widl-UIEvent-detail"><code>detail</code></a> [[!DOM-LEVEL-3-EVENTS]] attribute to 0.</p>
<p>Initialize the <code>bubbles</code> and <code>cancelable</code> attributes for the event according to the <a href="#pointer-event-type-table">Pointer Events table</a>.</p>
<p>For any pointer event initialize the attributes of the event according to the <a href="#h-pointer-event-types">Pointer Event types</a> section.</p>
<p>For <code>gotpointercapture</code> and <code>lostpointercapture</code> all the attributes except the ones defined in the table above should be the same as the Pointer Event that caused the user agent to run <a href="#process-pending-pointer-capture">Process Pending Pointer Capture</a> and fire the <code>gotpointercapture</code> and <code>lostpointercapture</code> events.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cleaner now, thanks. LGTM except for one last comment: please fix the "table above" here by moving this para to somewhere around Line 520.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks for the catch. I missed this one. Done.

@mustaqahmed
Copy link
Member

LGTM.

@@ -400,29 +400,9 @@
<section>
<h2>Firing events using the <code>PointerEvent</code> interface</h2>
<p>To <dfn>fire a pointer event name e</dfn> means to <dfn>fire an event named e</dfn> as defined in [[!DOM4]] with an event using the <a>PointerEvent</a> interface whose attributes are set as defined in <a href="#pointerevent-interface"><code>PointerEvent</code> Interface</a>.</p>
<p>For any pointer event initialize the attributes of the event according to the <a href="#h-pointer-event-types">Pointer Event types</a> section.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think would be clearer if merged the previous paragraph since it's really a continuation of the theme of initializing the attributes. Maybe: "... whose attributes are set as defined in PointerEvent interface, and in Pointer Event types for the type e.".

Also while you're editing that line, can you fix the typo: "fire a pointer event name e" -> "fire a pointer event named e"?

@RByers
Copy link
Contributor

RByers commented Aug 25, 2016

LGTM

@RByers
Copy link
Contributor

RByers commented Aug 25, 2016

Oh one more thing I almost forgot again before merging - add a ChangeLog entry with a link to this PR.

@RByers
Copy link
Contributor

RByers commented Aug 25, 2016

Great except now you have a merge conflict - need to rebase your branch

@RByers RByers merged commit e37f6d3 into w3c:gh-pages Aug 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants