Change events not fired for radios or checkboxes when clicking their labels (on touch devices)

[munkyjunky]

Clicking on a label doesn't propagate through to input fields on mobile devices. We tested this with a label next to a hidden radio button. This did work with a click event on desktop chrome, but not on mobile devices using onTouchStart

In particular, this didn't work on iOS 9.0.2.

<input type='radio' id='test-input' style={{ display: 'none' }}/>
<label htmlFor='test-input'>A test label value</label>

[JakeSidSmith]

Note to self: https://github.com/JakeSidSmith/react-fastclick/blob/master/index.js#L61

[JakeSidSmith]

Is the problem that the onChange handler is not triggered?

[JakeSidSmith]

Solved in #23, but not sure if this is the best way to handle it. I don't think I'll be able to keep the library as backwards compatible as I'd like to.

[munkyjunky]

It was the onChange handler on the input field that wasn't firing.

Looking at #23 I don't think using the test utils is a great way to go with a fix, however since this is a bit of an edge case, and we've since refactored that section of the code out to use buttons (thanks to #20) is it worth making this change now, especially if you're losing some backwards compatibility?

Additionally, we can't even install the branch because it needs a newer version of React that is in our project, and we can't update it, meaning we can't test this works with our issue!

[JakeSidSmith]

I'll have to have a think about this one. Unfortunately, due to React's event system, using dispatchEvent does not trigger React's internal events and the handler is therefore not triggered.

I'll take a look at how Simulate works and see if I can make a small backward compatible one. :)

[JakeSidSmith]

Note to self: https://github.com/facebook/react/blob/3a4e1dbb5b2957875cab6af11a5e5934e125a0d8/src/test/ReactTestUtils.js#L518

[JakeSidSmith]

@munkyjunky What version of React are you using?

[JakeSidSmith]

After picking through React's source code, and playing with several, usually inaccessible modules, realising that I couldn't use these anyway because 1 or 2 don't exist in React versions before 0.14, I gave up and decided to see if I could fix this in react-fastclick itself somehow...

...it was shortly after this, I discovered a bug with react-fastclick:

return !touchEvents.touched || new Date().getDate() > touchEvents.lastTouchDate + TOUCH_DELAY;

Should actually be

return !touchEvents.touched && new Date().getTime() > touchEvents.lastTouchDate + TOUCH_DELAY;

This was causing the click event to fire twice when clicking on labels, which I believe is why I event.preventDefault() inside my focusAndCheck function when manually checking checkboxes (to prevent them from being checked and immediately unchecked).

The line in question worked fine with every other use case, because touchEvents.touched was true for every other case that mattered, as no other cases, besides checkboxes and radios, relied on the clicked element being different from the one receiving an event.

I think fixing this line, and removing all my calls to event.preventDefault() could fix the bug.

I'm going to create a branch with these changes, and it'd be great if you could give them a quick test with some of your apps / sites. I will of course test as thoroughly as I can also. I'm worried these changes could have undesirable effects on other functionality.

[munkyjunky]

I'm using react 0.14.6. If you manage to get it sorted I'm happy to help with testing where possible!

[JakeSidSmith]

25 seems to have fixed it for everything I've tested so far (though there may be edge cases). If you could try it with your projects, that'd be awesome. :)

[JakeSidSmith]

@munkyjunky had a chance to test out that branch?

[munkyjunky]

Not yet - I'll try give it a go today or tomorrow

(/cc @brins0)

[JakeSidSmith]

@munkyjunky update? :)

[munkyjunky]

Sorry! I didn't realise this had been 3 weeks since I said I'd give this a test! Just gone over it now, everything is working as expected in my project. As I mentioned previously we'd refactored out the bit of code that did this originally, so I can't check it against the original issue - but I can say nothing appears to be broken.

[JakeSidSmith]

Cool. I think I'll take this opportunity to add some tests (which I've been meaning to do for a while) and then hopefully will catch anything manual testing has missed. :)

[dahjelle]

Ran into this same issue today when I switched from fastclick to react-fastclick. So far #25 seems to be working okay for us; will report back with any problems I discover.

[dahjelle]

Ran into an issue with #25 on desktop Chrome and <a> tags with onClick events. touchEvents.lastTouchDate doesn't get initialized if there aren't any touch events, so noTouchHappened always returns true in onMouseEvent because of comparing against NaN in noTouchHappened.

It looks like changing the initialization to:

  var touchEvents = {
    downPos: {},
    lastPos: {},
    lastTouchDate: new Date().getTime()
  };

fixes it for me; I can imagine wanting lastTouchDate: new Date().getTime() - TOUCH_DELAY instead, but I haven't dug in far enough to understand the delay bit.

[JakeSidSmith]

Ahh, I see what you mean @dahjelle. Have fixed this in #26

Will be continuing on that branch / PR from now on. :)

[JakeSidSmith]

Anyone care to review this PR? :D https://github.com/JakeSidSmith/react-fastclick/pull/26

[JakeSidSmith]

@dahjelle @munkyjunky ?

[joeruello]

Bump, same across this today. Would love a fix!

[dahjelle]

@JakeSidSmith I can't say I can do a real review here—I don't understand the technique enough for that—but I did test out #26 in all the situations I was having problems with, and it seems to work great! Thank you! Looking forward to release.

[munkyjunky]

I'll try and get this tested in my project tomorrow, but having a quick look through the code I do like the addition of tests & code coverage!

[JakeSidSmith]

Added some comments to the PR to explain my changes. :)