Skip to content

Fix the logic in detectSwipe() incorrectly returning 'false' on swipe and 'true' on tap. #94

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

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

proyection
Copy link

I identified a problem in the swipe detection that was incorrectly returning 'false' when a swipe is detected and 'true' when it was not. This appears to be the reverse of what is intended, as the consuming logic will emulate the 'click' on the 'input' element on receiving a 'false' and will not perform the 'click' on a 'true'.

This bug is interesting in that you would normally not see incorrect behavior on most platforms due to touchestart/touchend events that are 'taps' also sending the desktop 'click' event afterwards (for compatibility reasons). I identified an issue where the 'click' event was not firing in my application on iOS/Safari due to a :hover on the drop zone that prevents the 'click' event happening after a 'touchend'. Like I said, a very particular set of conditions are needed for this bug to manifest to users.

You can see a this warning due to this bug in the console on desktop (tested via Chrome), but users will not notice that there is a problem, typically:

  1. Open the demo page with Chrome,
  2. Open DevTools
  3. Toggle Device Toolbar to emulate mobile/touch device. This allows Chrome to send 'touchstart'/'touchend' events.
  4. On the 'Combo drop/select image only zone' element, swipe on the element.
  5. In the console you should see the warning: "File chooser dialog can only be shown with a user activation." indicating that the input.click() was called after the swipe, when it should not have been called in this case.

The warning is given since activating a .click() on the file input is a protected action requiring a 'click' from the user. The call is not allowed 'programmatically' and only is allowed normally through a click event.

To fix this, I've traded the 'true' and 'false' inside the 'touchend' event 'if' condition. I've also moved the 'evt.stopPropagation()' and 'evt.preventDefault()' calls to occur for taps/non-swipes with the traded positions correctly. That way, as discussed in a previous PR, the 'click' event is not triggered twice after a 'tap' when the platform correctly emulates the desktop 'click'.

Please let me know if you have an questions regarding this PR. (Hopefully I've also correctly built and checked in the 'dist' folder as well).

@AckerApple
Copy link
Owner

Acknowledged.

Please comment off the code you are removing with a date of removal including useful brief comment of problem it caused.

I will attempt to review everything within 7 days. Please feel free to contact me for an update if not provided in that time

@proyection
Copy link
Author

Thanks for the feedback. I've updated the PR by adding the code removed with date and commenting on reasoning for removal.

@AckerApple
Copy link
Owner

Thank you kindly. I will attempt to replicate and confirm your original instructions in issue description with in original time estimate.

Be well

@AckerApple
Copy link
Owner

It’s rare I get to meet someone born yesterday. Your github account was created just yesterday. It prompted me to hard review the build files. Wondering if you might provide some background in good jest. Regardless I will rebuild the files just to be extra safe... I was just curious as to such a fresh account with such assured changes.

@proyection
Copy link
Author

No worries, as I can totally understand receiving a PR from, what appears to be, a complete and total noob ;)

I've been fortunate in my 15+ year developer career to never have to create nor curate an online presence. Nor have I run across an open source library that I've ever experienced a bug that wasn't affecting numerous others that hadn't already started up discussions, submissions, and/or fixes for already. I'm typically content to watching and waiting while others hash out the details so I can focus on my other obligations.

In this case, in my many years of using open source libraries, I was the lucky one with a very particular page structure that prevented normal Safari behavior and no one was bothering to have that same problem as me! My problem is in my app and Safari specific, but, fortunately, you have a bit of code baked into the library that should handle it, but I saw the problem and thought to contribute a fix.

Technically speaking, I think you could probably remove the handlers for touchstart and touchend and remove the swipe detect code and it would continue to work for all normally working apps. It would truly only impact people where the 'proper' tapping events just aren't working correctly sending the 'click' event afterward (like me!).

tl;dr No one was having my same problem and/or reporting it, but I saw the needed fix and felt it was my obligation to create an account and fix it as a user of your library. And, hey, I finally contributed to open source! Thanks for creating this nice library and keeping it maintained.

@AckerApple
Copy link
Owner

AckerApple commented Jan 29, 2021

Haha jolly good show. Recommend not taking too much pride in being an open source hermit but glad you found your place to participate.

I enjoyed the additional details. Thank you kindly good soul. Be well. I will be in touch.

Special request, I like to have some form of visual identity with the people I collaborate with, please consider any profile icon of your liking for better visual memory. I typically save this request for after accepting code to have truly formally worked with a person, however in our chatty case, hey lets get to seeing something that represents you sooner than later ;)

@AckerApple
Copy link
Owner

This pR is still on my mind but had a busy week

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