Skip to content

Address issues where ID of nodeName causes internal errors in React #10076

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

Closed
nhunzaker wants to merge 2 commits into from
Closed

Address issues where ID of nodeName causes internal errors in React #10076

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
a84a433
Fix event handling when input name='nodeName'
jimfb Mar 21, 2016
1810b5a
Use nodeName getter instead of instanceof
nhunzaker Aug 9, 2017
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
16 changes: 16 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 @@ -70,6 +70,22 @@ class TextInputFixtures extends React.Component {
<InputTestCase type="url" defaultValue="" />
</TestCase>

<TestCase title="Inputs with an id of `nodeName`">
<TestCase.Steps>
<li>Type any character</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
There should be no errors in the console.
</TestCase.ExpectedResult>

<form className="control-box">
<fieldset>
<input id="nodeName" />
</fieldset>
</form>
</TestCase>

<TestCase title="All inputs" description="General test of all inputs">
<InputTestCase type="text" defaultValue="Text" />
<InputTestCase type="email" defaultValue="user@example.com" />
Expand Down
17 changes: 12 additions & 5 deletions src/renderers/dom/shared/ReactInputSelection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ function isInDocument(node) {
return containsNode(document.documentElement, node);
}

var nodeNameGetter = Object.getOwnPropertyDescriptor(Node.prototype, 'nodeName')
.get;

/**
* @ReactInputSelection: React input selection module. Based on Selection.js,
* but modified to be suitable for react and has a couple of bug fixes (doesn't
Expand All @@ -30,12 +33,16 @@ function isInDocument(node) {
*/
var ReactInputSelection = {
hasSelectionCapabilities: function(elem) {
var nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();
if (elem === window) {
return false;
}
Copy link
Contributor Author

@nhunzaker nhunzaker Aug 10, 2017

Choose a reason for hiding this comment

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

@sebmarkbage this is the only downside I see to calling the nodeName getter. Occasionally this value is the window. That's because the target instance falls back to the window when it can't find a target instance:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L149

I can't help but wonder if this could be document.body instead of window.

Copy link
Contributor Author

@nhunzaker nhunzaker Aug 10, 2017

Choose a reason for hiding this comment

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

A good example of this is if you throw a breakpoint in this function and click anywhere on the page other than the target input.

Copy link
Contributor Author

@nhunzaker nhunzaker Aug 10, 2017

Choose a reason for hiding this comment

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

Sorry. To clarify: calling nodeNameGetter on window raises an exception.


var nodeName = nodeNameGetter.call(elem);

return (
nodeName &&
((nodeName === 'input' && elem.type === 'text') ||
nodeName === 'textarea' ||
elem.contentEditable === 'true')
(nodeName === 'INPUT' && elem.type === 'text') ||
nodeName === 'TEXTAREA' ||
elem.contentEditable === 'true'
);
},

Expand Down
13 changes: 10 additions & 3 deletions src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,18 @@ var eventTypes = {
},
};

var nodeNameGetter = Object.getOwnPropertyDescriptor(Node.prototype, 'nodeName')
.get;

function shouldUseChangeEvent(elem) {
var nodeName = elem.nodeName && elem.nodeName.toLowerCase();
if (elem === window) {
return false;
}

var nodeName = nodeNameGetter.call(elem);

return (
nodeName === 'select' || (nodeName === 'input' && elem.type === 'file')
nodeName === 'SELECT' || (nodeName === 'INPUT' && elem.type === 'FILE')
);
}

Expand Down Expand Up @@ -159,7 +167,6 @@ var ChangeEventPlugin = {
}

var getTargetInstFunc, handleEventFunc;

if (shouldUseChangeEvent(targetNode)) {
getTargetInstFunc = getTargetInstForChangeEvent;
} else if (isTextInputElement(targetNode) && !isTextInputEventSupported) {
Expand Down
22 changes: 22 additions & 0 deletions src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,4 +1346,26 @@ describe('ReactDOMInput', () => {
expect(node.getAttribute('value')).toBe('1');
});
});

it('an input with an id of `nodeName` works as intended', () => {
var spy = jest.fn();

class Test extends React.Component {
render() {
return (
<form onSubmit={spy}>
<input id="nodeName" name="nodeName" defaultValue="test" />
<input type="submit" />
</form>
);
}
}

var stub = ReactTestUtils.renderIntoDocument(<Test />);
var node = ReactDOM.findDOMNode(stub);

ReactTestUtils.Simulate.submit(node);

expect(spy).toHaveBeenCalled();
});
});
6 changes: 2 additions & 4 deletions src/renderers/shared/utils/isTextInputElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,11 @@ var supportedInputTypes: {[key: string]: true | void} = {
};

function isTextInputElement(elem: ?HTMLElement): boolean {
var nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();

if (nodeName === 'input') {
if (elem instanceof HTMLInputElement) {
Copy link
Contributor

@sebmarkbage sebmarkbage Jun 30, 2017

Choose a reason for hiding this comment

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

This doesn't work when these nodes are rendered into another iframe. It's not a super common use case but a bigger discussion if we should stop supporting that.

Copy link
Contributor Author

@nhunzaker nhunzaker Jun 30, 2017

Choose a reason for hiding this comment

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

Woah:

iframe.contentWindow.window.HTMLInputElement instanceof HTMLInputElement // false

Today I learned....

Copy link
Contributor Author

@nhunzaker nhunzaker Jun 30, 2017

Choose a reason for hiding this comment

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

Seems like we could at least get some test coverage for this. I'll dig into that too. Though, if we don't have any coverage for it presently, I wonder if it's just a can of worms in JSDOM.

Copy link
Contributor Author

@nhunzaker nhunzaker Jun 30, 2017

Choose a reason for hiding this comment

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

How much slower would it be to do something like:

getInstanceForNode(elem)._tag === 'input'

That gets us around working with the DOM.

return !!supportedInputTypes[((elem: any): HTMLInputElement).type];
}

if (nodeName === 'textarea') {
if (elem instanceof HTMLTextAreaElement) {
return true;
}

Expand Down