Skip to content
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.

Sign up for GitHub

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

opt out of input events for ie 10 and 11 #4051

Merged
merged 1 commit into from
Oct 18, 2015
Merged

opt out of input events for ie 10 and 11 #4051

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,16 @@ function handleEventsForChangeEventIE8(
var isInputEventSupported = false;
if (ExecutionEnvironment.canUseDOM) {
// IE9 claims to support the input event but fails to trigger it when
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment still says IE9, despite documentMode being changed below.

// deleting text, so we ignore its input events
// deleting text, so we ignore its input events.
// IE10+ fire input events to often, such when a placeholder
// changes or when an input with a placeholder is focused.
isInputEventSupported = isEventSupported('input') && (
!('documentMode' in document) || document.documentMode > 9
!('documentMode' in document) || document.documentMode > 11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no documentMode greater than 11 (and probably never will).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there never is, then maybe there is no harm? I suppose I'd be fine either way, doesn't really matter to me, hard to tell when future proofing is beneficial/detrimental. @syranide let me know if you have a strongly preferred solution here. Maybe @spicyj has a strong feeling on this topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while true, Microsoft has said that a lot and then added another document mode :P it was more defensive than anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentMode does not exist in MS Edge. Also, it seems weird to assume this would somehow be fixed in this theoretical version 12, it doesn't seem right to have a special-case for a browser we know nothing about.

PS. So yeah, doesn't really matter to me and probably won't ever matter, but it seems weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's totally fair, no good way to say this is helpful "future proofing"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, IE will continue to exist in Enterprise installs for backward compat with ActiveX-based crud. So it's possible the version could be incremented at some point.

);
}

/**
* (For old IE.) Replacement getter/setter for the `value` property that gets
* (For IE <=11) Replacement getter/setter for the `value` property that gets
* set on the active element.
*/
var newValueProp = {
Expand All @@ -162,7 +164,7 @@ var newValueProp = {
};

/**
* (For old IE.) Starts tracking propertychange events on the passed-in element
* (For IE <=11) Starts tracking propertychange events on the passed-in element
* and override the value property so that we can distinguish user events from
* value changes in JS.
*/
Expand All @@ -176,11 +178,15 @@ function startWatchingForValueChange(target, targetID) {
);

Object.defineProperty(activeElement, 'value', newValueProp);
activeElement.attachEvent('onpropertychange', handlePropertyChange);
if (activeElement.attachEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should prefer to use addEventListener when it's available, or is there a specific reason not to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe https://github.com/facebook/react/pull/4051/files#r36645968 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, again more defensive than anything, I know this event works with attachEvent in browsers that support it per the docs and experience, whereas with addEventListener, I only confirm it works in ie11 since that's all I have access too (that despite the ambiguity of the MS docs on support).

It may be that its supported in IE11 just because attachEvent doesn't exist anymore. Also it was already using attachEvent in IE9 even tho addEventListener is supported, so I didn't want to introduce a change for the older code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jquense I have access to IE8-11, but yeah, if this works and was this way before, let's just keep it that way. No point messing with legacy browsers over a nit.

Copy link

@arendjr arendjr May 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I'm working on a related bug now, and it appears that indeed "onpropertychange" works with attachEvent(), but "propertychange" doesn't with addEventListener(). At least in IE11 in IE10 Compatibility Mode. FWIW :)

activeElement.attachEvent('onpropertychange', handlePropertyChange);
} else {
activeElement.addEventListener('propertychange', handlePropertyChange, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work. According to MSDN, the propertychange event is only fired when listened via the attachEvent API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I saw that as well, but right below that it also demonstrates it using addEventListener, and it does seem to work in my testing. It could probably stand further testing

}
}

/**
* (For old IE.) Removes the event listeners from the currently-tracked element,
* (For IE <=11) Removes the event listeners from the currently-tracked element,
* if any exists.
*/
function stopWatchingForValueChange() {
Expand All @@ -190,7 +196,12 @@ function stopWatchingForValueChange() {

// delete restores the original property definition
delete activeElement.value;
activeElement.detachEvent('onpropertychange', handlePropertyChange);

if (activeElement.detachEvent) {
activeElement.detachEvent('onpropertychange', handlePropertyChange);
} else {
activeElement.removeEventListener('propertychange', handlePropertyChange, false);
}

activeElement = null;
activeElementID = null;
Expand All @@ -199,7 +210,7 @@ function stopWatchingForValueChange() {
}

/**
* (For old IE.) Handles a propertychange event, sending a `change` event if
* (For IE <=11) Handles a propertychange event, sending a `change` event if
* the value of the active element has changed.
*/
function handlePropertyChange(nativeEvent) {
Expand Down Expand Up @@ -229,7 +240,6 @@ function getTargetIDForInputEvent(
}
}

// For IE8 and IE9.
function handleEventsForInputEventIE(
topLevelType,
topLevelTarget,
Expand All @@ -238,7 +248,7 @@ function handleEventsForInputEventIE(
// In IE8, we can capture almost all .value changes by adding a
// propertychange handler and looking for events with propertyName
// equal to 'value'
// In IE9, propertychange fires for most input events but is buggy and
// In IE9-11, propertychange fires for most input events but is buggy and
// doesn't fire when text is deleted, but conveniently, selectionchange
// appears to fire in all of the remaining cases so we catch those and
// forward the event if the value has changed
Expand Down