Fix #1790. Disabled input still clickable in IE11. #1820
Conversation
|
|
||
| // Store a reference to the <input> `ReactDOMComponent`. | ||
| var input = ReactDOM.input; | ||
|
|
||
| var mouseListenerNames = keyMirror({ | ||
| onClick: true, |
There was a problem hiding this comment.
I'd like to bring up the question again here, shouldn't all input events be in the list? Such as onTap, etc?
PS. Could probably also benefit from putting this in a separate file and reusing for both locations (including the filter logic).
There was a problem hiding this comment.
That's a fair question -- I'd need to hunt down the specification for disabled elements to speak with any confidence. As for placing this in a separate file... yes -- but is there an existing location it should go?
There was a problem hiding this comment.
Ah, excellent. Perhaps it should be explained in a comment then.
|
@syranide I added touch event coverage and pulled the filtering into a separate file. This was sort of a stab in the dark, let me know if I'm on the right track and I'll throw in test coverage for touch events. |
|
|
||
| for (var key in props) { | ||
| if (props.hasOwnProperty(key) && | ||
| (!props.disabled || !disabledEventBlacklist[key])) { |
There was a problem hiding this comment.
!props.disabled should probably be moved to the top and return the original props if it isn't disabled?
There was a problem hiding this comment.
Though we still need to clone the props. A couple of tests actually fail if we don't.
|
Leaving a public note for myself: Are keyboard events properly trapped too? If not, what events should actually fire on a disabled input/button? |
|
Also I think I need to check coverage for |
|
Okay! Here's how I tested: https://gist.github.com/nhunzaker/688708c06e775228afeb I verified that events fired as would be expected without the Chrome/Safari:Allow all keyboard events and focus/blur events. Android 4.4/iOS 7.1Allow focus/blur, keyboard, and touch events. So basically, Webkit desktop + all touch events FirefoxMousemove, wheel IE9-11 (emulated, windows 8)None IE8 (windows 7)None Across the board, mouse events did not trigger. If we do anything, we should filter out mouse events. Focus events are an interesting story because it is impossible in Safari/Chrome to focus on a disabled input, however the event fires; it's possible this is a flaw in my testing. As for keyboard events. Safari/Chrome allow them, however again, I do not know in the wild if it is even possible to trigger them if an input can't receive focus. The specific example I made allows keyboard events if you manually trigger them, however it isn't possible to actually press a key within the input. Now for touch events. We should leave these alone. They clearly fire when a disabled input is touched in both Android and Safari (http://codepen.io/nhunzaker/pen/veCsF). VerdictNothing new... 😿. Filter out click, double click, mousedown, mouseup, and mousemove. But we should definitely be doing this for textarea and select. Do we know if they have coverage right now? |
|
Any additional thoughts here? |
Takes some code from ReactDOMButton in order to solve the same problem within ReactDOMInput.
|
@syranide I just updated this with the latest from master. Is there anything outstanding to bury this? |
|
@nhunzaker ... @zpao or some other dev should weigh in on this. |
|
cc @yungsters |
|
(Also, sorry! We're getting out of a large backlog of issues/PRs and unfortunately some have slipped through the cracks completely 😞) |
| @@ -0,0 +1,52 @@ | |||
| /** | |||
| * Copyright 2013-2014 Facebook, Inc. | |||
There was a problem hiding this comment.
2014 should become 2015 before this is merged.
There was a problem hiding this comment.
It's the old license even :)
|
@zpao I'd be happy to update it, but is this PR still relevant? |
|
Whelp, for whatever reason I deleted my fork. Nice... I'll resubmit this. |
|
Closing this, please refer to #3349. |
This PR Takes some code from
ReactDOMButtonin order to solve the same problem and brings it over toReactDOMInput.I'll admit to complete duplication here with how this was solved in
ReactDOMButton. Where would the best place be to put the black list of mouse events for when an input/button is disabled?Fix #1790