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

Fix IE9 Undo Bug #12505

Closed
wants to merge 2 commits into from
Closed

Fix IE9 Undo Bug #12505

wants to merge 2 commits into from

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Mar 31, 2018

This commit fixes a case where IE9 raises an exception if modifying a detached text node.

This is reproducible with the following steps:

  1. Open http://react-dom-ie9-undo.surge.sh/text-inputs?version=16.3.0 in IE9
  2. Scroll to the "Bubbled Change Events" fixture
  3. Change the text in input
  4. Start debugging in the IE9 developer tools
  5. Press ctrl+z to undo your text change
  6. IE9 raises an exception when setting the nodeValue of the text label to the right of the input "invalid arguments".

The "local" build for that branch addresses the issue by removing a subscription to onpropertychange unique to IE9, which avoids a forced batch update that appears to be the root of the problem.


Fixes #11609

@@ -5,7 +5,7 @@ const React = window.React;
class NumberTestCase extends React.Component {
state = {value: ''};
onChange = event => {
const parsed = parseFloat(event.target.value, 10);
const parsed = parseFloat(event.currentTarget.value, 10);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

event.target was always the window in IE9. That seems like a bug too. I'm planning to investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed here: #12506. Pretty strange, I can't reproduce this outside of the test fixtures.

// eslint-disable-next-line
// https://connect.microsoft.com/IE/feedbackdetail/view/944330/invalid-argument-error-when-changing-nodevalue-of-a-text-node-removed-by-setting-innerhtml-on-an-ancestor
if (textInstance.parentNode) {
textInstance.nodeValue = newText;
Copy link
Contributor

Choose a reason for hiding this comment

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

The link is dead for me. Does accessing parentNode have an undue reflow effects? I don't have any reason to think it does but the DOM is weird :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Microsoft Connect has been retired

sigh. I'll try to find it again...

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually fix the undo behavior, or just stop it from throwing? Since we're not setting nodeValue I'd expect the behavior to still be broken, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just stops it from throwing. I haven't been able to determine if react is incorrectly updating stale nodes and IE9 is just less lenient, or if this is specific to IE9.

I can dig in a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed recently that it's possible for Fibers to retain references to detached nodes. I wonder if this is potentially related? Does this issue occur in React 15.x?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dunno if it helps: #12420 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back to the link issue. I'm having trouble digging up more information. For now, I've removed the link. I think the remaining comment is still sufficient.

@nhunzaker
Copy link
Contributor Author

What is a good next step here? My primary motivation is avoiding a problematic exception. Should we/I investigate further why React is trying to update a text node that has no parent?

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

Should we/I investigate further why React is trying to update a text node that has no parent?

This sounds like a good idea to me, I think? I wouldn't expect that.

Regarding the PR, I'm not a fan of putting an extra check relevant only to IE9 in a super hot path for text updates. We should understand more first.

@nhunzaker
Copy link
Contributor Author

Huh. Catching up on this, looks like this works outside of react:

https://codepen.io/nhunzaker/pen/Lrpqov?editors=1010

At the very least, undoing text doesn't break in the same way.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Jun 4, 2018

I've been able to narrow this down to the following code:

function manualDispatchChangeEvent(nativeEvent) {
const event = createAndAccumulateChangeEvent(
activeElementInst,
nativeEvent,
getEventTarget(nativeEvent),
);
// If change and propertychange bubbled, we'd just bind to it like all the
// other events and have it go through ReactBrowserEventEmitter. Since it
// doesn't, we manually listen for the events and so we have to enqueue and
// process the abstract event manually.
//
// Batching is necessary here in order to ensure that all event handlers run
// before the next rerender (including event handlers attached to ancestor
// elements instead of directly on the input). Without this, controlled
// components don't work properly in conjunction with event bubbling because
// the component is rerendered and the value reverted before all the event
// handlers can run. See https://github.com/facebook/react/issues/708.
batchedUpdates(runEventInBatch, event);
}

When this code runs, it results in a new text node in IE9.

Doing some quick testing, I actually don't think this line is necessary. I'll make a manual test fixture and confirm.


onChange = event => {
this.setState({
value: (event.srcElement || event.target).value,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this until #12976 lands

manualDispatchChangeEvent(nativeEvent);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jquense As far as I can test, this behavior is no longer necessary. Do you know what conditions onpropertychange is covering?

Copy link
Contributor

@jquense jquense Jun 4, 2018

Choose a reason for hiding this comment

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

@sophiebits original blog post on this is my primary text on the subject :P https://sophiebits.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html

I think you are right, I was reticent to yank it all out out of an abundance of caution, but it should be ok to remove (as well as the keyup and down handlers with selectionchange below)

@nhunzaker nhunzaker changed the title Add guard around commitTextValue to avoid IE9 undo bug Fix IE9 Undo Bug Jun 4, 2018
Before this commit, IE9 would force a change event to fire when the
onpropertychange callback fired. This threw an error when undoing text
changes: React would try to update a text node that was detached from
the DOM.
@nhunzaker
Copy link
Contributor Author

Okay, I think I have it! Things look good so far, but I'd like to do a more comprehensive sweep if we're able to merge #12976. It fixes the event target for inputs in IE9, making testing easier.

// components don't work properly in conjunction with event bubbling because
// the component is rerendered and the value reverted before all the event
// handlers can run. See https://github.com/facebook/react/issues/708.
batchedUpdates(runEventInBatch, event);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code dispatches a change event; it doesn't do anything by itself. Can we figure out why this was breaking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really feel comfortable removing this logic until we understand why it's buggy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally, 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, this code executes correctly. I believe the issue is a difference in browser behavior when using attachEvent vs addEventListener. The following code snippet seems to demonstrate what is happening:

Try this in IE9:
https://s.codepen.io/nhunzaker/debug/Lrpqov/ZoABaKanBoKr

Browse the code:
https://codepen.io/nhunzaker/pen/Lrpqov

It looks like IE9, when using attachEvent for text input, causes previously referenced text nodes to be replaced with a copy. This behavior doesn't present itself if you use addEventListener.

I think we were able to get away with this before (but I'm not super familiar) because we identified text via an embedded span, and then eventually relative to a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is... probably the strangest DOM thing I have seen. I would love a second/third opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't the onInput event catch that as well? Or does it not fire in that case for ie9?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to MDN:

IE 9 does not fire an input event when the user deletes characters from an input (e.g. by pressing Backspace or Delete, or using the "Cut" operation).

https://developer.mozilla.org/en-US/docs/Web/Events/input (footnote 2)

Copy link
Contributor

Choose a reason for hiding this comment

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

right, deleting doesn't work, we have the selectionchange and various key events to catch those cases tho (that is the intent anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, right I thought you were asking about input events with onInput, but you were probably talking about our polyfilled version :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we could catch some of these edge cases by comparing the value on focus:

https://github.com/facebook/react/blob/master/packages/react-dom/src/events/ChangeEventPlugin.js#L135-L139

I'm not sure what to do about "Cut", though I can't help but wonder if it triggers a selection change (you have text selected, you cut, and thus, the words disappear and selection changes).

@facebook-github-bot
Copy link

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

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

@stale
Copy link

stale bot commented Jan 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 17, 2020

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@stale stale bot closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo operation on text input throws exception in IE9
6 participants