Skip to content

Validate node nesting, take 3 #3467

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

Merged
merged 1 commit into from
Mar 24, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/browser/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ var ReactDOM = mapObject({
h6: 'h6',
head: 'head',
header: 'header',
hgroup: 'hgroup',
Copy link
Member

Choose a reason for hiding this comment

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

But hgroup was dropped from the spec (which is why I never added it). But I guess if it's still supported, fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh! Okay, will kill.

Copy link
Member

Choose a reason for hiding this comment

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

Meh, it doesn't matter too much anymore since React.createElement('hgroup') works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Guess I'll leave it then.

hr: 'hr',
html: 'html',
i: 'i',
Expand Down
29 changes: 27 additions & 2 deletions src/browser/ui/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var invariant = require('invariant');
var isEventSupported = require('isEventSupported');
var keyOf = require('keyOf');
var validateDOMNesting = require('validateDOMNesting');
var warning = require('warning');

var deleteListener = ReactBrowserEventEmitter.deleteListener;
Expand Down Expand Up @@ -172,6 +173,15 @@ function validateDangerousTag(tag) {
}
}

function processChildContext(context, tagName) {
if (__DEV__) {
// Pass down our tag name to child components for validation purposes
context = assign({}, context);
context[validateDOMNesting.parentTagContextKey] = tagName;
}
return context;
}

/**
* Creates a new React class that is idempotent and capable of containing other
* React components. It accepts event listeners and DOM properties that are
Expand Down Expand Up @@ -213,7 +223,18 @@ ReactDOMComponent.Mixin = {
*/
mountComponent: function(rootID, transaction, context) {
this._rootNodeID = rootID;

assertValidProps(this, this._currentElement.props);
if (__DEV__) {
if (context[validateDOMNesting.parentTagContextKey]) {
validateDOMNesting(
context[validateDOMNesting.parentTagContextKey],
this._tag,
this._currentElement
);
}
}

var tagOpen = this._createOpenTagMarkupAndPutListeners(transaction);
var tagContent = this._createContentMarkup(transaction, context);
if (!tagContent && omittedCloseTags[this._tag]) {
Expand Down Expand Up @@ -301,7 +322,7 @@ ReactDOMComponent.Mixin = {
var mountImages = this.mountChildren(
childrenToUse,
transaction,
context
processChildContext(context, this._tag)
);
ret = mountImages.join('');
}
Expand Down Expand Up @@ -342,7 +363,11 @@ ReactDOMComponent.Mixin = {
updateComponent: function(transaction, prevElement, nextElement, context) {
assertValidProps(this, this._currentElement.props);
this._updateDOMProperties(prevElement.props, transaction);
this._updateDOMChildren(prevElement.props, transaction, context);
this._updateDOMChildren(
prevElement.props,
transaction,
processChildContext(context, this._tag)
);
},

/**
Expand Down
11 changes: 11 additions & 0 deletions src/browser/ui/ReactDOMTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var ReactDOMComponent = require('ReactDOMComponent');

var assign = require('Object.assign');
var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var validateDOMNesting = require('validateDOMNesting');

/**
* Text nodes violate a couple assumptions that React makes about components:
Expand Down Expand Up @@ -65,6 +66,16 @@ assign(ReactDOMTextComponent.prototype, {
* @internal
*/
mountComponent: function(rootID, transaction, context) {
if (__DEV__) {
if (context[validateDOMNesting.parentTagContextKey]) {
validateDOMNesting(
context[validateDOMNesting.parentTagContextKey],
'span',
null
);
}
}

this._rootNodeID = rootID;
var escapedText = escapeTextContentForBrowser(this._stringText);

Expand Down
11 changes: 7 additions & 4 deletions src/browser/ui/ReactDefaultInjection.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var ReactElement = require('ReactElement');
var ReactEventListener = require('ReactEventListener');
var ReactInjection = require('ReactInjection');
var ReactInstanceHandles = require('ReactInstanceHandles');
var ReactInstanceMap = require('ReactInstanceMap');
var ReactMount = require('ReactMount');
var ReactReconcileTransaction = require('ReactReconcileTransaction');
var SelectEventPlugin = require('SelectEventPlugin');
Expand All @@ -51,12 +52,14 @@ function autoGenerateWrapperClass(type) {
return ReactClass.createClass({
tagName: type.toUpperCase(),
render: function() {
// Copy owner down for debugging info
var internalInstance = ReactInstanceMap.get(this);
return new ReactElement(
type,
null,
null,
null,
null,
null, // key
null, // ref
internalInstance._currentElement._owner, // owner
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we be doing this already? My warning didn't work without it.

null, // context
this.props
);
}
Expand Down
9 changes: 8 additions & 1 deletion src/browser/ui/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ var instantiateReactComponent = require('instantiateReactComponent');
var invariant = require('invariant');
var setInnerHTML = require('setInnerHTML');
var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
var validateDOMNesting = require('validateDOMNesting');
var warning = require('warning');

var SEPARATOR = ReactInstanceHandles.SEPARATOR;
Expand Down Expand Up @@ -246,8 +247,14 @@ function mountComponentIntoNode(
container,
transaction,
shouldReuseMarkup) {
var context = emptyObject;
if (__DEV__) {
context = {};
context[validateDOMNesting.parentTagContextKey] =
container.nodeName.toLowerCase();
}
var markup = ReactReconciler.mountComponent(
componentInstance, rootID, transaction, emptyObject
componentInstance, rootID, transaction, context
);
componentInstance._isTopLevel = true;
ReactMount._mountImageIntoNode(markup, container, shouldReuseMarkup);
Expand Down
52 changes: 50 additions & 2 deletions src/browser/ui/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -373,11 +373,11 @@ describe('ReactDOMComponent', function() {

var container = document.createElement('div');

React.render(<menuitem />, container);
React.render(<menu><menuitem /></menu>, container);

expect(container.innerHTML).toContain('</menuitem>');

React.render(<menuitem>children</menuitem>, container);
React.render(<menu><menuitem>children</menuitem></menu>, container);

expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain('void element');
Expand Down Expand Up @@ -651,6 +651,54 @@ describe('ReactDOMComponent', function() {
'Invariant Violation: Invalid tag: div><img /><div'
);
});
});

describe('nesting validation', function() {
var React;
var ReactTestUtils;

beforeEach(function() {
React = require('React');
ReactTestUtils = require('ReactTestUtils');
});

it('warns on invalid nesting', () => {
spyOn(console, 'warn');
ReactTestUtils.renderIntoDocument(<div><tr /></div>);

expect(console.warn.calls.length).toBe(1);
expect(console.warn.calls[0].args[0]).toBe(
'Warning: validateDOMNesting(...): <div> cannot contain a <tr> node.'
);
});

it('warns on invalid nesting at root', () => {
spyOn(console, 'warn');
var p = document.createElement('p');
React.render(<tr />, p);

expect(console.warn.calls.length).toBe(1);
expect(console.warn.calls[0].args[0]).toBe(
'Warning: validateDOMNesting(...): <p> cannot contain a <tr> node.'
);
});

it('warns nicely for table rows', () => {
spyOn(console, 'warn');
var Foo = React.createClass({
render: function() {
return <table><tr /></table>;
}
});
ReactTestUtils.renderIntoDocument(<Foo />);

expect(console.warn.calls.length).toBe(1);
expect(console.warn.calls[0].args[0]).toBe(
'Warning: validateDOMNesting(...): <table> cannot contain a <tr> ' +
'node. Add a <tbody> to your code to match the DOM tree generated by ' +
'the browser. Check the render method of `Foo`.'
);
});
});

});
Loading