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

Jump to bottom

Fix IE9 Undo Bug #12505

Closed
wants to merge 2 commits into from
Closed

Fix IE9 Undo Bug #12505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43b0829
Remove forced bubble code to prevent undo errors in IE9
nhunzaker Jun 4, 2018
5bda719
Format code with prettier
nhunzaker Jun 4, 2018
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
32 changes: 32 additions & 0 deletions fixtures/dom/src/components/fixtures/text-inputs/BubbledInputTestCase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import Fixture from '../../Fixture';
const React = window.React;

const noop = () => {};

class BubbledInputTestCase extends React.Component {
state = {
value: '',
};

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

});
};

render() {
return (
<Fixture>
<div className="control-box" onChange={this.onChange}>
<fieldset>
<legend>Controlled Text</legend>
<input value={this.state.value} onChange={noop} />
<p className="hint">Value: {JSON.stringify(this.state.value)}</p>
</fieldset>
</div>
</Fixture>
);
}
}

export default BubbledInputTestCase;
17 changes: 17 additions & 0 deletions fixtures/dom/src/components/fixtures/text-inputs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Fixture from '../../Fixture';
import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';
import InputTestCase from './InputTestCase';
import BubbledInputTestCase from './BubbledInputTestCase';
import ReplaceEmailInput from './ReplaceEmailInput';

const React = window.React;
Expand Down Expand Up @@ -141,6 +142,22 @@ class TextInputFixtures extends React.Component {
<ReplaceEmailInput />
</TestCase>

<TestCase
title="Bubbled Change Events"
affectedBrowsers="IE9"
relatedIssues="708,11609">
<TestCase.Steps>
<li>Enter text</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
Each input's value should change as expected, despite the change
event being attached to the parent element
</TestCase.ExpectedResult>

<BubbledInputTestCase type="text" />
</TestCase>

<TestCase title="All inputs" description="General test of all inputs">
<InputTestCase type="text" defaultValue="Text" />
<InputTestCase type="email" defaultValue="[email protected]" />
Expand Down
43 changes: 0 additions & 43 deletions packages/react-dom/src/events/ChangeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@
* LICENSE file in the root directory of this source tree.
*/

import * as EventPluginHub from 'events/EventPluginHub';
import {accumulateTwoPhaseDispatches} from 'events/EventPropagators';
import {enqueueStateRestore} from 'events/ReactControlledComponent';
import {batchedUpdates} from 'events/ReactGenericBatching';
import SyntheticEvent from 'events/SyntheticEvent';
import isTextInputElement from 'shared/isTextInputElement';
import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment';
Expand All @@ -23,7 +21,6 @@ import {
TOP_KEY_UP,
TOP_SELECTION_CHANGE,
} from './DOMTopLevelEventTypes';
import getEventTarget from './getEventTarget';
import isEventSupported from './isEventSupported';
import {getNodeFromInstance} from '../client/ReactDOMComponentTree';
import * as inputValueTracking from '../client/inputValueTracking';
Expand Down Expand Up @@ -77,31 +74,6 @@ function shouldUseChangeEvent(elem) {
);
}

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);
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).

}

function runEventInBatch(event) {
EventPluginHub.runEventsInBatch(event, false);
}

function getInstIfValueChanged(targetInst) {
const targetNode = getNodeFromInstance(targetInst);
if (inputValueTracking.updateValueIfChanged(targetNode)) {
Expand Down Expand Up @@ -135,7 +107,6 @@ if (ExecutionEnvironment.canUseDOM) {
function startWatchingForValueChange(target, targetInst) {
activeElement = target;
activeElementInst = targetInst;
activeElement.attachEvent('onpropertychange', handlePropertyChange);
}

/**
Expand All @@ -146,24 +117,10 @@ function stopWatchingForValueChange() {
if (!activeElement) {
return;
}
activeElement.detachEvent('onpropertychange', handlePropertyChange);
activeElement = null;
activeElementInst = null;
}

/**
* (For IE <=9) Handles a propertychange event, sending a `change` event if
* the value of the active element has changed.
*/
function handlePropertyChange(nativeEvent) {
if (nativeEvent.propertyName !== 'value') {
return;
}
if (getInstIfValueChanged(activeElementInst)) {
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 •
edited
Loading

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)

function handleEventsForInputEventPolyfill(topLevelType, target, targetInst) {
if (topLevelType === TOP_FOCUS) {
// In IE9, propertychange fires for most input events but is buggy and
Expand Down