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 support for event::Touch on web targets #1945

Closed
wants to merge 12 commits into from

Conversation

dsluijk
Copy link
Contributor

@dsluijk dsluijk commented May 20, 2021

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • 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

This pull request adds touch support to web targets by the Pointer events API.

This PR also adds a new datatype to the Touch struct, PointerType. This indicates which pointer is being used to generate the touch event. As this has been added to the platform-independent Touch struct, all platforms supporting touch had to be changed slightly. I manually checked all implementations and added a None value to all (non-web) Touch structs, but it is possible I missed one.

Closes #1673 .

Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

You indicated that you want to help out with WASM, @FuriouZz, and I think helping review and test this (and other) web PRs would be a good start. You don't need to be a maintainer to review and/or test PRs.

CHANGELOG.md Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
@maroider maroider added DS - web C - waiting on maintainer A maintainer must review this code C - waiting on testing Does this PR work? labels May 20, 2021
Copy link

@FuriouZz FuriouZz left a comment

Choose a reason for hiding this comment

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

The implementation sounds good to me. The force touch is currently implemented on iOS and it is possible on the web (see my comment). I prefer to validate this PR with its implementation.
Just asking @maroider, this may be the subject of another Issue/PR : winit does have any plans to support multi-touch ?

@maroider
Copy link
Member

Just asking @maroider, this may be the subject of another Issue/PR : winit does have any plans to support multi-touch ?

I believe an API for that (and a pointer event API) was explored in #1374, and potentially in #99 and/or #336,

Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

Is there anything being done in this PR to prevent non-touch pointer events being reported as WindowEvent::Touch?

src/platform_impl/web/stdweb/canvas.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/canvas/pointer_handler.rs Outdated Show resolved Hide resolved
@dsluijk
Copy link
Contributor Author

dsluijk commented May 23, 2021

Is there anything being done in this PR to prevent non-touch pointer events being reported as WindowEvent::Touch?

I've added a guard for it now.

@FuriouZz
Copy link

@maroider Just noticied that PR #1900 already exist for event::Touch support created by @zicklag. That PR use touch event and support multitouch.

@maroider
Copy link
Member

Well, I'll be damned. I had completely forgotten about that PR.

@dsluijk
Copy link
Contributor Author

dsluijk commented May 23, 2021

Do note that that PR uses touch events instead of pointers, as was preferred in #1673

@maroider
Copy link
Member

Do note that that PR uses touch events instead of pointers, as was preferred in #1673

We'll go ahead with this PR in favour of #1900, in that case.

@zicklag
Copy link

zicklag commented May 26, 2021

So I should close #1900 to avoid confusion, right?

@maroider
Copy link
Member

maroider commented May 26, 2021

So I should close #1900 to avoid confusion, right?

Sure.


#1941 got merged, @dsluijk, so you'll want to rebase onto master.

@dsluijk
Copy link
Contributor Author

dsluijk commented May 27, 2021

#1941 got merged, @dsluijk, so you'll want to rebase onto master.

Done! I think it's ready to merge now, is there anything what still needs to be updated?

Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

I'd like to do a real-world test (although I don't think it's strictly necessary for me to do so), but this should otherwise be ready to be merged, despite how CI is a bit wonky ATM.

I'd also like a final sign-off from @FuriouZz before merging.

@zicklag
Copy link

zicklag commented May 27, 2021

@maroider I can try this out, if you want before you merge it. I'm currently having to use my fork in order to power my Bounty Bros. game touch support on web so I have a perfect test case. I'll try to test it within the next ~12 hours ( probably less ) and comment here with the result.

@maroider
Copy link
Member

Sounds like a plan, @zicklag.

Copy link

@FuriouZz FuriouZz left a comment

Choose a reason for hiding this comment

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

Just some reword from pointer to touch

@@ -231,6 +231,62 @@ impl<T> WindowTarget<T> {
runner.request_redraw(WindowId(id));
});

let runner = self.runner.clone();
canvas.on_pointer_move(move |device_id, location| {

Choose a reason for hiding this comment

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

Suggested change
canvas.on_pointer_move(move |device_id, location| {
canvas.on_touch_move(move |device_id, location| {

});

let runner = self.runner.clone();
canvas.on_pointer_down(move |device_id, location| {

Choose a reason for hiding this comment

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

Suggested change
canvas.on_pointer_down(move |device_id, location| {
canvas.on_touch_down(move |device_id, location| {

});

let runner = self.runner.clone();
canvas.on_pointer_up(move |device_id, location| {

Choose a reason for hiding this comment

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

Suggested change
canvas.on_pointer_up(move |device_id, location| {
canvas.on_touch_up(move |device_id, location| {

});

let runner = self.runner.clone();
canvas.on_pointer_cancel(move |device_id, location| {

Choose a reason for hiding this comment

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

Suggested change
canvas.on_pointer_cancel(move |device_id, location| {
canvas.on_touch_cancel(move |device_id, location| {

@@ -247,6 +247,46 @@ impl Canvas {
}
}

pub fn on_pointer_move<F>(&mut self, handler: F)

Choose a reason for hiding this comment

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

Suggested change
pub fn on_pointer_move<F>(&mut self, handler: F)
pub fn on_touch_move<F>(&mut self, handler: F)

F: 'static + FnMut(i32, PhysicalPosition<f64>),
{
match &mut self.mouse_state {
MouseState::HasPointerEvent(h) => h.on_pointer_cancel(&self.common, handler),

Choose a reason for hiding this comment

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

Suggested change
MouseState::HasPointerEvent(h) => h.on_pointer_cancel(&self.common, handler),
MouseState::HasPointerEvent(h) => h.on_touch_cancel(&self.common, handler),

@@ -103,11 +111,103 @@ impl PointerHandler {
));
}

pub fn on_pointer_move<F>(&mut self, canvas_common: &super::Common, mut handler: F)

Choose a reason for hiding this comment

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

Suggested change
pub fn on_pointer_move<F>(&mut self, canvas_common: &super::Common, mut handler: F)
pub fn on_touch_move<F>(&mut self, canvas_common: &super::Common, mut handler: F)

));
}

pub fn on_pointer_down<F>(&mut self, canvas_common: &super::Common, mut handler: F)

Choose a reason for hiding this comment

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

Suggested change
pub fn on_pointer_down<F>(&mut self, canvas_common: &super::Common, mut handler: F)
pub fn on_touch_down<F>(&mut self, canvas_common: &super::Common, mut handler: F)

));
}

pub fn on_pointer_up<F>(&mut self, canvas_common: &super::Common, mut handler: F)

Choose a reason for hiding this comment

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

Suggested change
pub fn on_pointer_up<F>(&mut self, canvas_common: &super::Common, mut handler: F)
pub fn on_touch_up<F>(&mut self, canvas_common: &super::Common, mut handler: F)

));
}

pub fn on_pointer_cancel<F>(&mut self, canvas_common: &super::Common, mut handler: F)

Choose a reason for hiding this comment

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

Suggested change
pub fn on_pointer_cancel<F>(&mut self, canvas_common: &super::Common, mut handler: F)
pub fn on_touch_cancel<F>(&mut self, canvas_common: &super::Common, mut handler: F)

@zicklag
Copy link

zicklag commented May 27, 2021

I'll be investigating, more, but for some reason when testing with my game it doesn't seem to be picking up tap-and-drags, when inputing through the Chrome browser's mobile viewer. I think maybe it's not holding down the touch or something. I'll debug more in a bit to see what the difference in the the sent events are between my fork and this PR.

@Friz64

This comment has been minimized.

@maroider
Copy link
Member

I'll be investigating, more, but for some reason when testing with my game it doesn't seem to be picking up tap-and-drags, when inputing through the Chrome browser's mobile viewer.

This seems to be an oversight on my end. I thought the call to set_pointer_capture wasn't removed, and I think removing that call is what's causing this issue.

@zicklag
Copy link

zicklag commented May 27, 2021

Here's the event log I'm getting when touch-and-dragging if this helps:

image

It seems to capture the first so much of a move before getting a touch-cancelled event.

@maroider
Copy link
Member

Did you try it with set_pointer_capture re-added in the correct place, @zicklag?

@zicklag
Copy link

zicklag commented May 27, 2021

That didn't seem to fix it, but in my latest tests it doesn't seem like the event handler is getting run at all, maybe. I'm looking around.

Edit: Nevermind about the event handler not getting run. I messed something up.

Here's the update I made, and it still isn't working:

image

@FuriouZz
Copy link

FuriouZz commented May 27, 2021

The problem seems to come from the browser canceling pointermove event. Setting html, body { touch-action: none; } fix the issue. Does exist an alternative of that fix?
@zicklag @maroider

pointermove documentation

The pointermove event is fired when a pointer changes coordinates, and the pointer has not been canceled by a browser touch-action.

touch-action documentation

By default, panning (scrolling) and pinching gestures are handled exclusively by the browser. An application using Pointer events will receive a pointercancel event when the browser starts handling a touch gesture. By explicitly specifying which gestures should be handled by the browser, an application can supply its own behavior in pointermove and pointerup listeners for the remaining gestures. Applications using Touch events disable the browser handling of gestures by calling preventDefault(), but should also use touch-action to ensure the browser knows the intent of the application before any event listeners have been invoked.

When a gesture is started, the browser intersects the touch-action values of the touched element and its ancestors, up to the one that implements the gesture (in other words, the first containing scrolling element). This means that in practice, touch-action is typically applied only to top-level elements which have some custom behavior, without needing to specify touch-action explicitly on any of that element's descendants.

@maroider
Copy link
Member

maroider commented May 27, 2021

Does it work if you only apply the style to the canvas? If so, we can apply the style programmatically.
We'll probably also want to look into what using preventDefault() looks like, although I don't think we can get away with using it unconditionally.

@FuriouZz
Copy link

FuriouZz commented May 27, 2021

It works on my sample

canvas.style().set_property("touch-action", "none");

We'll probably also want to look into what using preventDefault() looks like, although I don't think we can get away with using it unconditionally.

I tried that, but it does not work.

@zicklag
Copy link

zicklag commented May 27, 2021

canvas.style().set_property("touch-action", "none");

That worked for me, too. Looks like that might be all we need.

@FuriouZz
Copy link

FuriouZz commented May 28, 2021

canvas.style().set_property("touch-action", "none");

Must be added here.

@ArturKovacs
Copy link
Contributor

If I'm not mistaken then the previous message was directed to you, @dsluijk. If you don't have the opportunity to continue working on this, feel free to indicate that.

@SamiPirbay
Copy link

It works but the origin (0, 0) is at the top-left corner, with Y going downwards.

@FuriouZz
Copy link

Up please

@johanhelsing
Copy link
Contributor

Is there something I can do to help this one along?

I backported the PR to winit 0.24 (that was what bevy 0.5 was on) and tested it with one of my games, and it worked great. Would be nice to have it merged.

@dsluijk, if you don't have the time, I can try to see if I can resolve the conflicts and flip the y-axis.

@ryo33 ryo33 mentioned this pull request Feb 8, 2022
5 tasks
@madsmtm
Copy link
Member

madsmtm commented Dec 23, 2022

#2188 has been merged, which fixes this as well.

@madsmtm madsmtm closed this Dec 23, 2022
@FuriouZz
Copy link

Finally!

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 C - waiting on testing Does this PR work? DS - web
Development

Successfully merging this pull request may close these issues.

Implement touch handling on web target(s)
9 participants