Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

v3.10.0 broke the onChange for the input radio #288

Closed
cedmax opened this issue Jan 19, 2017 · 7 comments
Closed

v3.10.0 broke the onChange for the input radio #288

cedmax opened this issue Jan 19, 2017 · 7 comments

Comments

@cedmax
Copy link
Contributor

cedmax commented Jan 19, 2017

The changes to fix the onChange being executed twice for the checkboxes (issue: preactjs/preact#358, pr: #247) introduced a problem with the input[type="radio"]: looking at the code it seems like you are deleting the onChange for any input type not checkbox, in favour of an onInput, but the radio wouldn't work with it.

I created a pull request for it too
#287

@cedmax cedmax changed the title v3.10.0 broke the onChange for the inpur radio v3.10.0 broke the onChange for the input radio Jan 19, 2017
@developit
Copy link
Member

Awesome and thanks a bunch for the quick PR!

@mjsisley
Copy link

This breaks when attribute.type === 'file' with components such as https://github.com/odysseyscience/react-s3-uploader that use onChange.

I stopped deletion of the onChange prop with if (attributes.type !== 'file') { delete attributes[props.onchange] };

Firefox triggers the onChange callback twice. (Chrome and Safari work as expected, haven't tried IE/Edge).

@developit
Copy link
Member

I think @cedmax's fix would fix that case too.

@CPatchane
Copy link

CPatchane commented Jan 28, 2017

Agree with @mjsisley, the event onChange (and onInput) for input type='file' still doesn't fire on Chrome and Safari even using the preact-compat last version v3.11.0. But that seems to work on Firefox. Here is the code part I used for the test:

<input
    type='file'
    onChange ={e => console.log(e)}
  />

(That works correctly when using preact only.)

@developit
Copy link
Member

Here's a thought: what if we changed this line from its current implementation:

let attr = nodeName==='input' && /^che|rad/i.test(attributes.type) ? 'onclick' : 'oninput',

...to:

let attr = nodeName==='input' && /^fil|che|rad/i.test(attributes.type) ? 'onchange' : 'oninput',

... think that'd work? IIRC onchange works as desired for checkbox and radio inputs like we want for type="file".

@CPatchane
Copy link

CPatchane commented Jan 29, 2017

Awesome @developit, this fix seems to do the job, onChange events are now correctly fired on Chrome, Safari and Firefox in my tests. And onChange events for checkbox and radio work as desired.

Thanks 👍 . Looking forward to get this fix release 🎉

@developit
Copy link
Member

Fixed on master! Waiting for a resolution to #292 before we release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants