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

web: Fix position of touch events to be relative to the canvas #2704

Merged
merged 1 commit into from
May 30, 2023
Merged

web: Fix position of touch events to be relative to the canvas #2704

merged 1 commit into from
May 30, 2023

Conversation

tronical
Copy link
Contributor

@tronical tronical commented Feb 28, 2023

Use the same logic as for mouse events when not captured.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@tronical
Copy link
Contributor Author

This is reproducible with this patch:

diff --git a/run-wasm/src/main.rs b/run-wasm/src/main.rs
index 6961358d..cd044b3e 100644
--- a/run-wasm/src/main.rs
+++ b/run-wasm/src/main.rs
@@ -1,3 +1,3 @@
 fn main() {
-    cargo_run_wasm::run_wasm_with_css("body { margin: 0px; }");
+    cargo_run_wasm::run_wasm_with_css("body { margin: 0px; } canvas { position: absolute; top: 100px; left: 100px; }");
 }

After applying, run it with chrome and in the developer tools switch between device emulation or not. Observe in the console debug output how the mouse event coordinates are relative to the canvas, while the touch coordinates are not.

@madsmtm
Copy link
Member

madsmtm commented Mar 4, 2023

@ryo33 you added the comment in #2188, do you have some input on why you didn't do it to start with?

@ryo33
Copy link
Contributor

ryo33 commented Mar 4, 2023

I would not add it, and I didn't know what is the problem and how to fix it.
https://github.com/ryo33/winit/blame/1e9dca12c0ee5c4f312c29b743a1fa8bd6323651/src/platform_impl/web/web_sys/event.rs#L251

@tronical
Copy link
Contributor Author

When using winit with the web on a device that delivers touch events (mobile phone), the coordinates of the touch points that winit delivers to the application should be relative to the winit window. So (0/0) should be the top-left of the window and thus the canvas element. This regressed in the last winit release when dom touch events were changed to be delivered directly.

The issue is reproducible with the patch I mentioned on an earlier comment. It places the canvas at a non-zero coordinate and then when tapping within the canvas, the coordinates of the touch point that winit sends to the application are in fact absolute. So tapping at the top-left corner of the canvas now produces (100/100) when it should be (0/0).

@ryo33
Copy link
Contributor

ryo33 commented Mar 17, 2023

Yes, I've thought that it should be as this pr. But I just did not know and regard this case when I wrote my pr.

@tronical
Copy link
Contributor Author

@madsmtm ping. Anything I can do to help move this forward? :)

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM.

I also checked why calling mouse_position() wouldn't have been enough, apparently the spec doesn't support offsetX/Y for touch events.

@kchibisov
Copy link
Member

This will need a rebase, other than that it looks good to me.

@tronical
Copy link
Contributor Author

Rebased once more to resolve ChangeLog conflicts.

@tronical
Copy link
Contributor Author

ping :-). Anything I can do to help get this merged?

@daxpedda
Copy link
Member

This should probably wait until after #2662.

Use the same logic as for mouse events when not captured.
@tronical
Copy link
Contributor Author

Rebased on top of current master branch.

@daxpedda
Copy link
Member

Will probably be moot after #2731.
Anyway, it's great for now!

Thank you!

@daxpedda daxpedda merged commit de53274 into rust-windowing:master May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants