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

Close button not working on Chrome #6

Open
josdejong opened this issue May 15, 2015 · 14 comments
Open

Close button not working on Chrome #6

josdejong opened this issue May 15, 2015 · 14 comments

Comments

@josdejong
Copy link

The Close button doesn't work anymore on the latest version of Chrome, nor clicking on the overlay around the modal. Works fine on FireFox though.

To reproduce: On Chrome 42, click "Open Modal 1" in http://jsfiddle.net/aKL44/3/, then try to click on the Close button or outside of the modal.

@mstrater
Copy link

Hmmm. So far I can't reproduce on Chrome Version 42.0.2311.152 m. Are there any more details that might help?

@josdejong
Copy link
Author

I got this reported by a user, and encounter it myself too.

I myself am running Chrome Version 42.0.2311.152 (64-bit), Linux Mint, and my laptop has a touch screen (could this be related to issue #4?).

Via saucelabs I tested the jsfiddle on Chrome 42 on Win 7, Win 8, and Linux. All runs fine there. So it seems not to be related to OS. Could it be touch screen related?

@mofosyne
Copy link

I can confirm the same behaviour. It works in firefox, but not chrome.

My chrome version is Version 42.0.2311.152 m

Javascript console did not show any error.

@josdejong
Copy link
Author

@mofosyne do you have a touch screen too? And what OS are you using?

@mofosyne
Copy link

surface pro 3 - windows 8.1

Touchscreen yes. Didn't use it...

huh? touching the screen works.

@josdejong
Copy link
Author

Ah, same here: the modal works when using touch screen, not when using the mouse.

@kylepaulsen
Copy link
Owner

Hmm this is troublesome. I don't have a touch device (besides my phone that has chrome mobile, but I couldn't reproduce). If anyone has the time (and a touch device with the bug), I would like to know if using the "click" event instead of "mousedown" fixes it. If you get the unminified version, this would be the line to change:

addListener("mousedown", wrappedHandler);

Thanks for reporting and investigating.

@josdejong
Copy link
Author

Ok this is funny. The problem does only occur with the minified file nanomodal.min.js, not with the non-minified file nanomodal.js. When I minify on the commandline, the minified version works fine.

Now I see that nanomodal.js was last updated 2 months ago, whilst the last update of nanomodal.min.js was 4 months ago. Can it be that the minified version is just outdated and is still suffering from the issue solved in #4 ?

@kylepaulsen
Copy link
Owner

Oh yeah... that must be it. Looking at the commit logs, we didn't update the minified version. Oops. Well... I just now re committed the fix, this time actually putting the fix in the source files. I rebuilt and both min and unmin should be ok now. I'll have to update NPM too.

Thanks for finding that.

@josdejong
Copy link
Author

Ok great.

btw I'm not sure why you use two listeners for mousedown and touchstart, a regular click works in all cases (like you proposed). I just tested to be sure and it works fine.

@kylepaulsen
Copy link
Owner

Basically, someone created issue #4 . They have a link to a stack overflow, although it talks about blackberry devices... I thought the problem was with Microsoft surface devices. So you have a surface device and it works with just click? I don't have these devices so I can't test. Anyway... the idea was to hack in a solution that will fire a "click" event no matter what device you have.

@josdejong
Copy link
Author

From all possible events in the web world I would expect the click to just work everywhere, its the most basic one...

The stackoverflow issue seems to address a different problem: the famous 300ms delay between a touch and the click event being fired. If you care about this for nanomodal you indeed need such a special solution that acts on touchstart too. You could also make the library robust against firing the click twice (just ignore consecutive click events as soon as the modal is already closed), and allow firing both click and touchstart.

@kylepaulsen
Copy link
Owner

Are you sure that the click event works on all devices (both touch and multi-input devices)? I was pretty sure I ran into a situation with some device that didn't like the click event but maybe that was some other issue. Anyway if you are sure, I would be happy to simplify the code to just use the click event. I'm not really concerned with a "sluggish" feel from the 300ms problem. My main concern is that it "just works" on as many devices as possible.

@josdejong
Copy link
Author

I'm quite sure about that, onclick is the oldest event handler there is, but I don't know if there are obscure (mobile?) browsers not supporting it.

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

No branches or pull requests

4 participants