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 touch event #2188

Merged
merged 37 commits into from
Dec 23, 2022
Merged

Web touch event #2188

merged 37 commits into from
Dec 23, 2022

Conversation

ryo33
Copy link
Contributor

@ryo33 ryo33 commented Feb 8, 2022

  • 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

Closes #1673

This PR is inherited from inactive PR #1945.

The differences are as follows:

  • Correctly calculates the physical position by treating with scale factor and flipping the Y-axis.
  • Support touch pressure.
  • add touch-action: none to a canvas to prevent the browser from canceling pointer events.
  • Working demo (pressure will be reflected in the cursor size).

Source code of the demo is here

cursor

CHANGELOG.md Outdated Show resolved Hide resolved
src/platform_impl/web/event_loop/window_target.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/canvas/pointer_handler.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/canvas/pointer_handler.rs Outdated Show resolved Hide resolved
@ryo33 ryo33 force-pushed the web-touch-event branch from 907ebfb to 1911832 Compare May 26, 2022 07:53
@ryo33
Copy link
Contributor Author

ryo33 commented May 26, 2022

I resolved the conflicts.

@ryo33
Copy link
Contributor Author

ryo33 commented May 26, 2022

Also, I update the demo with the latest code

Copy link
Contributor

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

LGTM

@ryo33
Copy link
Contributor Author

ryo33 commented May 26, 2022

Thanks for reviewing!

@ryo33
Copy link
Contributor Author

ryo33 commented May 30, 2022

@ryanisaacg Could you review this and suggest another reviewer for this?

@ryanisaacg
Copy link
Contributor

I haven't been involved in any winit development for a few years at least; I'm not a good reviewer any longer. (Besides, it does look like you have an approving review?)

@gilescope
Copy link

One weird thing, the y axis seems inverted - needs to be height - y. Top left should be 0,0. egui touch handling and my brain expects this, but it seems that bottom left is currently 0,0.
I'm assuming that oddness is coming from this PR.

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 13, 2022

Oh, I may misunderstand the treatment of the y-axis in winit.

https://docs.rs/winit/latest/winit/event/enum.WindowEvent.html#variant.CursorMoved

(x,y) coords in pixels relative to the top-left corner of the window. Because the range of this data is limited by the display area and it may have been transformed by the OS to implement effects such as cursor acceleration, it should not be used to implement non-cursor-like interactions such as 3D camera control.

So we don't need y-axis flipping?

@gilescope
Copy link

gilescope commented Nov 13, 2022 via email

@mockersf
Copy link
Contributor

mockersf commented Nov 15, 2022

So we don't need y-axis flipping?

I'm trying this PR in Bevy, and I had to flip back the Y axis in Bevy for it to work. So I would say that it isn't needed.
Nevermind I think it's expected in Bevy to flip it for now.

@donmai-me
Copy link

I've tried the demo on my iPad and it works great! However, there are two problems I've encountered. First is that touch events using Apple Pencil don't register. Second is that canceled touch events, such as when switching windows when using four-finger swipes, aren't removed.

The last one may be because the demo's source code doesn't handle canceled touch events.

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 22, 2022

Apple Pencil emits pointer events with pointer_type = "pen".
Does anyone know how winit handles Apple Pencil on iOS target? If it handles it as touches, we need to handle the "pen" pointer type.

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 26, 2022

I'm finally confident that we don't need the y-flip, and it's ready to merge.
I don't think we should support pen events for now because other platforms do not support it yet, as #99.

Demo improvement (now it's really simple, comprehensive, and easy to understand)

See demo and its source.

Changes are:

  • Use UI instead of Sprite because bevy's UI and winit share the same coordinate system (so no complex position transformation).
  • Handle the touch cancel event (thanks to @donmai-me for reporting).

@ryo33 ryo33 requested review from maroider and removed request for madsmtm November 26, 2022 09:03
@adsick
Copy link

adsick commented Nov 30, 2022

tried to run the demo on my android phone using chrome browser and don't see anything going on except for some gray area. (I'm pushing the screen here and there)

Screenshot

Screenshot_2022-11-30-15-40-07-98_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

@maroider
Copy link
Member

maroider commented Dec 1, 2022

I can reproduce the issue with the demo on Chrome for Android, but it works fine everywhere else I could test it (Desktop Chromium, Firefox on Desktop and Android). Our web example also works fine on Chrome for Android, so I wouldn't consider this to be a blocking issue, as this is likely an issue with how the demo is set up.

@gilescope
Copy link

gilescope commented Dec 1, 2022 via email

@peterhirn
Copy link

I was curious and fixed the android issue: demo, repo. Everything seems to work as expected. Nice work.

@gilescope
Copy link

gilescope commented Dec 19, 2022 via email

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm sorry, I haven't really been following this issue, and am a bit confused - @ryo33, do you feel this is ready now?

I'd be fine with merging it as-is (once CI passes), it has gone for long enough and while it may not be perfect it's clearly an improvement :)

src/event.rs Outdated Show resolved Hide resolved
Co-authored-by: Mads Marquart <[email protected]>
@ryo33
Copy link
Contributor Author

ryo33 commented Dec 23, 2022

@madsmtm Yes, I think it’s ready to merge.

@madsmtm madsmtm merged commit f43ce2a into rust-windowing:master Dec 23, 2022
@madsmtm
Copy link
Member

madsmtm commented Dec 23, 2022

Cool, thanks for your contribution, and sorry again it took so long (it's difficult when we don't have a maintainer for web)!

@ryo33 ryo33 deleted the web-touch-event branch December 23, 2022 08:53
@ryo33
Copy link
Contributor Author

ryo33 commented Dec 23, 2022

I haven’t figured winit out yet, but I hope I would be a maintainer for web in the future maybe. I’m creating a huge product that depends on winit, and it should work well on web. I may work on keyboard issues when my hands are free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - web
Development

Successfully merging this pull request may close these issues.

Implement touch handling on web target(s)