Add new onInput event #32
Conversation
'input' is supported in IE9+ and all other browsers according to https://developer.mozilla.org/en-US/docs/Web/API/window.oninput Test Plan: Modified ballmer-peak example to use onInput instead of onKeyUp and tested that it works properly on latest Chrome.
onInput has the advantage that it responds to repeated key events before onKeyUp and is called when modifying the input without the keyboard (such as pasting with the mouse). Test Plan: Opened the ballmer-peak example and docs homepage in Chrome and checked that both examples update whenever the text is changed.
| @@ -211,6 +211,7 @@ function listenAtTopLevel(touchNotMouse) { | |||
| trapBubbledEvent(topLevelTypes.topKeyUp, 'keyup', mountAt); | |||
| trapBubbledEvent(topLevelTypes.topKeyPress, 'keypress', mountAt); | |||
| trapBubbledEvent(topLevelTypes.topKeyDown, 'keydown', mountAt); | |||
| trapBubbledEvent(topLevelTypes.topInput, 'input', mountAt); | |||
There was a problem hiding this comment.
Nice work jumping into the React event subsystem.
We'd still like to support IE8 if possible. Fortunately, our synthetic event system means that we can use keyUp and click to simulate onInput in older browsers such that it works as expected.
However, since users won't expect onInput to work in IE8 anyway, I wonder if it's worth building this into the event plugin.
@jordow -- what do you think? Should we simulate onInput using keyUp and click in some event plugin? I think I'm in favor of it.
There was a problem hiding this comment.
For comparison, Angular's ng-model directive uses input on modern browsers and IE10+ and a combination of the keydown change paste cut events on IE8 and IE9.
https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L411-L444
(Note that they have hasEvent('input') return false in IE9: https://github.com/IgorMinar/angular.js/blob/master/src/ng/sniffer.js#L53-L56.)
|
BTW, I am super stoked to get rid of all the keyUp's in the examples! You rock, thanks for finding high-leverage fixes. |
|
@spicyj Are you at jsconf this week? It would be great to talk about some cool things we can do with this across browsers. |
|
@jordow Sorry, I'm not. (Happy to talk on IRC or something though!) |
Test Plan: With the ballmer-peak example (modified to use input), tested that the percentage updates when adding or deleting text in the field on Chrome and IE9. After adding es5-shim and es5-sham to the ballmer-peak page, IE8 works properly too.
|
Here's a stab at using the synthetic event system to simulate onInput in all browsers, including IE8. Unlike the real input event, this one can get triggered multiple times for the same change and sometimes in cases where there wasn't any input. The only way I can think of to handle this is to store the old value on the DOM element so that we can compare and trigger the event only if there's been a change, but that feels sort of gross to me. I'm also unsure of the setTimeout I used, since it seems that setTimeout isn't used anywhere else in the codebase but I don't know of any other way to get the desired effect here since we need to wait for the browser to process the keydown event before the value attribute is updated. I did verify that when deferring, the AbstractEvent is still released after the event handlers are run. Let me know what you think. |
| case topLevelTypes.topKeyDown: | ||
| key = nativeEvent.keyCode; | ||
| // Ignore command, modifiers, and arrow keys, respectively | ||
| if (key === 91 || (15 < key && key < 19) || (37 <= key && key <= 40)) { |
There was a problem hiding this comment.
Might be a bit clearer what's going on if these were constants
|
@spicyj this is awesome and going to be a huge improvement to the framework. I've been wanting to do this for a while. The overall structure of this code is great. Thanks for making it a new event plugin. We don't have a precedent for doing the deferred stuff yet. I'd want @jordow's opinion on how to go about that. But we may be able to make this work without deferring. I am noticing that the events we need to defer are cut, paste, and keyDown. What if instead we got rid of those and just used keyUp and click and threw out all of the deferred stuff? While it'll be slightly slower (waiting for keyUp to reconcile rather than doing it immediately on keyDown), I think it gets us 90% of the way there without forcing us to introduce this deferred concept. Remember, since this is React, reconciles are generally considered cheap so if we trigger a few extra input events even though the content hasn't changed our reconciler will figure it out and make it a no-op. Finally, I'd like to get test coverage on the event plugin. If you feel like tackling it, great, but I'm comfortable taking this pull without one and I can add a test case upstream. @ide @yungsters @jordow whatcha think? |
|
@spicyj You keep reading my mind. I'll take a closer look at this tonight. |
| var abstractEventTypes = { | ||
| input: { | ||
| phasedRegistrationNames: { | ||
| bubbled: keyOf({onInput: null}), |
There was a problem hiding this comment.
Because we're bound to not be able to replicate the onInput behavior exactly as it occurs in IE, we should name these events something that makes it clear that this is a synthetic event. Maybe onTextChanged or onInputChanged?
There was a problem hiding this comment.
Perhaps onValueChange? (Change seems better than Changed for consistency with existing DOM event naming.)
This reverts commit 580e8f0.
|
Looks fine. I'll let @zpao merge this in just in case he wants to test the repo syncing flow. |
|
I'm actually going to hold off on landing this just because it changes docs and we need to be able to update the docs with the current API. It's going to involve branches and making sure everybody building docs knows the process... it'll be gross but you don't have to do anything, it'll be on me/us. I will merge it in ASAP though. |
Break the text into next line for long props
The
inputevent is supported in IE9+ and all other browsers according to https://developer.mozilla.org/en-US/docs/Web/API/window.oninput. onInput has the advantage that it responds to repeated key events before onKeyUp and is also called when modifying the input without the keyboard (such as pasting with the mouse).I also modified the two examples that use onKeyUp (markdown, ballmer-peak) to use onInput but am happy to revert that commit if for some reason you prefer to keep using onKeyUp.