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.

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

Conversation

Projects
None yet
7 participants
@jquense
Copy link
Collaborator

commented Jun 7, 2015

while supported, the "input" event is too noisy in IE. It Fires on
placeholder sets, and when an input is focused with a placeholder.

This is an potential alternative fix to #3826

cc @syranide

fixes #3377 and fixes #3484

@jquense

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2015

Ditto this one, I know ya'll are the most busy, but it'd be great to get a fix for this into 0.14.

Let me know if there is anything I can do to help.

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

This comment has been minimized.

Copy link
@probablyup

probablyup Aug 10, 2015

Contributor

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

This comment has been minimized.

Copy link
@jquense

jquense Aug 10, 2015

Author Collaborator

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

@@ -142,7 +152,7 @@ if (ExecutionEnvironment.canUseDOM) {
// IE9 claims to support the input event but fails to trigger it when

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 16, 2015

Contributor

Comment still says IE9, despite documentMode being changed below.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2015

This solution feels simple and clean to me, it avoids having a whole additional browser-dependent code path to follow (ie. better than #3826).

I'll leave it unmerged for a day so people can raise any objections. If there are none, let's ship it.

if (activeElement.attachEvent) {
activeElement.attachEvent('onchange', manualDispatchChangeEvent);
} else {
activeElement.addEventEventListener('change', manualDispatchChangeEvent, false);

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

Can this actually be reached? It's kind of hard to tell at first glance, but this only targets IE8 right? So it shouldn't be.

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 16, 2015

Contributor

@syranide Prior to this change, this code path targeted IE8 and IE9. This change makes IE10 and IE11 also follow the same code path. IE11 does not have attachEvent which is why this is reachable. See #3826 (comment) for context.

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

@jimfb But...

  doesChangeEventBubble = isEventSupported('change') && (
    !('documentMode' in document) || document.documentMode > 8
  );
      if (doesChangeEventBubble) {
        getTargetIDFunc = getTargetIDForChangeEvent;
      } else {
        handleEventFunc = handleEventsForChangeEventIE8;
      }

doesChangeEventBubble should only be false for IE8 and thus it shouldn't go down that path for any other browser as far as I can tell. Or am I reading this wrong.

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 16, 2015

Contributor

I'll defer to @jquense ; Jason, thoughts?

This comment has been minimized.

Copy link
@jquense

jquense Oct 16, 2015

Author Collaborator

I think this is currently unreachable yeah, Its been a few months but I may have just been overzealous refactoring. Let me remove it...

@@ -176,7 +186,11 @@ function startWatchingForValueChange(target, targetID) {
);

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

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

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

This comment has been minimized.

This comment has been minimized.

Copy link
@jquense

jquense Oct 16, 2015

Author Collaborator

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.

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

@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.

This comment has been minimized.

Copy link
@arendjr

arendjr May 12, 2016

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 :)

@@ -142,7 +152,7 @@ if (ExecutionEnvironment.canUseDOM) {
// IE9 claims to support the input event but fails to trigger it when
// deleting text, so we ignore its input events
isInputEventSupported = isEventSupported('input') && (
!('documentMode' in document) || document.documentMode > 9
!('documentMode' in document) || document.documentMode > 11

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

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

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 16, 2015

Contributor

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?

This comment has been minimized.

Copy link
@jquense

jquense Oct 16, 2015

Author Collaborator

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

This comment has been minimized.

Copy link
@syranide

syranide Oct 16, 2015

Contributor

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.

This comment has been minimized.

Copy link
@jquense

jquense Oct 16, 2015

Author Collaborator

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

This comment has been minimized.

Copy link
@probablyup

probablyup Oct 19, 2015

Contributor

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.

opt out of input events for ie 10 and 11
while supported then Input event is too noisy in IE. Firing on
placeholder sets, and when an input is focused with a placeholder.

fixes #3377 and fixes #3484
@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 16, 2015

@jquense updated the pull request.

jimfb added a commit that referenced this pull request Oct 18, 2015

Merge pull request #4051 from jquense/ie-noisy-input-event
opt out of input events for ie 10 and 11

@jimfb jimfb merged commit dd3c447 into facebook:master Oct 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2015

Thanks @jquense!

@zpao

This comment has been minimized.

Copy link
Member

commented Oct 19, 2015

cc @salier as a heads up since you've been dealing with this sort of stuff.

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.