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

Fallback to event.srcElement for IE9 #12976

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jun 4, 2018

It looks like we accidentally removed a fallback condition for the event target in IE9 when we dropped some support for IE8. This commit adds the event target specific support code back to getEventTarget.js

Fixes #12506

Test Plan

  1. Open IE9
  2. Visit http://react-nh-fix-ie9-target.surge.sh/number-inputs
  3. Try to enter text into the inputs

Local Behavior: Includes this fix. event.target falls back to event.srcElement and text correctly updates
16.4.0 Behavior: Text does not update because event.target.value references the window, which is always undefined.

http://react-nh-fix-ie9-target.surge.sh/number-inputs?version=16.4.0

Note: It's a bummer that IE9 can't fetch our latest tags... I'll try to do something about that.

@nhunzaker
Copy link
Contributor Author

Grr. Looks like CI got garbled on Danger.

@nhunzaker nhunzaker requested a review from aweary June 4, 2018 11:28
let target = nativeEvent.target || window;
// Fallback to nativeEvent.srcElement for IE9
// https://github.com/facebook/react/issues/12506
let target = nativeEvent.target || nativeEvent.srcElement || window;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have an internal unit test for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really like to track down the reason for this so we don't accidentally remove it again as well. Everything I can find suggests that srcElement should not be needed in IE9, so I'm wondering where the issue is coming from. I haven't found much some-places suggest that when attachEvent is used it follows the ie8 event model (with target undefined), I wonder if some branches are still hitting attachEvent?

Copy link
Contributor

Choose a reason for hiding this comment

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

the change polyfill uses it, Perhaps that's the root cause? I think we could use addEventListener there...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... I actually remove that listener in #12505. I'll hunt!

Copy link
Contributor

Choose a reason for hiding this comment

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

My other guess is that maybe it's like a quirksmode sort of thing, where folks are seeing something in document mode they don't realize they are in, in ie9 :/

@nhunzaker nhunzaker mentioned this pull request Jun 4, 2018
@nhunzaker
Copy link
Contributor Author

@jquense I bow to your mastery of the DOM. Using addEventListener addressed the issue as well obviated my PR to fix undo in IE9 (#12505).

@nhunzaker
Copy link
Contributor Author

Well actually... 🤦‍♂️ I didn't update the event listener name when I went to addEventListener. Here we go again...

@nhunzaker
Copy link
Contributor Author

@jquense looks like propertychange only fires if you assign input.onpropertychange or use attachEvent.

Still: the false positive suggests that there's something goofy going on here. I'll investigate why the onpropertychange handler appears not to be necessary (also demonstrated in #12505).

@jquense
Copy link
Contributor

jquense commented Jun 4, 2018

I was afraid it'd all coming tumbling down if we moved one piece :p I can try and take a closer look as well a bit later if it helps at all

@aweary
Copy link
Contributor

aweary commented Jun 5, 2018

If we get rid of onpropertychange in #12505 then we shouldn't need this fix right?

@nhunzaker
Copy link
Contributor Author

@aweary Correct. This is just temporarily addresses the event target issue in IE9 until we can figure out if onpropertychange can go.

@gaearon
Copy link
Collaborator

gaearon commented Jun 5, 2018

It wasn't accidental, I removed it in #11515 because I thought it's IE8-only. Sorry.

@nhunzaker
Copy link
Contributor Author

@gaearon we also approved it :D. It looks like we need this as long as we use onpropertychange, which emits an MSEventObject circa IE8.

It sounds like (from #12505) removing onpropertychange is going to be a can of worms. I don't know when the next patch release is, but it would be cool to get this out in the next patch, if we can.

CI is failing because of danger. Is there a way to fix that?

@gaearon
Copy link
Collaborator

gaearon commented Jun 5, 2018

Does rebasing help?

It looks like we accidentally removed a fallback condition for the
event target in IE9 when we dropped some support for IE8. This commit
adds the event target specific support code back to getEventTarget.js

Fixes facebook#12506
@aweary
Copy link
Contributor

aweary commented Jun 5, 2018

Fixing this for IE9 is pretty low priority (it's been broken for a long time and nobody reported it), so it may be worth spending more time on a better solution if we think we can find one.

@nhunzaker
Copy link
Contributor Author

@gaearon sure enough.

@nhunzaker
Copy link
Contributor Author

@aweary This isn't much of a change to revert if we sort that out, but I think it's probably fine if we want to just worry about what to do with onpropertychange.

joshd-infotrack added a commit to joshd-infotrack/react that referenced this pull request Jun 7, 2018
@gaearon gaearon merged commit 4ac6f13 into facebook:master Jun 11, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 11, 2018

Let's just get this in for now, it's a simple fix and now has a GH issue link.
Happy to merge a more comprehensive solution later.

bors bot referenced this pull request in mozilla/delivery-console Jun 13, 2018
193: Update dependency react to v16.4.1 r=magopian a=renovate[bot]

This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1`



<details>
<summary>Release Notes</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`https://github.com/facebook/react/pull/12911`))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`https://github.com/facebook/react/pull/12135`))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`https://github.com/facebook/react/pull/12996`))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`https://github.com/facebook/react/pull/12939`))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`https://github.com/facebook/react/pull/12925`))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`https://github.com/facebook/react/pull/12976`))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`https://github.com/facebook/react/pull/12966`))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`https://github.com/facebook/react/pull/12985`), [@&#8203;gaearon] in [#&#8203;13019](`https://github.com/facebook/react/pull/13019`))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`https://github.com/facebook/react/pull/13017`))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`https://github.com/facebook/react/pull/13030`))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot referenced this pull request in mythmon/corsica-tree-status Jun 14, 2018
25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`https://github.com/facebook/react/pull/12911`))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`https://github.com/facebook/react/pull/12135`))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`https://github.com/facebook/react/pull/12996`))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`https://github.com/facebook/react/pull/12939`))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`https://github.com/facebook/react/pull/12925`))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`https://github.com/facebook/react/pull/12976`))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`https://github.com/facebook/react/pull/12966`))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`https://github.com/facebook/react/pull/12985`), [@&#8203;gaearon] in [#&#8203;13019](`https://github.com/facebook/react/pull/13019`))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`https://github.com/facebook/react/pull/13017`))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`https://github.com/facebook/react/pull/13030`))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
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.

Possible incorrect event.target on number inputs in IE9?
5 participants