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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new textChange event: input + IE shim #75

Merged
merged 4 commits into from Jun 12, 2013

Conversation

Projects
None yet
4 participants
@sophiebits
Member

sophiebits commented Jun 9, 2013

IE8 doesn't support oninput and IE9 supports it badly but we can do almost a perfect shim by listening to a handful of different events (focus, blur, propertychange, selectionchange, keyup, keydown).

This always triggers event handlers during the browser's event loop (not later in a setTimeout) and after the value property has been updated.

The only case I know of where this doesn't fire the event immediately is if (in IE8) you modify the input value using JS and then the user does a key repeat, in which case we fire the event on the second keydown.

Test Plan:
Modify ballmer-peak example to add es5-shim and to use onTextChange instead of onInput. In IE8, IE9, and latest Chrome, make sure that the event is fired upon:

  • typing normally,
  • backspacing,
  • forward-deleting,
  • cutting,
  • pasting,
  • context-menu deleting,
  • dragging text to reorder characters.

After modifying the example to change .value, make sure that the event is not fired as a result of the changes from JS (even when the input box is focused).

Questions:

  • Does it make sense to pass on the native event? Since it could be one of seven different types, I'm not sure that application code would be able to do anything useful with it.
  • The propertychange event doesn't bubble so we can't just attach it to document with the other events but is it preferable to have it go through ReactEventTopLevelCallback like the other events even though we're the only ones who'll be interested? Doing so will get rid of the manual enqueueAbstractEvents/processAbstractEventQueue calls I have but I imagine it'll be slower and won't give any other advantages.
  • Is the IE9 sniffing code for isInputSupported reasonable? I don't know the preferred way here to handle situations like this.
  • IE loses the textarea cursor position when updating the textarea contents during the selectionchange event and having ReactInputSelection attempt to restore the contents doesn't seem to help so I've disabled the event on textarea for now. Ideas?
Add new textChange event: input + IE shim
IE8 doesn't support oninput and IE9 supports it badly but we can do
almost a perfect shim by listening to a handful of different events
(focus, blur, propertychange, selectionchange, keyup, keydown).

This always triggers event handlers during the browser's event loop (not
later in a setTimeout) and after the value property has been updated.

The only case I know of where this doesn't fire the event immediately is
if (in IE8) you modify the input value using JS and then the user does a
key repeat, in which case we fire the event on the second keydown.

Test Plan:
Modify ballmer-peak example to add es5-shim and to use onTextChange
instead of onInput. In IE8, IE9, and latest Chrome, make sure that the
event is fired upon:

* typing normally,
* backspacing,
* forward-deleting,
* cutting,
* pasting,
* context-menu deleting,
* dragging text to reorder characters.

After modifying the example to change .value, make sure that the event
is not fired as a result of the changes from JS (even when the input box
is focused).
@zpao

This comment has been minimized.

Member

zpao commented Jun 10, 2013

// IE9 claims to support the input event but fails to trigger it when deleting
// text, so we ignore its input events
var isInputSupported = isEventSupported('input') && (

This comment has been minimized.

@petehunt

petehunt Jun 10, 2013

Contributor

Please use ExecutionEnvironment.canUseDOM such that this module could be required and not fatal if there is no document symbol in scope. I'm not sure if it'll get required in practice or not, but a good idea nonetheless.

This comment has been minimized.

@sophiebits

sophiebits Jun 11, 2013

Member

Done. (I had canUseDOM originally but lost it somewhere along the way.)

var isInputSupported;
if (ExecutionEnvironment.canUseDOM) {
isInputSupported = isEventSupported('input') && (
!("documentMode" in document) || document.documentMode > 9

This comment has been minimized.

@yungsters

yungsters Jun 12, 2013

Contributor

This check looks good to me.

@yungsters

This comment has been minimized.

Contributor

yungsters commented Jun 12, 2013

Does it make sense to pass on the native event? Since it could be one of seven different types, I'm not sure that application code would be able to do anything useful with it.

Yes, keep passing it along. It's up to the end user to do what they will with it, but I have a new set of "synthetic events" that will allow us to normalize the API for both DOM and custom events like the one you're adding.

nativeEvent
);
EventPropagators.accumulateTwoPhaseDispatches(abstractEvent);
EventPluginHub.enqueueAbstractEvents(abstractEvent);

This comment has been minimized.

@yungsters

yungsters Jun 12, 2013

Contributor

Add a comment about how propertychange does not bubble so we cannot listen to this using top-level event delegation and have to enqueue and process manually. (By the way, @jordow, can you explain why we need two steps for enqueuing and processing instead of one?)

);
Object.defineProperty(topLevelTarget, 'value', newValueProp);
topLevelTarget.attachEvent('onpropertychange', handlePropertyChange);

This comment has been minimized.

@yungsters

yungsters Jun 12, 2013

Contributor

Change topLevelTarget.attachEvent to activeElement.attachEvent so it's easier for readers to find the detachEvent call below.

// changed from JS so we redefine a setter for `.value` that updates our
// activeElementValue variable, allowing us to ignore those changes
if (hasInputCapabilities(topLevelTarget)) {
activeElement = topLevelTarget;

This comment has been minimized.

@yungsters

yungsters Jun 12, 2013

Contributor

I've always had the impression that focus and blur events are flakey (e.g. if the OS steals focus from the browser, etc.). Is it ever possible for the blur event to not get fired before another focus? If activeElement, maybe we should do the cleanup below.

} else {
return;
}

This comment has been minimized.

@yungsters

yungsters Jun 12, 2013

Contributor

The conditionals are correct, but a little hard to follow. Do you think this would be any better?

if (isInputSupported) {
  if (topLevelType !== topLevelTypes.topInput) {
    return;
  }
  // ...
} else {
  if (topLevelType === topLevelTypes.topFocus) {
    // ...
    return;
  }
  if (topLevelType === topLevelTypes.topBlur) {
    // ...
    return;
  }
  if (topLevelType !== topLevelTypes.topSelectionChange &&
      topLevelType !== topLevelTypes.topKeyUp &&
      topLevelType !== topLevelTypes.topKeyDown) {
    return;
  }
  // ...
}

Or better yet...

var extractEvents;
if (isInputSupported) {
  extractEvents = function() {
    // Modern browsers...
  };
} else {
  extractEvents = function() {
    // IE<10...
  };
}

sophiebits added some commits Jun 12, 2013

yungsters added a commit that referenced this pull request Jun 12, 2013

Merge pull request #75 from spicyj/textchange
Add new textChange event: input + IE shim

@yungsters yungsters merged commit dfd76be into facebook:master Jun 12, 2013

1 check passed

default The Travis CI build passed
Details

andreypopp pushed a commit to andreypopp/jsxx that referenced this pull request Feb 2, 2014

Upgrade TextChangeEventPlugin to ChangeEventPlugin and support more f…
…orm elements

Upgrade `TextChangeEventPlugin` to be the `onChange` event that React
fires. In React, `onChange` will now fire when `input` fires for form elements in
modern browsers.

Handle this for:

  input[type=text]
  input[type=password]
  input[type=checkbox]
  input[type=radio]
  textarea
  select

Support:

- OSX Chrome
- OSX Safari
- OSX Firefox
- Win 7 / IE8
- Win 7 / IE9
- Win 7 / IE10

Everything works but caret selection / placement differs from browser to
browser.

For <select> elements, the event is fired with `change`. This is a
conscious decision, even though in some browsers (OSX firefox, IE), it
can be argued that the event should fire more due to how the UI looks.

Builds on facebook/react#75, which handled only
text inputs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment