Disabled inputs should not respond to clicks in IE #6215

nhunzaker · 2016-03-08T13:43:08Z

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 · 2016-03-28T23:29:22Z

src/renderers/dom/client/wrappers/ReactDOMInput.js

@@ -81,7 +82,7 @@ var ReactDOMInput = {
      onChange: inst._wrapperState.onChange,
    });

-    return nativeProps;
+    return DisabledInputUtils.getNativeProps(inst, nativeProps);


Why is DisabledInputUtils.getNativeProps called last here unlike ReactDOMSelect and all other components where it is called inline in the assign call?

Good point. Looking at this again, I've got a new thought regarding assign. I'll post that in a second.

I'm playing around with an idea to try to save that object allocation, but to directly reply to your comment. It should be consistent with the other components. I'll do that now.

Yeah I think we should really try hard to save allocations here.

Okay. I believe I've addressed the original desire with 2eeebf9. I've also rebased from upstream.

gaearon · 2016-03-29T00:29:37Z

src/renderers/dom/client/wrappers/ReactDOMInput.js

@@ -81,7 +82,7 @@ var ReactDOMInput = {
      onChange: inst._wrapperState.onChange,
    });

-    return nativeProps;
+    return nativeProps


facebook-github-bot · 2016-03-29T00:41:56Z

@nhunzaker updated the pull request.

nhunzaker · 2016-03-29T00:56:38Z

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

nhunzaker#1

What do you think?

gaearon · 2016-03-29T00:59:15Z

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

nhunzaker · 2016-03-29T01:00:15Z

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

gaearon · 2016-03-29T01:01:09Z

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

nhunzaker · 2016-03-29T01:04:44Z

src/renderers/dom/client/wrappers/DisabledInputUtils.js

+    // Copy the props, except the mouse listeners
+    var nativeProps = {};
+    for (var key in props) {
+      if (props.hasOwnProperty(key) && !disableableMouseListenerNames[key]) {


There's a small optimization here. No need to check hasOwnProperty if the key is a disabled event handler.

@gaearon I know you just tested this everywhere, but do you mind if I change this to:

for (var key in props) { if (!disableableMouseListenerNames[key] && props.hasOwnProperty(key)) { nativeProps[key] = props[key]; } }

It'll cut hasOwnProperty checks.

Sure if you feel this makes a difference. I wouldn’t worry too much about optimizing this code path because usually you only have a handful of disabled inputs on the page.

Oh it might save a few microseconds. I'll go ahead and do it, since I've got everything open still.

Done it 6a1b3cc

sophiebits · 2016-03-29T01:09:42Z

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

gaearon · 2016-03-29T01:14:24Z

Is it likely somebody relies on onChange firing on disabled elements in IE when it isn’t 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’s a bugfix but minor would be a bit lower risk. Is that what you were saying?

facebook-github-bot · 2016-03-29T01:24:23Z

@nhunzaker updated the pull request.

sophiebits · 2016-03-29T01:25:57Z

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 · 2016-03-29T01:28:10Z

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

sophiebits · 2016-03-29T03:31:54Z

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

nhunzaker · 2016-03-29T14:01:08Z

@spicyj That has been my observation.

sophiebits · 2016-03-29T18:18:18Z

@nhunzaker Thanks for the investigation and for confirming.

nhunzaker · 2016-03-29T18:38:59Z

@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 · 2016-03-31T04:44:10Z

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

nhunzaker · 2016-03-31T15:17:59Z

@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 · 2016-03-31T16:26:15Z

Just updated this with master.

gaearon · 2016-03-31T19:19:09Z

We’ll cut 15 and get back to this asap.

nhunzaker · 2016-03-31T19:47:40Z

Sounds good!

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 · 2016-04-09T00:03:18Z

@nhunzaker updated the pull request.

gaearon · 2016-04-14T18:40:30Z

Don’t see any reason not to get this in. Thanks!

jimfb · 2016-04-14T20:18:29Z

Thanks @nhunzaker and @gaearon!

nhunzaker · 2016-04-14T20:56:47Z

Hizzah!

zpao · 2016-04-22T19:47:34Z

@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 · 2016-04-22T20:29:48Z

@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—it just wasn’t wide enough.

zpao · 2016-04-22T20:30:50Z

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

Disabled inputs should not respond to clicks in IE (cherry picked from commit 36e4fe5)

nhunzaker mentioned this pull request Mar 8, 2016

Fix #1790. Disabled input still clickable in IE11. #3349

Closed

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 8, 2016

nhunzaker force-pushed the nh-fix-disabled-inputs branch from 03b4c59 to 0e63de5 Compare March 17, 2016 00:36

waldreiter mentioned this pull request Mar 20, 2016

Disabled input button should not fire onClick in IE11 #6304

Closed

gaearon reviewed Mar 28, 2016
View reviewed changes

nhunzaker force-pushed the nh-fix-disabled-inputs branch from 0e63de5 to 2eeebf9 Compare March 29, 2016 00:05

gaearon reviewed Mar 29, 2016
View reviewed changes

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

gaearon added GH Review: accepted and removed GH Review: review-needed labels Mar 29, 2016

nhunzaker reviewed Mar 29, 2016
View reviewed changes

nhunzaker force-pushed the nh-fix-disabled-inputs branch from 6a1b3cc to 387a36a Compare March 31, 2016 16:25

nhunzaker added 4 commits April 8, 2016 19:47

Disabled inputs should not respond to clicks in IE

fe739c2

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.

Move getNativeProps usage inline

0200946

Follow style conventions

fc23239

Flip conditional in DisabledInputUtils to cut hasOwnProperty calls

ec2c542

nhunzaker force-pushed the nh-fix-disabled-inputs branch from 387a36a to ec2c542 Compare April 8, 2016 23:48

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

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

zpao added the semver-patch label Apr 22, 2016

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

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

Merge pull request #6215 from nhunzaker/nh-fix-disabled-inputs

3655e30

Disabled inputs should not respond to clicks in IE (cherry picked from commit 36e4fe5)

gaearon mentioned this pull request May 10, 2016

IE11 block change events if disabled #4457

Closed

nhunzaker mentioned this pull request Sep 2, 2016

Move mouse event disabling on interactive elements to SimpleEventPlugin. Related perf tweak to click handlers. #7642

Merged

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Disabled inputs should not respond to clicks in IE #6215

Disabled inputs should not respond to clicks in IE #6215

Conversation

nhunzaker commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 29, 2016

nhunzaker commented Mar 29, 2016

gaearon commented Mar 29, 2016

nhunzaker commented Mar 29, 2016

gaearon commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented Mar 29, 2016

gaearon commented Mar 29, 2016

facebook-github-bot commented Mar 29, 2016

sophiebits commented Mar 29, 2016

nhunzaker commented Mar 29, 2016

sophiebits commented Mar 29, 2016

nhunzaker commented Mar 29, 2016

sophiebits commented Mar 29, 2016

nhunzaker commented Mar 29, 2016

sophiebits commented Mar 31, 2016

nhunzaker commented Mar 31, 2016

nhunzaker commented Mar 31, 2016

gaearon commented Mar 31, 2016

nhunzaker commented Mar 31, 2016

facebook-github-bot commented Apr 9, 2016

gaearon commented Apr 14, 2016

jimfb commented Apr 14, 2016

nhunzaker commented Apr 14, 2016

zpao commented Apr 22, 2016

gaearon commented Apr 22, 2016

zpao commented Apr 22, 2016