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

Check fabric.window for ontouchstart #4302

Merged
merged 1 commit into from
Sep 16, 2017

Conversation

jordan-acosta
Copy link
Contributor

Fixes #2664

@jordan-acosta
Copy link
Contributor Author

This fix worked for me, tested in Chrome OSX & iOS Safari. Can test on more browsers, devices tonight.

Having trouble running tests on OSX, without this change. Will post tonight with more details if problem persists. Never contributed to this project before, so I may just have botched setup.

@asturur
Copy link
Member

asturur commented Sep 13, 2017

https://github.com/Modernizr/Modernizr/blob/master/feature-detects/touchevents.js#L40

Can you please try firefox, 52 or higher what does it do?

Also unsure, if touchstart is a thing or we should check pointer events?

@jordan-acosta
Copy link
Contributor Author

isTouchSupported gets set to false in Chrome 60, FF 55, and true when each is switched into mobile dev mode.

Checking for ontouchstart is the most reliable approach. That line in Modernizr is pretty old, and the DocumentTouch API is obsolete: https://developer.mozilla.org/en-US/docs/Web/API/DocumentTouch

You raise a good point, though. I'll find time tonight to test across Chrome, FF, Safari, IE & Edge on my Macbook, iPad, Desktop & Android devices. I'll also look into more robust detection options.

The recommended approach these days is to support touch/pointer events in response to interactions -- in addition to mouse/keyboard -- instead of switching between mouse and touch modes. That may be beyond the scope of this fix, but I can see that being useful for the project I'm using Fabric for.

@asturur
Copy link
Member

asturur commented Sep 14, 2017

i m available to pair on this, as i said somewhere else in the issues. I took a touch device with the purpose of fixing things.

@asturur
Copy link
Member

asturur commented Sep 16, 2017

let's get this in.

@asturur asturur merged commit cdd28b0 into fabricjs:master Sep 16, 2017
@jordan-acosta
Copy link
Contributor Author

@asturur , sorry for going missing yesterday, putting out a fire at my day job. Thanks for merging this in!

I'm using Fabric to build a fairly sophisticated vector drawing app for mobile, so rock-solid touch support is a requirement for me.

@asturur
Copy link
Member

asturur commented Sep 16, 2017

Please let me know how event handling works.
Not using it on touch devices ( yet ) makes hard for me build features or fix bugs.

The idea would be searching a better replacement for eventjs if exist, and add a gesture supports.
Did you do some work in this area?

@jordan-acosta
Copy link
Contributor Author

I'll let you know. Looks good so far.

I have done work in this area. I began my career doing mobile and cross-platform sites, though I've since moved on to doing full-stack work (mostly Golang these days.) I'm working on a side project to get back in touch with my front-end roots. While the project itself is proprietary, I'm using it as an excuse to contribute more to open source.

Also, I was initially just using Fabric to prototype, but it's performance has impressed me. The work you guys put into caching and off-canvas rendering really payed off. I want to see just how far I can push Fabric on mobile devices.

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.

2 participants