-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix ElementTouchEvent to convert native DOM Touch objects to PlayCanvas Touch objects #8081
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
base: main
Are you sure you want to change the base?
Conversation
…ouch objects Co-authored-by: willeastcott <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the ElementTouchEvent class to convert native browser Touch objects to PlayCanvas Touch objects, ensuring consistent Touch object types throughout the framework.
Key changes:
- Modified
ElementTouchEventconstructor to wrap native browser Touch objects fromevent.touchesandevent.changedTouchesarrays into PlayCanvasTouchobjects - Added import of
Touchclass from the touch-event module - Added comprehensive test coverage for the Touch object conversion behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/framework/input/element-input.js | Updated ElementTouchEvent constructor to convert native browser Touch arrays to PlayCanvas Touch objects using Array.from().map() |
| test/framework/input/element-touch-event.test.mjs | Added new test file to verify that ElementTouchEvent properly converts native Touch objects to PlayCanvas Touch objects |
Comments suppressed due to low confidence (1)
src/framework/input/element-input.js:286
- The JSDoc type for the
touchparameter is incorrect. It should be{globalThis.Touch}instead of{Touch}because this parameter receives a native browser Touch object (fromevent.changedTouches[i]), not a PlayCanvas Touch object. The PlayCanvas Touch objects are created from the native touch arrays in lines 297 and 304.
* @param {Touch} touch - The touch object that triggered the event.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
That looks like breaking change. |
|
@Maksims I think you have to address Copilot directly to suggest to it you think it's a breaking change (it claimed it's backwards compatible). |
|
I don't work with AI, I can clearly see, that public API docs suggest to return PlayCanvas's It will break, as people rely on these touch objects, and that they are native Touch objects, not PlayCanvas's. |
|
@Maksims This PR is an experiment to see how useful GitHub Copilot is as resolving an issue. If you don't want to participate in the experiment, feel free to drop out of the convo. |
ElementTouchEventwas passing native DOMTouchobjects directly to consumers instead of converting them to PlayCanvasTouchobjects. This causedtouch.idto returnundefinedsince native touches usetouch.identifier.Changes
element-input.jsElementTouchEventconstructor:ElementTouchEventprovides PlayCanvas Touch objects with accessibleidpropertyExample
Before this fix:
After this fix:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.