-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Fix] isTouchDevice: improve detection logic #516
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing window.ontouchstart
being null
in my Firefox.
src/utils/isTouchDevice.js
Outdated
@@ -1,4 +1,4 @@ | |||
export default function isTouchDevice() { | |||
return !!(typeof window !== 'undefined' && 'ontouchstart' in window) || | |||
return !!(typeof window !== 'undefined' && window.ontouchstart != null) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What value will window.ontouchstart
have in a browser that supports touch?
However, https://hacks.mozilla.org/2013/04/detecting-touch-its-the-why-not-the-how/ seems to imply that we should be checking a few more things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at what Modernizr does, it looks like they do some more checks too. Even though it is not still a bug I could still update this to have a better check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be super helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that'd be appreciated :-)
I updated the They also use some media queries, but that involves adding a lot of vendor prefixes: https://drafts.csswg.org/mediaqueries/#pointer These two articles seemed relevant: http://www.stucox.com/blog/you-cant-detect-a-touchscreen/ Let me know what you think of this approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
wrap() | ||
.withOverride(() => window, 'DocumentTouch', () => DocumentTouch) | ||
.withGlobal('document', () => new (function Document() {})()) | ||
.it('returns false when document is not an instance of DocumentTouch', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to assert that document is not an instance of DocumentTouch, as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
wrap() | ||
.withOverride(() => window, 'DocumentTouch', () => DocumentTouch) | ||
.withGlobal('document', () => new DocumentTouch()) | ||
.it('returns true when document is an instance of DocumentTouch', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to assert that document is an instance of DocumentTouch, as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'll defer to @majapw for merging.
return !!(typeof window !== 'undefined' && 'ontouchstart' in window) || | ||
!!(typeof navigator !== 'undefined' && navigator.maxTouchPoints); | ||
return ( | ||
!!(typeof window !== 'undefined' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic could probably be split up into if/elses at this point, but nbd
window.ontouchstart
is a defiend property in Firefox with a value of null. This PR fixes theisTouchDevice
function to check thatontouchstart != null
.This PR fixes #515.