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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disabled inputs should not respond to clicks in IE #6215

Merged
merged 4 commits into from Apr 14, 2016

Conversation

Projects
None yet
6 participants
@nhunzaker
Collaborator

nhunzaker commented Mar 8, 2016

This PR reimplements the fix I provided in #3349 using the latest version of React.

Basically, it creates a general purpose utility for filtering events that should not fire when an input is disabled, then it applies that behavior to the input, select and textarea wrappers.

Third times a charm! I promise I won't let this hang for another year (though I still have one day 馃槃).


Fixes #1790

@gaearon gaearon added this to the 15.x milestone Mar 29, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 29, 2016

@nhunzaker updated the pull request.

facebook-github-bot commented Mar 29, 2016

@nhunzaker updated the pull request.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2016

Collaborator

@gaearon This is where I am at to reduce allocations:

nhunzaker#1

What do you think?

Collaborator

nhunzaker commented Mar 29, 2016

@gaearon This is where I am at to reduce allocations:

nhunzaker#1

What do you think?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 29, 2016

Member

I missed that you are reusing the props when it鈥檚 not disabled. In this case I think it is fine as is.
2eeebf9 solved my concern but maybe somebody else has more.

Member

gaearon commented Mar 29, 2016

I missed that you are reusing the props when it鈥檚 not disabled. In this case I think it is fine as is.
2eeebf9 solved my concern but maybe somebody else has more.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2016

Collaborator

Cool. In the mean time I'll fix that semicolon!

Collaborator

nhunzaker commented Mar 29, 2016

Cool. In the mean time I'll fix that semicolon!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 29, 2016

Member

I tested this on every major browser on OS X, and in IE9+ and Edge on Windows.
It looks good to me so I鈥檓 marking as accepted and setting 15.x milestone.
@zpao and @spicyj, if you have any objections please let me know.

Member

gaearon commented Mar 29, 2016

I tested this on every major browser on OS X, and in IE9+ and Edge on Windows.
It looks good to me so I鈥檓 marking as accepted and setting 15.x milestone.
@zpao and @spicyj, if you have any objections please let me know.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 29, 2016

Member

I don't feel strongly either way. This could theoretically be breaking so probably shouldn't go out in a patch release.

Member

sophiebits commented Mar 29, 2016

I don't feel strongly either way. This could theoretically be breaking so probably shouldn't go out in a patch release.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 29, 2016

Member

Is it likely somebody relies on onChange firing on disabled elements in IE when it isn鈥檛 the case for other browsers? Not trolling, just not sure how often such situations happen in practice.

Maybe we could include it in a minor. We still think it鈥檚 a bugfix but minor would be a bit lower risk. Is that what you were saying?

Member

gaearon commented Mar 29, 2016

Is it likely somebody relies on onChange firing on disabled elements in IE when it isn鈥檛 the case for other browsers? Not trolling, just not sure how often such situations happen in practice.

Maybe we could include it in a minor. We still think it鈥檚 a bugfix but minor would be a bit lower risk. Is that what you were saying?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 29, 2016

@nhunzaker updated the pull request.

facebook-github-bot commented Mar 29, 2016

@nhunzaker updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 29, 2016

Member

From #1790 it wasn't clear to me which browsers are affected. If it is inconsistent across browsers then I'm less worried, but still probably a little better to put it in a minor rather than a patch.

Member

sophiebits commented Mar 29, 2016

From #1790 it wasn't clear to me which browsers are affected. If it is inconsistent across browsers then I'm less worried, but still probably a little better to put it in a minor rather than a patch.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2016

Collaborator

@spicyj I did some work on that here:#1820 (comment)

Collaborator

nhunzaker commented Mar 29, 2016

@spicyj I did some work on that here:#1820 (comment)

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 29, 2016

Member

So no browsers fire mouse events on disabled inputs normally? If so, this seems smart.

Member

sophiebits commented Mar 29, 2016

So no browsers fire mouse events on disabled inputs normally? If so, this seems smart.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2016

Collaborator

@spicyj That has been my observation.

Collaborator

nhunzaker commented Mar 29, 2016

@spicyj That has been my observation.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 29, 2016

Member

@nhunzaker Thanks for the investigation and for confirming.

Member

sophiebits commented Mar 29, 2016

@nhunzaker Thanks for the investigation and for confirming.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2016

Collaborator

@spicyj You're welcome!

As a final note. I forgot I had some work handling this by hacking the SimpleEventPlugin:

nhunzaker/react@nh-fix-disabled-inputs...nhunzaker:nh-disabled-event-plugin

Originally, I couldn't get this to work, but with more knowledge about how the synthetic event system works, I was able to get the test suite to pass. It also eliminates the wrapper around Button and saves some extra memory allocations.

This isn't fully thought through, but does it jump out as a promising lead?

Collaborator

nhunzaker commented Mar 29, 2016

@spicyj You're welcome!

As a final note. I forgot I had some work handling this by hacking the SimpleEventPlugin:

nhunzaker/react@nh-fix-disabled-inputs...nhunzaker:nh-disabled-event-plugin

Originally, I couldn't get this to work, but with more knowledge about how the synthetic event system works, I was able to get the test suite to pass. It also eliminates the wrapper around Button and saves some extra memory allocations.

This isn't fully thought through, but does it jump out as a promising lead?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 31, 2016

Member

That would seem to also work. I don't have a strong feeling on which is better.

Member

sophiebits commented Mar 31, 2016

That would seem to also work. I don't have a strong feeling on which is better.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 31, 2016

Collaborator

@spicyj I want to keep exploring an event plugin, but I think the safest route is to move forward with what is in this PR.

Collaborator

nhunzaker commented Mar 31, 2016

@spicyj I want to keep exploring an event plugin, but I think the safest route is to move forward with what is in this PR.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 31, 2016

Collaborator

Just updated this with master.

Collaborator

nhunzaker commented Mar 31, 2016

Just updated this with master.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 31, 2016

Member

We鈥檒l cut 15 and get back to this asap.

Member

gaearon commented Mar 31, 2016

We鈥檒l cut 15 and get back to this asap.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 31, 2016

Collaborator

Sounds good!

Collaborator

nhunzaker commented Mar 31, 2016

Sounds good!

nhunzaker added some commits Mar 8, 2016

Disabled inputs should not respond to clicks in IE
This commit migrates over the disabled property behavior from
ReactDOMButton into a general purpose disabled event filter. It also
applies that behavior to inputs, selects, and textareas.
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 9, 2016

@nhunzaker updated the pull request.

facebook-github-bot commented Apr 9, 2016

@nhunzaker updated the pull request.

@gaearon gaearon modified the milestones: 15.x, 15.0.x Apr 9, 2016

@gaearon gaearon merged commit 36e4fe5 into facebook:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 14, 2016

Member

Don鈥檛 see any reason not to get this in. Thanks!

Member

gaearon commented Apr 14, 2016

Don鈥檛 see any reason not to get this in. Thanks!

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 14, 2016

Contributor

Thanks @nhunzaker and @gaearon!

Contributor

jimfb commented Apr 14, 2016

Thanks @nhunzaker and @gaearon!

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 14, 2016

Collaborator

Hizzah!

Collaborator

nhunzaker commented Apr 14, 2016

Hizzah!

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Apr 22, 2016

Member

@gaearon Is this safe in 15.0.x? Seems like could be potentially behavior changing. Or does it just fix a bug in some browsers?

Member

zpao commented Apr 22, 2016

@gaearon Is this safe in 15.0.x? Seems like could be potentially behavior changing. Or does it just fix a bug in some browsers?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 22, 2016

Member

@zpao

As far as I tested, it just makes the behavior in older versions of IE (<= 11) consistent with all other browsers that ignore mouse events on disabled inputs. We already had an identical fix for buttons鈥攊t just wasn鈥檛 wide enough.

Member

gaearon commented Apr 22, 2016

@zpao

As far as I tested, it just makes the behavior in older versions of IE (<= 11) consistent with all other browsers that ignore mouse events on disabled inputs. We already had an identical fix for buttons鈥攊t just wasn鈥檛 wide enough.

@zpao zpao added the semver-patch label Apr 22, 2016

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Apr 22, 2016

Member

Cool, thanks! I just wanted to double since I didn't look closely.

Member

zpao commented Apr 22, 2016

Cool, thanks! I just wanted to double since I didn't look closely.

@zpao zpao modified the milestones: 15.0.2, 15.0.x Apr 28, 2016

zpao added a commit that referenced this pull request Apr 28, 2016

Merge pull request #6215 from nhunzaker/nh-fix-disabled-inputs
Disabled inputs should not respond to clicks in IE
(cherry picked from commit 36e4fe5)

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment