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

Add note about rounding coordinates for click, auxclick, contextmenu #404

Merged

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Aug 4, 2021

See the comment about web compat issues with the "upgraded" click/auxclick/contextmenu as pointer events #100 (comment)

Closes #100


Preview | Diff

@patrickhlauke patrickhlauke marked this pull request as draft August 4, 2021 12:38
@patrickhlauke
Copy link
Member Author

Currently set this as a note, as I'm not sure if we want to set this in stone as a requirement (though I guess then that invalidates the SHOULD I used ... not sure). Let's discuss at today's meeting.

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Aug 4, 2021

Related: should we also mention anything related to this explicitly in the mouse compatibility section? So user agents know that they should also round/convert to long when generating compat mouse events?

@patrickhlauke
Copy link
Member Author

Last tangential point: currently, the definition for {{PointerEvent}} links to https://w3c.github.io/pointerevents/#dom-pointerevent - should it not go to what is essentially the interface https://w3c.github.io/pointerevents/#pointerevent-interface ? (if so, just need to repoint the <dfn> appropriately). To me this would make more sense, but not sure what the conventions are for specs...

@patrickhlauke
Copy link
Member Author

And last additional point, I promise: wondering if the current note in 4.1

The PointerEvent interface inherits from MouseEvent, defined in UI Events and extended by CSSOM View Module.

should actually be expanded to make clear what that last bit means ... fractional coordinates. took me a while to work out that nowhere in the PE spec we actually explicitly say anything about fractional coordinates/that they're double

@patrickhlauke patrickhlauke marked this pull request as ready for review August 4, 2021 20:35
@patrickhlauke
Copy link
Member Author

patrickhlauke commented Aug 4, 2021

Made the changes discussed in today's meeting:

  • expanded the note at the end of 4.1 (to foreshadow the topic a bit more about what CSSOM proposes, the fact that it's still only a proposal not a standard, and that there are UA requirements for those UAs that do already follow this)
  • made the proposed note for 4.2.12 normative instead

While I was in 4.2.12, also cleaned up the structure of the prose there to make the distinction between events that came from pointing devices and those that didn't more obvious when scanning over the section.

@patrickhlauke
Copy link
Member Author

Just adding here, now that I thought it through some more: so the compat problems/issues would come about in user agents where PointerEvents have been implemented already using fractional coordinates, but the same implementation hasn't been extended to regular MouseEvents, right? If a browser decided to already implement fractional coordinates for MouseEvents, this would actually lead to problems again (essentially, the opposite type of problem this tries to address), right?

In short, the problem only appeared because Chrome (and others?) didn't apply the CSSOM approach for MouseEvents, but just for PointerEvents, right? So my proposed sentence

For user agents that already implement this proposed extension, there are additional normative requirements when it comes to the click, auxclick, and contextmenu events

should probably more specifically be about

For user agents that already implement this proposed extension for PointerEvents, but not for MouseEvents, there are additional normative requirements when it comes to the click, auxclick, and contextmenu events

likewise, in

However, this change — when applied to click, auxclick, and contextmenu — has proven to lead to web compatibility issues with legacy code.

it should probably read more along the lines of

However, this change — when applied to PointerEvents, but not to regular MouseEvents — has proven to lead to web compatibility issues with legacy code when applied to click, auxclick, and contextmenu.

i.e. if the user agent hasn't implemented fractional coordinates anywhere (not in MouseEvents, not in PointerEvents), it's a non-issue. and it's also a non-issue if the user agent has applied this change to both MouseEvents and PointerEvents. it's only a problem when it's only implemented in one and not the other, right?

@patrickhlauke
Copy link
Member Author

Just pushed a change to this PR to clarify this further. I think this now more accurately explains where the problem lies, and when this requirement needs to be implemented.

@mustaqahmed
Copy link
Member

mustaqahmed commented Aug 5, 2021

Just pushed a change to this PR to clarify this further. I think this now more accurately explains where the problem lies, and when this requirement needs to be implemented.

I am afraid it was more accurate before this last commit! Let me add a few points to focus on, in case they call for more discussion:

  • Double coordinates (as per CSSOM View spec) are known to be better but we can't move MouseEvent coordinates away from long for compat reasons. We logically expect that all UAs would pick double coordinates for PointerEvents (as per CSSOM) even if those UAs remain stuck with long coordinates in MouseEvents (as per UI Event spec).
  • The expectation above applied only to "original" PointerEvents (pointerdown, pointermove etc). The "newly promoted" PointerEvents (click, auxclick and contextmenu) suffer from the similar compat issues like MouseEvents.
  • The two points above are orthogonal to compat MouseEvents.
  • The compat event section only defines the firing of followup MouseEvents, without touching any other details of the events---because those details belong to the UIEvent spec. Adding more details here means more redundancy and may possibly call for more monkey-patching.

[EDIT: I mentioned these points only for discussion. Most likely they don't deserve to be included in the PR here.]

@patrickhlauke
Copy link
Member Author

We logically expect that all UAs would pick double coordinates for PointerEvents (as per CSSOM) even if those UAs remain stuck with long coordinates in MouseEvents (as per UI Event spec)

we don't normatively, explicitly, actually say this in our spec at the moment, right? A UA could implement PEs but with long rather than double? (as normatively the WebIDL only says we extend MouseEvent, and the CSSOM change is mentioned in an informative note only.

we can't move MouseEvent coordinates away from long for compat reasons

but IF a user agent decided to apply the CSSOM fractional coordinates to their regular MouseEvent as well, then rounding click/auxclick/contextmenu to long would then cause the same weird mismatch, no? or is the expectation that the CSSOM change will never actually make it to regular MouseEvent ?

The two points above are orthogonal to compat MouseEvents.

yes and my PR doesn't mention compat events. when I talk about MouseEvent, I mean the actual ones.

@mustaqahmed
Copy link
Member

we don't normatively, explicitly, actually say this in our spec at the moment, right? A UA could implement PEs but with long rather than double? (as normatively the WebIDL only says we extend MouseEvent, and the CSSOM change is mentioned in an informative note only.

we can't move MouseEvent coordinates away from long for compat reasons

but IF a user agent decided to apply the CSSOM fractional coordinates to their regular MouseEvent as well, then rounding click/auxclick/contextmenu to long would then cause the same weird mismatch, no? or is the expectation that the CSSOM change will never actually make it to regular MouseEvent ?

I ruled out this consideration because this is known to be impractical, therefore presumed all UAs ignore CSSOM extension for MouseEvents. After seeing (again!) that this can't be normative in the PE spec, I am finally able to see the motivation of your last patch. Phew!

The two points above are orthogonal to compat MouseEvents.

yes and my PR doesn't mention compat events. when I talk about MouseEvent, I mean the actual ones.

Looks like I misinterpreted "regular MouseEvent" as "compat MouseEvent", for no good reason! I am taking back my compat event points.

@patrickhlauke
Copy link
Member Author

it's a tangled web that we weave...this CSSOM thing stretched my brain even more than that target discussion a few months ago...

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

patrickhlauke commented Aug 24, 2021

per the discussion at last week's meeting https://www.w3.org/2021/08/18-pointerevents-minutes.html changed the "round" to reference Math.floor and point to ECMAScript.

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.

Two super picky comments!

index.html Outdated Show resolved Hide resolved
@patrickhlauke patrickhlauke merged commit 76c9195 into gh-pages Sep 1, 2021
@patrickhlauke patrickhlauke deleted the patrickhlauke-click-auxclick-contextmenu-coords branch September 1, 2021 15:12
@smaug---- smaug---- added needs-wpt Investigation whether the issue needs a wpt test has been done and wpt is missing and removed wpt labels Nov 23, 2022
@mustaqahmed
Copy link
Member

I am looking into a WPT for this!

@smaug---- smaug---- removed the needs-wpt Investigation whether the issue needs a wpt test has been done and wpt is missing label Feb 21, 2023
@smaug----
Copy link
Contributor

@mustaqahmed
Copy link
Member

Here I was modifying pointerevent_fractional_coordinates.html to cover integer coordinates in click-like events, and I think we still need that. Did I miss anything?

@mustaqahmed mustaqahmed added the needs-wpt Investigation whether the issue needs a wpt test has been done and wpt is missing label Feb 28, 2023
@smaug----
Copy link
Contributor

oops, I posted the link to wrong issue.

@mustaqahmed
Copy link
Member

No worries, I updated the issue you meant: #456

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 23, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 23, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 23, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}
@mustaqahmed
Copy link
Member

A WPT has been added.

@mustaqahmed mustaqahmed removed the needs-wpt Investigation whether the issue needs a wpt test has been done and wpt is missing label Aug 23, 2023
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 13, 2023
…tional coordinates., a=testonly

Automatic update from web-platform-tests
Add "click" to the existing WPT for fractional coordinates.

This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}

--

wpt-commits: bea5b03a40531e0beadde1a05228beaec5a98c33
wpt-pr: 41582
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Sep 14, 2023
…tional coordinates., a=testonly

Automatic update from web-platform-tests
Add "click" to the existing WPT for fractional coordinates.

This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}

--

wpt-commits: bea5b03a40531e0beadde1a05228beaec5a98c33
wpt-pr: 41582
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
This will resolve the "needs-wpt" label in
w3c/pointerevents#404

Change-Id: I06e27cd6e3adb790dbf1355c9f8cc856335bebb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797838
Reviewed-by: Kevin Ellis <[email protected]>
Auto-Submit: Mustaq Ahmed <[email protected]>
Commit-Queue: Kevin Ellis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1187412}
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?
4 participants