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

Support cross-browser touch input #527

Closed

Conversation

jakmeier
Copy link

@jakmeier jakmeier commented Aug 9, 2019

Commit Message:
This commit fixes single touch input for a number of mobile browsers.
It uses TouchEvents and MouseEvents instead of PointerEvents only,
which is necessary in mobile FF, Opera, Safari, and Samsung Internet.

Resolves #526

Implementation Details

I have removed dependency on PointerEvents, which are only implemented in some browsers. (also described in more detail in #526)

In particular, these changes were made to the code:

  • Add event listener to Touch-Start and trigger a MouseMoved + a MouseButton (left, pressed) Event on single touch input. Move is necessary to update the position before the MouseButton event is fired. (Multi-touch input remains to be ignored.)
  • Add event listener to Touch-End and trigger MouseButton (left, released) when all touch points are removed.
  • Add event listener to TouchMove, updating the mouse cursor position on single point touch movement. (Multi-touch input remains to be ignored.)
  • Change existing listeners for PointerEvents to use MouseEvents where a corresponding TouchEvent is used now. (to avoid double-reporting of events)

Motivation and Context

Fixes #526

Other than that, I tried to keep everything as it is. (No changes on public API were made.)

Testing

The behavior on browsers that have not been affected by this bug should remain untouched. I have manually click-monkey tested this in Firefox (on Android and Ubuntu), Samsung Internet, and Chrome on Android.
I don't think there is much reason to be concerned that other browsers could be broken now.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checks

  • I have read CONTRIBUTING.md.
  • This Pull Request targets the right branch
  • I have updated CHANGES.md, with [BREAKING] next to all breaking changes
  • I have updated the documentation accordingly if necessary
  • I have updated / added tests to cover my changes if necessary

This commit fixes single touch input for a number of mobile browsers.
It uses TouchEvents and MouseEvents instead of PointerEvents only,
which is necessary in mobile FF, Opera, Safari, and Samsung Internet.

Resolves ryanisaacg#526
@codecov-io
Copy link

codecov-io commented Aug 9, 2019

Codecov Report

Merging #527 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development     #527   +/-   ##
============================================
  Coverage        29.93%   29.93%           
============================================
  Files               33       33           
  Lines             1894     1894           
============================================
  Hits               567      567           
  Misses            1327     1327
Impacted Files Coverage Δ
src/lifecycle/run.rs 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54081db...9d2d58f. Read the comment docs.

@jakmeier
Copy link
Author

Earlier today, I added a commit to avoid this warning in the console:

Screenshot from 2019-08-10 21-18-04

My commit did successfully prevent the warning by not calling preventDefault() in the event handler. But that caused some browsers to fire a Mouse event in addition to the Touch event and Quicksilver ended up reporting the button event twice.

Now I removed the commit again (with a force push). With this current status, there is no double-reporting in any browser to the best of my knowledge after testing it thoroughly.
Unfortunately, the warning persists in FF on Android.
On the positive side, no warning is shown in FF on Ubuntu, Chrome on Android, and Samsung Internet.

Is this warning acceptable for now? It is only on mobile and it's currently the best solution I can provide to enable touch input compatible with all mobile browsers.

@ryanisaacg
Copy link
Owner

Just a heads up, I probably won't have time to look at this until later next week.

@ryanisaacg
Copy link
Owner

I'm not sure the extra effort to support low-usage browsers is worth it, especially because Quicksilver doesn't really have support for mobile interfaces.

Also, Quicksilver's web input code will be moved upstream to winit soon, so I'm reluctant to make short-lived changes to it.

@jakmeier
Copy link
Author

jakmeier commented Aug 25, 2019

Thanks for looking into it. I understand your concerns, the long tail of work included to support each and every browser is far too much for Quicksilver and its current plans for the future.

However, I think the reported bug is bigger than I might have made it look in my description. Let me try to elaborate a bit more.

With way of handling input that seems obvious to me, it is impossible to know the coordinates of a click event caused by a single touch. The move event is produced too late, if at all. This is independent of the browser used, as long as the input-method is touch.

In an implementation that uses Quicksilver and just assumes it works as intended, the click position lags always one behind. In other words, the user can tap once at (100,100) and nothing happens. Then, the user tapping again (anywhere, say (300,300)) produces a click event in Quicksilver while the mouse position on the window is at at (100,100). And, if the touch triggers a PointerMove, the move event is produced by Quicksilver right afterwards, so you could theoretically buffer and delay the handling of the event as a user of the library as a workaround. But as far as I remember, this PointerMove is only triggered when the finger position changes slightly during the touch.

Therefore, it is crucial that Quicksilver reads the position of the TouchStart event. Or, if you want to keep the PointerEvents, then at least read the position of the PointerDownEvent, which is not done right now.
But you could have iPhone users and Mozilla Firefox users on board for free by reading the TouchEvents instead, as I suggested.

At least this has been my experience. But maybe I have been doing something wrong? Has anyone else actively tested and used the touch input of Quicksilver in a Game? If I am right, it should be a problem for others as well...

Anyway, if you don't have the time right now to look into this, it is also fine with me if this is not merged. I am already using my fork of Quicksilver including the fix and there is no pressing reason why I cannot continue to do so.

If you want to check the touch on your own phone, you could check it on my demo of my little hobby project at http://demo.paddlers.ch/ (very much WIP, please ignore the ugly UI). I use double-taps to simulate right-clicks, therefore all inputs are a bit delayed. Note also that this is using my fix. With Quicksilver 0.3.18 it would not work as intended.
The same WASM is delivered to Mobile and Desktop, so you can also test the behavior in your favorite desktop browser.

@ryanisaacg
Copy link
Owner

That's a good point about PointerDownEvent, I'll make sure that behavior is fixed in the winit web backend.

I don't have any touch hardware other than an iPhone, which can't run WebGL2, so I find it hard to test Quicksilver touch input (hence the obvious, breaking but that's gone unfixed.)

If you're fine with your fork, is this good to close for now?

@jakmeier
Copy link
Author

I don't have any touch hardware other than an iPhone, which can't run WebGL2 [...]

Oh, I see. I didn't know that WebGL2 doesn't run on the iPhone.

If you're fine with your fork, is this good to close for now?

Yes, it's fine with me.

One further question, only partially related to this issue. Given that you want to move this into winit, does that mean mobile support is off the table within Quicksilver? I will require full mobile support for my game anyway and I would like to know if it is a feasible option that I can contribute this to Quicksilver or if I should put this code elsewhere. For me, it doesn't really matter tbh.

@jakmeier jakmeier closed this Aug 26, 2019
@ryanisaacg
Copy link
Owner

ryanisaacg commented Aug 26, 2019

Mobile support would likely depend on further work on Winit and the underlying graphics libraries, but if they add support I guess Quicksilver would get support as well.

@lenscas
Copy link
Contributor

lenscas commented Aug 26, 2019

I don't have any touch hardware other than an iPhone, which can't run WebGL2, so I find it hard to test Quicksilver touch input (hence the obvious, breaking but that's gone unfixed.)

Just putting this here in the hopes it helps: Firefox is able to emulate touch events. I will admit that I never tried it but being able to simulate it is probably a lot better than not being able to test touch input at all.

Open the dev tools -> response design mode -> click the hand at the top of the screen
https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode

@ryanisaacg
Copy link
Owner

@lenscas Thanks!

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.

4 participants