search

kylepaulsen / NanoModal

A small, self-contained JavaScript modal library with some extra features.
MIT License
47 stars 5 forks source link

Close button not working on Chrome #6

Open josdejong opened 9 years ago

josdejong commented 9 years ago

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 commented 9 years ago

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

josdejong commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

mofosyne commented 9 years ago

surface pro 3 - windows 8.1

Touchscreen yes. Didn't use it...

huh? touching the screen works.

josdejong commented 9 years ago

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

kylepaulsen commented 9 years ago

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: https://github.com/kylepaulsen/NanoModal/blob/5acbf5fb6d50cda110cfdca283a6fcea5d7a7e37/nanomodal.js#L55

Thanks for reporting and investigating.

josdejong commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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.