-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
iOS 11.3 fix #416
iOS 11.3 fix #416
Conversation
return; | ||
} | ||
|
||
const isUsingSafari11: RegExp = /AppleWebKit.*Version\/11/g; |
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.
You know it is a bad day when you need to do some browser sniffing 😢
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.
LGTM.
} | ||
|
||
// Adding a persistent event handler | ||
window.addEventListener('touchmove', (event: TouchEvent) => { |
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.
Is there any part of the lifecycle where we would want to remove the event listener? E.g. on unmount?
return true; | ||
})(); | ||
|
||
if (!shouldBlock || 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.
Nit: The typeof window
part of the conditional won't get hit since it's being checked within shouldBlock
.
|
||
const shouldBlock: boolean = (() => { | ||
// All browsers on iPhone or iPad | ||
const target: RegExp = /OS\s11_\d\slike\sMac\sOS\sX/g; |
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.
As discussed with @seancurtis, I will remove this check. This will match against iphones and ipads - but not other webkit based browsers. Given that the cost of this is a single event listener I am okay to add it redundantly to other browsers which do not strictly need it in order to avoid other webkit browsers slipping through the cracks
Webkit does not allow event.preventDefault() in dynamically added handlers (i.e., a handler added to 'touchmove' after handling a 'touchstart'), so we add a permanent 'touchmove' handler to get around this. webkit bug: https://bugs.webkit.org/show_bug.cgi?id=184250 based on: atlassian/react-beautiful-dnd#416
The frustration in your comments is understandable! I’m currently considering migrating from Dragula to this package because I cannot get this to work with it. I’d much prefer to not do this though. I’m wondering if you might provide a vanilla js explanation for your thinking behind this implementation, at a glance, I get it but I’ve not seen the |
Fixes #413
This is a hack to get things working again. I cannot seem to find the exact root cause. There is a bug that I have noticed and raised with webkit. If that is fixed then I can remove the hack. However, my find does not explain why it worked on iOS 11.2. I am trying really hard to find an answer