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 click and contextmenu event types to PointerEvent #5324

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

NavidZ
Copy link
Contributor

@NavidZ NavidZ commented Mar 2, 2020

As per change in ui events change click and
contextmenu event types to PointerEvent.


/indices.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )

@rniwa
Copy link

rniwa commented Mar 3, 2020

Are you sure this change is Web compatible? @cdumez

@NavidZ
Copy link
Contributor Author

NavidZ commented Mar 3, 2020

Are you sure this change is Web compatible?

FYI @BoCupp-Microsoft @smaug----

This was discussed at length in Pointer Events working group. As far as we tested this on Chrome through flags enabling the feature we didn't see any reports from anyone. However, we would like to at least have a wording in the spec and send an intent to ship and enable this by default so we get 100% of traffic on this feature and see whether there is anything out there that might be broken due to this. If anything comes up we need to investigate and see what we can do to mitigate that or even maybe rollback the change.

@domenic
Copy link
Member

domenic commented Mar 3, 2020

Can you restore the pull request template that was deleted? It contains important information we need to have filled out.

@NavidZ
Copy link
Contributor Author

NavidZ commented Mar 9, 2020

Can you restore the pull request template that was deleted? It contains important information we need to have filled out.

done.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This doesn't appear to change any normative text; only the non-normative index. I guess this is just following changes to https://w3c.github.io/uievents/ which says to use PointerEvent for click. (Although it doesn't seem to define contextmenu at all 🙁.)

Is the intention to change https://html.spec.whatwg.org/#fire-a-synthetic-mouse-event as well?

source Outdated
@@ -3242,6 +3242,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<p>The following features are defined in <cite>Pointer Events</cite>: <ref spec=POINTEREVENTS></p>

<ul class="brief">
<li><dfn data-x-href="https://www.w3.org/TR/pointerevents3/#pointerevent-interface"><code>PointerEvent</code></dfn> interface</li>
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to the editor's draft, like the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@NavidZ
Copy link
Contributor Author

NavidZ commented Mar 9, 2020

This doesn't appear to change any normative text; only the non-normative index. I guess this is just following changes to https://w3c.github.io/uievents/ which says to use PointerEvent for click. (Although it doesn't seem to define contextmenu at all 🙁.)

Yeah. Unfortunately I couldn't find anywhere defining contextmenu event in a normative way.

Is the intention to change https://html.spec.whatwg.org/#fire-a-synthetic-mouse-event as well?

Good point. Although firing that in a PointerEvent manner wouldn't help at much as we would have to fill all the default attributes for that click event but that doesn't hurt to change at as well to be consistent. I'll make sure to also change that in Chromium and measure the impact as whole in terms of backward compatibility.

@smaug----
Copy link

(Chrome and Edge counts as one implementation obviously ;) )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This looks good to me as an editorial PR that just updates HTML to reflect changes already made to UI events.

I'm unsure whether we should merge now so that the two specs are consistent, or wait until there are tests and compat data showing this is possible. I think it would have been ideal if we waited for both specs.

I tend to prefer waiting, even for editorial/alignment changes, so that HTML can serve as a "checkpoint" that helps encourage tests/implementer interest. So for now I'll tag this "do not merge yet".

That also seems like the right call since we may want to update the click() algorithm too, and we don't know yet.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Mar 10, 2020
@NavidZ
Copy link
Contributor Author

NavidZ commented Mar 10, 2020

(Chrome and Edge counts as one implementation obviously ;) )

Fair enough. I guess I should get into the habit of using Chromium instead of Chrome to be more inclusive.

I tend to prefer waiting, even for editorial/alignment changes, so that HTML can serve as a "checkpoint" that helps encourage tests/implementer interest. So for now I'll tag this "do not merge yet".

Sounds good to me.

@patrickhlauke
Copy link
Member

Just wanted to check if there's been any further discussion/movement on this, as we have a PR on the Pointer Events spec that's currently hanging/waiting. w3c/pointerevents#317

Also, cross-referencing this UI Events PR which has been merged already w3c/uievents#259

@annevk
Copy link
Member

annevk commented Sep 15, 2020

@patrickhlauke looking at OP it seems this needs tests as well as more browser bugs at least. And per the comments above some demonstration that this is web-compatible would also be good. Has this change shipped somewhere?

@patrickhlauke
Copy link
Member

@NavidZ @mustaqahmed can probably speak more to that

@mustaqahmed
Copy link
Contributor

In Chrome we are currently running a limited trial which seems to suggest the change in web compatible. To get a conclusive evidence, we have a plan to expand the trial to more population in a careful way---we expect to finalize a plan by end of this month.

@NavidZ
Copy link
Contributor Author

NavidZ commented Sep 15, 2020

In Chrome we are currently running a limited trial which seems to suggest the change in web compatible. To get a conclusive evidence, we have a plan to expand the trial to more population in a careful way---we expect to finalize a plan by end of this month.

@liviutinta @mustaqahmed could you also add a link to the wpt test for this as well as other browser bugs to the issue description.

@mustaqahmed
Copy link
Contributor

@liviutinta just landed the following three WPTs in pointerevents/ folder:
pointerevent_auxclick_is_a_pointerevent.html
pointerevent_click_is_a_pointerevent.html
pointerevent_contextmenu_is_a_pointerevent.html

@domenic
Copy link
Member

domenic commented Nov 3, 2020

Great! I can take care of rebasing.

Have you confirmed that we don't want to change element.click(), and that will continue to use MouseEvent?

@mustaqahmed
Copy link
Contributor

I think any "click" event created by the UA would be a PointerEvent, including the one from element.click(). And of course the fired event will be an intanceof MouseEvent as well by event class hierarchy.

We are gradually ramping up our Chrome Beta experiment because we are not seeing any regression. We are expecting to send out an intent to ship soon.

@domenic
Copy link
Member

domenic commented Nov 3, 2020

Oh, OK. In that case we need more spec and test updates to proceed; this spec PR does not change click(), and the above tests don't seem to test it.

@NavidZ
Copy link
Contributor Author

NavidZ commented Nov 4, 2020

I think any "click" event created by the UA would be a PointerEvent, including the one from element.click(). And of course the fired event will be an intanceof MouseEvent as well by event class hierarchy.

I might be wrong but I don't remember making that change to change element.click to be PointerEvent as well. You might want to double check and see whether that is also part of the change or not in Chromium.

@domenic
Copy link
Member

domenic commented Nov 6, 2020

I'll rebase and merge this since click() is being handled separately in #6126.

This only updates the event index. The normative text for firing click
is in UI Events, and was changed in
w3c/uievents#259. There is no normative text for
firing the contextmenu event, sadly.
@domenic domenic merged commit 2eae259 into whatwg:master Nov 6, 2020
@domenic domenic added topic: events impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation and removed do not merge yet Pull request must not be merged per rationale in comment labels Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: events
Development

Successfully merging this pull request may close these issues.

None yet

7 participants