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

Changed to !document.documentMode #7594

Merged
merged 1 commit into from
Aug 29, 2016

Conversation

SchleyB
Copy link

@SchleyB SchleyB commented Aug 28, 2016

Line 124 of ChangeEventPlugin.js

isInputEventSupported = isEventSupported('input') && (!('documentMode' in document) || document.documentMode > 11);

Is incorrectly returning false in non-IE browsers when paired with some 3rd party code, specifically

!('documentMode' in document)

Is returning false because 3rd party code is setting document.documentMode to undefined. This is causing these errors on input fields

Uncaught TypeError: activeElement.detachEvent is not a function
TypeError·Uncaught TypeError: Cannot delete property 'value' of #

A better check in this case is

!document.documentMode

As this would still accomplish the intended check while minimizing conflicts with other codebases

I have found a couple of other instances of this issue:
#7421
#5920

@ghost
Copy link

ghost commented Aug 28, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost
Copy link

ghost commented Aug 28, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Aug 28, 2016

Have you verified that IE9 still works as intended?

@SchleyB
Copy link
Author

SchleyB commented Aug 28, 2016

Yes, the check is still correctly returning false in IE9
screenshot 2016-08-28 13 20 33

@aweary
Copy link
Contributor

aweary commented Aug 29, 2016

Seems good to me then 👍

@gaearon
Copy link
Collaborator

gaearon commented Aug 29, 2016

@aweary Please also set the milestone to 15-next so release manager sees the change. (I'm not sure if there is any case when this is unnecessary, @zpao might know.)

@gaearon
Copy link
Collaborator

gaearon commented Aug 29, 2016

Also, since contributors can’t commit, once you accept, you can merge.
When you merge docs you’ll need to put “needs merge to stable” label on it.

@ghost ghost added the CLA Signed label Aug 29, 2016
@aweary aweary merged commit 8397ef5 into facebook:master Aug 29, 2016
@aweary aweary added this to the 15-next milestone Aug 29, 2016
@aweary
Copy link
Contributor

aweary commented Aug 29, 2016

Thanks @gaearon, forgot to do that here 👍

@zpao zpao modified the milestones: 15.3.2, 15-next Sep 8, 2016
zpao pushed a commit that referenced this pull request Sep 15, 2016
@just-boris
Copy link
Contributor

Just curious, but can anybody show me an evidence that Google Tag Manager changes the documentMode?

I have checked their script and have found only read from this property, but not set.

I am paying so much attention to this because it is not okay to blame for something, that he doesn't do actually.

@SchleyB
Copy link
Author

SchleyB commented Sep 20, 2016

@just-boris I thought that it was GTM in this case but I don't have direct evidence to support that so I went ahead and took that out of the original comment. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants