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

Consider pointerup as user gesture #1875

Merged
merged 4 commits into from
Oct 12, 2016

Conversation

NavidZ
Copy link
Contributor

@NavidZ NavidZ commented Oct 7, 2016

@domenic @RByers

I wanted pointerup to also be considered as user gesture so playing media or opening popup would work for pointerup events. However, I was not sure where the best place would be to add this. There is no mention of pointerup event here in this doc. @domenic, do you think having it here without any other place in this spec mentioning it is okay or do you think it should be added to the pointer events spec? Also do you have any suggestion for add tests for this to wpt? Do we have tests that verify other events in this category already?

@domenic
Copy link
Member

domenic commented Oct 7, 2016

This area was previously discussed in #1358. Basically the issue is a lot more complex and ideally we'd be figuring out what browsers actually do and speccing that. But we should definitely consider short-term fixes if they'd clarify the situation for implementers.

Could you clarify when a pointerup would be fired but a click/mouseup/another event from that list would not? That would give us some indication of how to test this (even manually).

If this change ends up being desired as-is, regarding the reference issues, you'll need to fix things by adding a reference to the pointer events spec (as the CI failure is telling you). You can copy the pattern already used for UI events and touch events. ("The following features are defined in the Touch Events specification".) At some point we'll probably want to add pointer and touch events to the list of global event handlers, but that's a separate topic.

@RByers
Copy link

RByers commented Oct 7, 2016

Note that this came up mainly because we came across a real site which expected to be able to invoke HTMLMediaElement.play() on touchend and pointerup listeners. We accidentally broke that site when pointer events was enabled. So having pointerup have at least the same status as touchend here is required for web compat.

@NavidZ
Copy link
Contributor Author

NavidZ commented Oct 7, 2016

Fair enough. I'll work on adding the reference if we agreed on adding the change here as-is. Regarding when pointerup is fired, it does include mouseup but it also includes the touch interactions for example when user taps.

@domenic
Copy link
Member

domenic commented Oct 7, 2016

But what I don't understand is, doesn't user tapping also trigger mouseup or click, thus user tapping already causes this clause to be true? That is, isn't this patch entirely redundant?

@RByers
Copy link

RByers commented Oct 7, 2016

I guess that depends on how your read the "queued by" clause. The way I see it, tapping the touchscreen queues separate tasks for trusted touchend, pointerup and click event listeners. Our bug was that we had the "triggered by user activation" behavior from the tasks running touchend and click listeners but not the tasks running the pointerup listeners, and that broke a website that expected otherwise.

@domenic
Copy link
Member

domenic commented Oct 7, 2016

Ah, I see, this comes down to the fact that UI events doesn't actually define a processing model, so we have no idea how many tasks are queued :(. I wonder which browsers queue one task vs. many.

If some browsers are queuing a task per event, then presumably that list needs to be dramatically expanded...

@domenic
Copy link
Member

domenic commented Oct 12, 2016

OK, so let's talk about getting this merged. Putting aside larger issues for now, I think we'd need to do the following:

  • Fix the xref (per the CI failure)
  • Add a similar thing for touchup; having pointerup but not touchup seems too confusing.

@NavidZ, are you willing to do that work? If not, let me know and I can take this over.

@annevk
Copy link
Member

annevk commented Oct 12, 2016

Are touchup and pointerup not always in the same task then? (I guess we still don't know?)

@domenic
Copy link
Member

domenic commented Oct 12, 2016

We just have no idea, since there is no UI events processing model. Since it's causing implementer confusion, we can try to patch over it on our side now. Worst case is that it becomes redundant, I guess.

@NavidZ
Copy link
Contributor Author

NavidZ commented Oct 12, 2016

@domenic does this sound good?
@RByers for touch I assume we only need it for touchend. Right?

@@ -72416,9 +72416,10 @@ END:VCARD</pre>
<li><code data-x="event-click">click</code></li>
<li><code data-x="event-dblclick">dblclick</code></li>
<li><code data-x="event-mouseup">mouseup</code></li>
<li><code data-x="event-pointerup">pointerup</code></li>
<li><dfn data-x-href="https://w3c.github.io/pointerevents/#the-pointerup-event"><code>pointerup</code></dfn></li> <ref spec=POINTEREVENTS>
Copy link
Member

Choose a reason for hiding this comment

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

The dfn does not belong in the list of events. You need to move this to around line 3199, and introduce two new bulleted lists ("The following features are defined in the Pointer Events specification:" and "The following features are defined in the Touch Events specification:")

@@ -119226,6 +119227,9 @@ INSERT INTERFACES HERE
<dt id="refsPNG">[PNG]</dt>
<dd><cite><a href="https://www.w3.org/TR/PNG/">Portable Network Graphics (PNG) Specification</a></cite>, D. Duce. W3C.</dd>

<dt id="refsPOINTEREVENTS">[POINTEREVENTS]</dt>
<dd><cite><a href="https://w3c.github.io/pointerevents/">Pointer Events - Level 2</a></cite>, J. Rossi, M. Brubeck, R. Byers, P. H. Lauke. W3C.</dd>
Copy link
Member

Choose a reason for hiding this comment

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

No " - Level 2"

@domenic
Copy link
Member

domenic commented Oct 12, 2016

LGTM! Can you also add yourself the acknowledgments? Will merge afterward.

@domenic domenic merged commit adafe99 into whatwg:master Oct 12, 2016
@domenic
Copy link
Member

domenic commented Oct 12, 2016

Awesome! Congrats on your first commit, and thanks very much for getting this fixed!

@domenic
Copy link
Member

domenic commented Oct 12, 2016

Oh, and for the record: no tests required for this since testing user gestures is not possible. Manual tests might be nice, but we can wait on that until we address #1358 more generally.

@NavidZ
Copy link
Contributor Author

NavidZ commented Oct 12, 2016

I see. Alright.

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

Successfully merging this pull request may close these issues.

4 participants