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

Change the type of click, auxclick, and contextmenu to PointerEvent #317

Merged
merged 5 commits into from
Dec 14, 2020

Conversation

NavidZ
Copy link
Member

@NavidZ NavidZ commented Mar 9, 2020

This is the followup change to WHATWG html spec and ui events spec pull requests to change the type of click, auxclick, and contextmenu events to pointerevents:

Closes #100


Preview | Diff

@NavidZ NavidZ requested a review from smaug---- March 9, 2020 17:42
@patrickhlauke
Copy link
Member

what's the status of this? just waiting for a review, or any more things happening upstream/in UIEvents?

@NavidZ
Copy link
Member Author

NavidZ commented Apr 20, 2020

The ui events part landed. The html spec part is sent. But Domenic suggested to wait until Chrome at least enables this and makes sure there is no regression. If so we will land the one in html spec. We can do either. Either merge now and if it turns out to cause some unrecoverable regressions then revert it. Or just wait until Chrome ships and confirms there is no bad regressions and then merge. I'll leave it up to you.

@patrickhlauke patrickhlauke self-requested a review July 13, 2020 22:46
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

Just getting this ready but waiting for any tests/regression checks before merging.

@smaug----
Copy link
Contributor

@NavidZ Is this now enabled by default in Chrome?

index.html Outdated Show resolved Hide resolved
@NavidZ
Copy link
Member Author

NavidZ commented Oct 9, 2020

Looking at the code it seems it is still under experimental flag.

@mustaqahmed @liviutinta were running some experiments to make sure it is backward compatible and it doesn't cause any issues. I haven't heard any bad news out of that so far. Could you provide any update you might have?

@liviutinta
Copy link
Contributor

We are running an experiment, and gradually expanding its scope. There were no bugs logged yet. If there are no bugs logged by November we plan to start the shipping process. We believe the major risk will be around clientX/clientY being fractional. If there is any breakage, we plan to have a flag in the code to return clientX/clientY as long but keep click/auxclick/contextmenu events as PointerEvent.

- remove the requirement for positive pointerId which was not backwards-compatible (and I couldn't find reference to this change in minutes from PEWG meetings around that time)
- add mention of special case for zero pointerId (reusing some of the language from later on about PointerEvent stream)
Copy link
Member

@mustaqahmed mustaqahmed left a comment

Choose a reason for hiding this comment

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

I have a few more suggestions, sorry didn't spot them last time.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

@smaug---- @mustaqahmed @NavidZ @liviutinta one final review ... we happy with the latest wording/changes on this?

Copy link
Member

@mustaqahmed mustaqahmed left a comment

Choose a reason for hiding this comment

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

Thanks Patrick for addressing my comments, LGTM. I have one last suggestion; if you see a better way, please go ahead without waiting for my review again.

index.html Outdated Show resolved Hide resolved
@patrickhlauke patrickhlauke merged commit bb02e92 into gh-pages Dec 14, 2020
@patrickhlauke patrickhlauke deleted the click-auxclick-contextmenu-type branch December 14, 2020 00:45
@@ -313,7 +313,7 @@ <h2><code>PointerEvent</code> Interface</h2>
<dl data-dfn-for="PointerEvent" data-link-for="PointerEvent">
<dt><dfn>pointerId</dfn></dt>
<dd>
<p>A unique identifier for the pointer causing the event. This identifier MUST be unique from all other <a data-lt="active pointer">active pointers</a> in the <a>top-level browsing context</a> (as defined by [[HTML]]) at the time. A user agent MAY recycle previously retired values for <code>pointerId</code> from previous active pointers, if necessary.</p>
<p>A unique identifier for the pointer causing the event. This identifier MUST be unique from all other <a data-lt="active pointer">active pointers</a> in the <a>top-level browsing context</a> (as defined by [[HTML]]) at the time. The <code>pointerId</code> value of zero is reserved to indicate events that were generated by something other than a pointing device. A user agent MAY recycle previously retired values for <code>pointerId</code> from previous active pointers, if necessary.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

"value of zero is reserved to indicate events that were generated by something other than a pointing device." seems like backwards incompatible change.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. But the reason we chose that was no other implementation uses 0 that at this point. So we believe it should be safe to do so now. Otherwise, technically all the numbers are valid in pointerid space. Actually now that we are at it for breaking compat we can force all valid pointerid ones to be positive numbers (again something that the current implementations already do AFAIK) so maybe we can reserver those negative numbers for other future features or something.
Having said that, unless we add yet another field to the prototype to indicate non-pointing device I cannot imagine a way to be fully backward compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Good point...looks like we assumed pointerId must start with 1. After your comment, I realized this behavior is mentioned only in the Note below. I will file an issue now.

@smaug----
Copy link
Contributor

There seem to be tests for this. _is_a_pointerevent...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should "click", "dblclick" and "contextmenu" events be PointerEvents?
5 participants