From da9193c2245fd278f22992997275e637f1ade23a Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 28 Dec 2013 23:07:18 -0700 Subject: [PATCH 1/6] Validate node nesting structure This is a better version of #644. Fixes #101. --- .../ReactComponentBrowserEnvironment.js | 10 +++++ src/browser/ReactDOMComponent.js | 21 ++++++++-- src/browser/dom/Danger.js | 18 +------- src/core/ReactCompositeComponent.js | 10 +++++ src/core/ReactMultiChild.js | 10 +++++ src/core/__tests__/ReactDOMComponent-test.js | 25 +++++++++-- src/dom/getNodeNameFromReactMarkup.js | 42 +++++++++++++++++++ src/dom/nodeCanContainNode.js | 39 +++++++++++++++++ src/dom/validateNodeNesting.js | 37 ++++++++++++++++ src/utils/joinAccumulated.js | 30 +++++++++++++ 10 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 src/dom/getNodeNameFromReactMarkup.js create mode 100644 src/dom/nodeCanContainNode.js create mode 100644 src/dom/validateNodeNesting.js create mode 100644 src/utils/joinAccumulated.js diff --git a/src/browser/ReactComponentBrowserEnvironment.js b/src/browser/ReactComponentBrowserEnvironment.js index af8e97999dd2d..3eb169ac8a140 100644 --- a/src/browser/ReactComponentBrowserEnvironment.js +++ b/src/browser/ReactComponentBrowserEnvironment.js @@ -29,6 +29,11 @@ var ReactReconcileTransaction = require('ReactReconcileTransaction'); var getReactRootElementInContainer = require('getReactRootElementInContainer'); var invariant = require('invariant'); +if (__DEV__) { + var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); + var validateNodeNesting = require('validateNodeNesting'); +} + var ELEMENT_NODE_TYPE = 1; var DOC_NODE_TYPE = 9; @@ -133,6 +138,11 @@ var ReactComponentBrowserEnvironment = { 'See renderComponentToString() for server rendering.' ); + if (__DEV__) { + var nodeName = getNodeNameFromReactMarkup(markup); + validateNodeNesting(container.nodeName, nodeName); + } + // Asynchronously inject markup by ensuring that the container is not in // the document when settings its `innerHTML`. var parent = container.parentNode; diff --git a/src/browser/ReactDOMComponent.js b/src/browser/ReactDOMComponent.js index 8ef1fb884e689..9f0e9098bce7d 100644 --- a/src/browser/ReactDOMComponent.js +++ b/src/browser/ReactDOMComponent.js @@ -30,10 +30,17 @@ var ReactPerf = require('ReactPerf'); var escapeTextForBrowser = require('escapeTextForBrowser'); var invariant = require('invariant'); +var joinAccumulated = require('joinAccumulated'); var keyOf = require('keyOf'); var merge = require('merge'); var mixInto = require('mixInto'); +if (__DEV__) { + var forEachAccumulated = require('forEachAccumulated'); + var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); + var validateNodeNesting = require('validateNodeNesting'); +} + var deleteListener = ReactEventEmitter.deleteListener; var listenTo = ReactEventEmitter.listenTo; var registrationNameModules = ReactEventEmitter.registrationNameModules; @@ -114,9 +121,17 @@ ReactDOMComponent.Mixin = { mountDepth ); assertValidProps(this.props); + var contentMarkup = this._createContentMarkup(transaction); + if (__DEV__) { + var tagName = this.tagName; + forEachAccumulated(contentMarkup, function(markup) { + var nodeName = getNodeNameFromReactMarkup(markup); + validateNodeNesting(tagName, nodeName); + }); + } return ( this._createOpenTagMarkupAndPutListeners(transaction) + - this._createContentMarkup(transaction) + + joinAccumulated(contentMarkup, '') + this._tagClose ); } @@ -172,7 +187,7 @@ ReactDOMComponent.Mixin = { * * @private * @param {ReactReconcileTransaction} transaction - * @return {string} Content markup. + * @return {string|array} Content markup or list of content markup. */ _createContentMarkup: function(transaction) { // Intentional use of != to avoid catching zero/false. @@ -192,7 +207,7 @@ ReactDOMComponent.Mixin = { childrenToUse, transaction ); - return mountImages.join(''); + return mountImages; } } return ''; diff --git a/src/browser/dom/Danger.js b/src/browser/dom/Danger.js index ccefcde8302bb..ce440b5b28ea2 100644 --- a/src/browser/dom/Danger.js +++ b/src/browser/dom/Danger.js @@ -26,26 +26,12 @@ var ExecutionEnvironment = require('ExecutionEnvironment'); var createNodesFromMarkup = require('createNodesFromMarkup'); var emptyFunction = require('emptyFunction'); var getMarkupWrap = require('getMarkupWrap'); +var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); var invariant = require('invariant'); var OPEN_TAG_NAME_EXP = /^(<[^ \/>]+)/; var RESULT_INDEX_ATTR = 'data-danger-index'; -/** - * Extracts the `nodeName` from a string of markup. - * - * NOTE: Extracting the `nodeName` does not require a regular expression match - * because we make assumptions about React-generated markup (i.e. there are no - * spaces surrounding the opening tag and there is at least one attribute). - * - * @param {string} markup String of markup. - * @return {string} Node name of the supplied markup. - * @see http://jsperf.com/extract-nodename - */ -function getNodeName(markup) { - return markup.substring(1, markup.indexOf(' ')); -} - var Danger = { /** @@ -72,7 +58,7 @@ var Danger = { markupList[i], 'dangerouslyRenderMarkup(...): Missing markup.' ); - nodeName = getNodeName(markupList[i]); + nodeName = getNodeNameFromReactMarkup(markupList[i]); nodeName = getMarkupWrap(nodeName) ? nodeName : '*'; markupByNodeName[nodeName] = markupByNodeName[nodeName] || []; markupByNodeName[nodeName][i] = markupList[i]; diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 4f6f62a6a852c..1488b31405f8f 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -36,6 +36,11 @@ var mixInto = require('mixInto'); var objMap = require('objMap'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); +if (__DEV__) { + var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); + var validateNodeNesting = require('validateNodeNesting'); +} + /** * Policies that describe methods in `ReactCompositeComponentInterface`. */ @@ -1224,6 +1229,11 @@ var ReactCompositeComponentMixin = { transaction, this._mountDepth + 1 ); + if (__DEV__) { + var parentNode = this.getDOMNode().parentNode; + var nodeName = getNodeNameFromReactMarkup(nextMarkup); + validateNodeNesting(parentNode.nodeName, nodeName); + } ReactComponent.BackendIDOperations.dangerouslyReplaceNodeWithMarkupByID( prevComponentID, nextMarkup diff --git a/src/core/ReactMultiChild.js b/src/core/ReactMultiChild.js index e697ac6002279..4896df77eb9db 100644 --- a/src/core/ReactMultiChild.js +++ b/src/core/ReactMultiChild.js @@ -25,6 +25,11 @@ var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes'); var flattenChildren = require('flattenChildren'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); +if (__DEV__) { + var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); + var validateNodeNesting = require('validateNodeNesting'); +} + /** * Updating children of a component may trigger recursive updates. The depth is * used to batch recursive updates to render markup more efficiently. @@ -349,6 +354,11 @@ var ReactMultiChild = { * @protected */ createChild: function(child, mountImage) { + if (__DEV__) { + var parentNode = this.getDOMNode(); + var nodeName = getNodeNameFromReactMarkup(child._mountImage); + validateNodeNesting(parentNode.nodeName, nodeName); + } enqueueMarkup(this._rootNodeID, mountImage, child._mountIndex); }, diff --git a/src/core/__tests__/ReactDOMComponent-test.js b/src/core/__tests__/ReactDOMComponent-test.js index 68aa1d0fbada5..f4b454cb48b0b 100644 --- a/src/core/__tests__/ReactDOMComponent-test.js +++ b/src/core/__tests__/ReactDOMComponent-test.js @@ -204,6 +204,20 @@ describe('ReactDOMComponent', function() { stub.receiveComponent({props: {}}, transaction); expect(nodeValueSetter.mock.calls.length).toBe(1); }); + + it("should warn on invalid markup nesting", function() { + spyOn(console, 'warn'); + expect(console.warn.argsForCall.length).toBe(0); + var stub = ReactTestUtils.renderIntoDocument( +
+ ); + + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toBe( + 'validateNodeNesting(...): Node of type div cannot contain node of ' + + 'type tr.' + ); + }); }); describe('createOpenTagMarkup', function() { @@ -260,10 +274,12 @@ describe('ReactDOMComponent', function() { beforeEach(function() { require('mock-modules').dumpCache(); - var mixInto = require('mixInto'); var ReactDOMComponent = require('ReactDOMComponent'); var ReactReconcileTransaction = require('ReactReconcileTransaction'); + var joinAccumulated = require('joinAccumulated'); + var mixInto = require('mixInto'); + var NodeStub = function(initialProps) { this.props = initialProps || {}; this._rootNodeID = 'test'; @@ -272,11 +288,12 @@ describe('ReactDOMComponent', function() { genMarkup = function(props) { var transaction = new ReactReconcileTransaction(); - return (new NodeStub(props))._createContentMarkup(transaction); + var markup = (new NodeStub(props))._createContentMarkup(transaction); + return joinAccumulated(markup, ''); }; this.addMatchers({ - toHaveInnerhtml: function(html) { + toHaveInnerHTML: function(html) { var expected = '^' + quoteRegexp(html) + '$'; return this.actual.match(new RegExp(expected)); } @@ -287,7 +304,7 @@ describe('ReactDOMComponent', function() { var innerHTML = {__html: 'testContent'}; expect( genMarkup({ dangerouslySetInnerHTML: innerHTML }) - ).toHaveInnerhtml('testContent'); + ).toHaveInnerHTML('testContent'); }); }); diff --git a/src/dom/getNodeNameFromReactMarkup.js b/src/dom/getNodeNameFromReactMarkup.js new file mode 100644 index 0000000000000..db52ba961af2d --- /dev/null +++ b/src/dom/getNodeNameFromReactMarkup.js @@ -0,0 +1,42 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule getNodeNameFromReactMarkup + */ + +"use strict"; + +/** + * Extracts the `nodeName` from a string of markup generated by React. + * + * NOTE: Extracting the `nodeName` does not require a regular expression match + * because we make assumptions about React-generated markup (i.e. there are no + * spaces surrounding the opening tag and there is at least one attribute). + * + * @param {string} markup String of markup. + * @return {string} Node name of the supplied markup. + * @see http://jsperf.com/extract-nodename/2 + */ +function getNodeNameFromReactMarkup(markup) { + if (markup.charAt(0) === '<') { + return markup.substring(1, markup.indexOf(' ')); + } else if (markup) { + return '#text'; + } else { + return ''; + } +} + +module.exports = getNodeNameFromReactMarkup; diff --git a/src/dom/nodeCanContainNode.js b/src/dom/nodeCanContainNode.js new file mode 100644 index 0000000000000..5c772da525cb2 --- /dev/null +++ b/src/dom/nodeCanContainNode.js @@ -0,0 +1,39 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule nodeCanContainNode + */ + +"use strict"; + +// TODO: Validate all possible tag combinations, along the lines of +// https://github.com/facebook/xhp/blob/master/php-lib/html.php + +var nodeCanContainNode = function(parentNodeName, childNodeName) { + parentNodeName = parentNodeName.toLowerCase(); + childNodeName = childNodeName.toLowerCase(); + + if (childNodeName === 'p') { + return parentNodeName !== 'p'; + } else if (childNodeName === 'tr') { + return ['tbody', 'tfoot', 'thead'].indexOf(parentNodeName) !== -1; + } else if (['tbody', 'tfoot', 'thead'].indexOf(childNodeName) !== -1) { + return parentNodeName === 'table'; + } + + return true; +}; + +module.exports = nodeCanContainNode; diff --git a/src/dom/validateNodeNesting.js b/src/dom/validateNodeNesting.js new file mode 100644 index 0000000000000..5d39b1e908b40 --- /dev/null +++ b/src/dom/validateNodeNesting.js @@ -0,0 +1,37 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule validateNodeNesting + */ + +"use strict"; + +if (__DEV__) { + var nodeCanContainNode = require('nodeCanContainNode'); +} + +var validateNodeNesting = function(parentNodeName, childNodeName) { + if (__DEV__) { + if (!nodeCanContainNode(parentNodeName, childNodeName)) { + console.warn( + 'validateNodeNesting(...): Node of type ' + + parentNodeName.toLowerCase() + ' cannot contain node of type ' + + childNodeName.toLowerCase() + '.' + ); + } + } +}; + +module.exports = validateNodeNesting; diff --git a/src/utils/joinAccumulated.js b/src/utils/joinAccumulated.js new file mode 100644 index 0000000000000..ea2cbdb02e2d3 --- /dev/null +++ b/src/utils/joinAccumulated.js @@ -0,0 +1,30 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule joinAccumulated + */ + +"use strict"; + +var joinAccumulated = function(arr, sep) { + if (Array.isArray(arr)) { + return arr.join(sep); + } else if (arr != null) { + return String(arr); + } + return ''; +}; + +module.exports = joinAccumulated; From ef46fa84a57238bc03f2d698c21217970d5cc2d5 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 30 Dec 2013 22:00:35 -0700 Subject: [PATCH 2/6] Inline nodeCanContainNode module into validate fn --- src/dom/nodeCanContainNode.js | 39 ------------------------------ src/dom/validateNodeNesting.js | 44 ++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 51 deletions(-) delete mode 100644 src/dom/nodeCanContainNode.js diff --git a/src/dom/nodeCanContainNode.js b/src/dom/nodeCanContainNode.js deleted file mode 100644 index 5c772da525cb2..0000000000000 --- a/src/dom/nodeCanContainNode.js +++ /dev/null @@ -1,39 +0,0 @@ -/** - * Copyright 2013 Facebook, Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - * @providesModule nodeCanContainNode - */ - -"use strict"; - -// TODO: Validate all possible tag combinations, along the lines of -// https://github.com/facebook/xhp/blob/master/php-lib/html.php - -var nodeCanContainNode = function(parentNodeName, childNodeName) { - parentNodeName = parentNodeName.toLowerCase(); - childNodeName = childNodeName.toLowerCase(); - - if (childNodeName === 'p') { - return parentNodeName !== 'p'; - } else if (childNodeName === 'tr') { - return ['tbody', 'tfoot', 'thead'].indexOf(parentNodeName) !== -1; - } else if (['tbody', 'tfoot', 'thead'].indexOf(childNodeName) !== -1) { - return parentNodeName === 'table'; - } - - return true; -}; - -module.exports = nodeCanContainNode; diff --git a/src/dom/validateNodeNesting.js b/src/dom/validateNodeNesting.js index 5d39b1e908b40..0f2e584ad98e2 100644 --- a/src/dom/validateNodeNesting.js +++ b/src/dom/validateNodeNesting.js @@ -18,20 +18,40 @@ "use strict"; +var emptyFunction = require('emptyFunction'); + +var validateNodeNesting = emptyFunction; + if (__DEV__) { - var nodeCanContainNode = require('nodeCanContainNode'); -} + // TODO: Validate all possible tag combinations, along the lines of + // https://github.com/facebook/xhp/blob/master/php-lib/html.php + + var nodeCanContainNode = function(parentNodeName, childNodeName) { + parentNodeName = parentNodeName.toLowerCase(); + childNodeName = childNodeName.toLowerCase(); -var validateNodeNesting = function(parentNodeName, childNodeName) { - if (__DEV__) { - if (!nodeCanContainNode(parentNodeName, childNodeName)) { - console.warn( - 'validateNodeNesting(...): Node of type ' + - parentNodeName.toLowerCase() + ' cannot contain node of type ' + - childNodeName.toLowerCase() + '.' - ); + if (childNodeName === 'p') { + return parentNodeName !== 'p'; + } else if (childNodeName === 'tr') { + return ['tbody', 'tfoot', 'thead'].indexOf(parentNodeName) !== -1; + } else if (['tbody', 'tfoot', 'thead'].indexOf(childNodeName) !== -1) { + return parentNodeName === 'table'; } - } -}; + + return true; + }; + + validateNodeNesting = function(parentNodeName, childNodeName) { + if (__DEV__) { + if (!nodeCanContainNode(parentNodeName, childNodeName)) { + console.warn( + 'validateNodeNesting(...): Node of type ' + + parentNodeName.toLowerCase() + ' cannot contain node of type ' + + childNodeName.toLowerCase() + '.' + ); + } + } + }; +} module.exports = validateNodeNesting; From 410471fe4ab184806e406789bab32eb9bfa0db95 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 30 Dec 2013 22:11:31 -0700 Subject: [PATCH 3/6] Consolidate validation logic into DOM modules --- src/browser/dom/DOMChildrenOperations.js | 7 +++++++ src/browser/dom/Danger.js | 7 +++++++ src/core/ReactCompositeComponent.js | 10 ---------- src/core/ReactMultiChild.js | 10 ---------- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/browser/dom/DOMChildrenOperations.js b/src/browser/dom/DOMChildrenOperations.js index 9e48b1ae9c36a..d760dc0b6e1cd 100644 --- a/src/browser/dom/DOMChildrenOperations.js +++ b/src/browser/dom/DOMChildrenOperations.js @@ -24,6 +24,10 @@ var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes'); var getTextContentAccessor = require('getTextContentAccessor'); +if (__DEV__) { + var validateNodeNesting = require('validateNodeNesting'); +} + /** * The DOM property to use when setting text content. * @@ -41,6 +45,9 @@ var textContentAccessor = getTextContentAccessor(); * @internal */ function insertChildAt(parentNode, childNode, index) { + if (__DEV__) { + validateNodeNesting(parentNode.nodeName, childNode.nodeName); + } var childNodes = parentNode.childNodes; if (childNodes[index] === childNode) { return; diff --git a/src/browser/dom/Danger.js b/src/browser/dom/Danger.js index ce440b5b28ea2..676a86c4092c0 100644 --- a/src/browser/dom/Danger.js +++ b/src/browser/dom/Danger.js @@ -29,6 +29,10 @@ var getMarkupWrap = require('getMarkupWrap'); var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); var invariant = require('invariant'); +if (__DEV__) { + var validateNodeNesting = require('validateNodeNesting'); +} + var OPEN_TAG_NAME_EXP = /^(<[^ \/>]+)/; var RESULT_INDEX_ATTR = 'data-danger-index'; @@ -165,6 +169,9 @@ var Danger = { ); var newChild = createNodesFromMarkup(markup, emptyFunction)[0]; + if (__DEV__) { + validateNodeNesting(oldChild.parentNode.nodeName, newChild.nodeName); + } oldChild.parentNode.replaceChild(newChild, oldChild); } diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 1488b31405f8f..4f6f62a6a852c 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -36,11 +36,6 @@ var mixInto = require('mixInto'); var objMap = require('objMap'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); -if (__DEV__) { - var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); - var validateNodeNesting = require('validateNodeNesting'); -} - /** * Policies that describe methods in `ReactCompositeComponentInterface`. */ @@ -1229,11 +1224,6 @@ var ReactCompositeComponentMixin = { transaction, this._mountDepth + 1 ); - if (__DEV__) { - var parentNode = this.getDOMNode().parentNode; - var nodeName = getNodeNameFromReactMarkup(nextMarkup); - validateNodeNesting(parentNode.nodeName, nodeName); - } ReactComponent.BackendIDOperations.dangerouslyReplaceNodeWithMarkupByID( prevComponentID, nextMarkup diff --git a/src/core/ReactMultiChild.js b/src/core/ReactMultiChild.js index 4896df77eb9db..e697ac6002279 100644 --- a/src/core/ReactMultiChild.js +++ b/src/core/ReactMultiChild.js @@ -25,11 +25,6 @@ var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes'); var flattenChildren = require('flattenChildren'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); -if (__DEV__) { - var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); - var validateNodeNesting = require('validateNodeNesting'); -} - /** * Updating children of a component may trigger recursive updates. The depth is * used to batch recursive updates to render markup more efficiently. @@ -354,11 +349,6 @@ var ReactMultiChild = { * @protected */ createChild: function(child, mountImage) { - if (__DEV__) { - var parentNode = this.getDOMNode(); - var nodeName = getNodeNameFromReactMarkup(child._mountImage); - validateNodeNesting(parentNode.nodeName, nodeName); - } enqueueMarkup(this._rootNodeID, mountImage, child._mountIndex); }, From 2b8ae0eda8e978a91631901c5c24b1aa7b014b80 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Thu, 2 Jan 2014 10:56:03 -0700 Subject: [PATCH 4/6] Validate all node nesting based on HTML spec --- src/browser/ReactDOM.js | 10 +- src/browser/dom/DOMChildrenOperations.js | 5 + ...actCompositeComponentDOMMinimalism-test.js | 8 +- src/core/__tests__/refs-test.js | 2 +- src/dom/validateNodeNesting.js | 163 ++++++++++++++++-- 5 files changed, 164 insertions(+), 24 deletions(-) diff --git a/src/browser/ReactDOM.js b/src/browser/ReactDOM.js index b7eeecf004dea..a337f5f745b25 100644 --- a/src/browser/ReactDOM.js +++ b/src/browser/ReactDOM.js @@ -72,12 +72,12 @@ var ReactDOM = objMapKeyVal({ a: false, abbr: false, address: false, - area: false, + area: true, article: false, aside: false, audio: false, b: false, - base: false, + base: true, bdi: false, bdo: false, big: false, @@ -127,7 +127,7 @@ var ReactDOM = objMapKeyVal({ label: false, legend: false, li: false, - link: false, + link: true, main: false, map: false, mark: false, @@ -156,7 +156,7 @@ var ReactDOM = objMapKeyVal({ section: false, select: false, small: false, - source: false, + source: true, span: false, strong: false, style: false, @@ -178,7 +178,7 @@ var ReactDOM = objMapKeyVal({ ul: false, 'var': false, video: false, - wbr: false, + wbr: true, // SVG circle: false, diff --git a/src/browser/dom/DOMChildrenOperations.js b/src/browser/dom/DOMChildrenOperations.js index d760dc0b6e1cd..ce7179abc1c38 100644 --- a/src/browser/dom/DOMChildrenOperations.js +++ b/src/browser/dom/DOMChildrenOperations.js @@ -128,6 +128,11 @@ var DOMChildrenOperations = { ); break; case ReactMultiChildUpdateTypes.TEXT_CONTENT: + if (__DEV__) { + if (update.textContent !== '') { + validateNodeNesting(update.parentNode.nodeName, '#text'); + } + } update.parentNode[textContentAccessor] = update.textContent; break; case ReactMultiChildUpdateTypes.REMOVE_NODE: diff --git a/src/core/__tests__/ReactCompositeComponentDOMMinimalism-test.js b/src/core/__tests__/ReactCompositeComponentDOMMinimalism-test.js index 4e956f13f5a88..99bd7b2ec0926 100644 --- a/src/core/__tests__/ReactCompositeComponentDOMMinimalism-test.js +++ b/src/core/__tests__/ReactCompositeComponentDOMMinimalism-test.js @@ -95,9 +95,9 @@ describe('ReactCompositeComponentDOMMinimalism', function() { it('should not render extra nodes for non-interpolated text', function() { var instance = ( -
    - This text causes no children in ul, just innerHTML -
+ + This text causes no children in span, just innerHTML + ); ReactTestUtils.renderIntoDocument(instance); @@ -108,7 +108,7 @@ describe('ReactCompositeComponentDOMMinimalism', function() { .toBeDOMComponentWithTag('div') .toBeDOMComponentWithChildCount(1) .expectRenderedChildAt(0) - .toBeDOMComponentWithTag('ul') + .toBeDOMComponentWithTag('span') .toBeDOMComponentWithNoChildren(); }); diff --git a/src/core/__tests__/refs-test.js b/src/core/__tests__/refs-test.js index fd9ff2eb12853..78fb590a40e12 100644 --- a/src/core/__tests__/refs-test.js +++ b/src/core/__tests__/refs-test.js @@ -44,7 +44,7 @@ var ClickCounter = React.createClass({ var i; for (i=0; i < this.state.count; i++) { children.push( -
+ var flow = [ + 'a', 'abbr', 'address', 'area', 'article', 'aside', 'audio', 'b', 'bdi', + 'bdo', 'blockquote', 'br', 'button', 'canvas', 'cite', 'code', 'data', + 'datalist', 'del', 'details', 'dfn', 'div', 'dl', 'em', 'embed', + 'fieldset', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', + 'header', 'hr', 'i', 'iframe', 'img', 'input', 'ins', 'kbd', 'keygen', + 'label', 'link', 'main', 'map', 'mark', 'menu', 'meta', 'meter', 'nav', + 'noscript', 'object', 'ol', 'output', 'p', 'pre', 'progress', 'q', 'ruby', + 's', 'samp', 'script', 'section', 'select', 'small', 'span', 'strong', + 'style', 'sub', 'sup', 'svg', 'table', 'textarea', 'time', 'u', 'ul', + 'var', 'video', 'wbr', '#text' + ]; + + // Phrasing elements are inline elements that can appear in a + var phrase = [ + 'a', 'abbr', 'area', 'audio', 'b', 'bdi', 'bdo', 'br', 'button', 'canvas', + 'cite', 'code', 'data', 'datalist', 'del', 'dfn', 'em', 'embed', 'i', + 'iframe', 'img', 'input', 'ins', 'kbd', 'keygen', 'label', 'link', 'map', + 'mark', 'meta', 'meter', 'noscript', 'object', 'output', 'progress', 'q', + 'ruby', 's', 'samp', 'script', 'select', 'small', 'span', 'strong', 'sub', + 'sup', 'svg', 'textarea', 'time', 'u', 'var', 'video', 'wbr', '#text' + ]; + + // Metadata elements can appear in + var metadata = [ + 'base', 'link', 'meta', 'noscript', 'script', 'style', 'title' + ]; + + // By default, we assume that flow elements can contain other flow elements + // and phrasing elements can contain other phrasing elements. Here are the + // exceptions: + var allowedChildren = { + 'a': flow, + 'audio': ['source', 'track'].concat(flow), + 'body': flow, + 'button': phrase, + 'caption': flow, + 'canvas': flow, + 'colgroup': ['col'], + 'dd': flow, + 'del': flow, + 'details': ['summary'].concat(flow), + 'dl': ['dt', 'dd'], + 'dt': flow, + 'fieldset': flow, + 'figcaption': flow, + 'figure': ['figcaption'].concat(flow), + 'h1': phrase, + 'h2': phrase, + 'h3': phrase, + 'h4': phrase, + 'h5': phrase, + 'h6': phrase, + 'head': metadata, + 'hgroup': ['h1', 'h2', 'h3', 'h4', 'h5', 'h6'], + 'html': ['body', 'head'], + 'iframe': [], + 'ins': flow, + 'label': phrase, + 'legend': phrase, + 'li': flow, + 'map': flow, + 'noscript': ['*'], + 'object': ['param'].concat(flow), + 'ol': ['li'], + 'optgroup': ['option'], + 'p': phrase, + 'pre': phrase, + 'rp': phrase, + 'rt': phrase, + 'ruby': ['rp', 'rt', '#text'], + 'script': ['#text'], + 'select': ['option', 'optgroup'], + 'style': ['#text'], + 'summary': phrase, + 'table': ['caption', 'colgroup', 'tbody', 'tfoot', 'thead'], + 'tbody': ['tr'], + 'td': flow, + 'textarea': ['#text'], + 'tfoot': ['tr'], + 'th': flow, + 'thead': ['tr'], + 'title': ['#text'], + 'tr': ['td', 'th'], + 'ul': ['li'], + 'video': ['source', 'track'].concat(flow), + + // SVG + // TODO: Validate nesting of all svg elements + 'svg': [ + 'circle', 'defs', 'g', 'line', 'linearGradient', 'path', 'polygon', + 'polyline', 'radialGradient', 'rect', 'stop', 'text' + ], + + // Self-closing tags + 'area': [], + 'base': [], + 'br': [], + 'col': [], + 'embed': [], + 'hr': [], + 'img': [], + 'input': [], + 'keygen': [], + 'link': [], + 'menuitem': [], + 'meta': [], + 'param': [], + 'source': [], + 'track': [], + 'wbr': [] + }; + + var allowedChildrenMap = {}; + for (var el in allowedChildren) { + if (allowedChildren.hasOwnProperty(el)) { + allowedChildrenMap[el] = createObjectFrom(allowedChildren[el]); + } + } + + // Fall back to phrasing first because all phrasing elements are flow + // elements as well + var phraseMap = createObjectFrom(phrase); + for (var i = 0, l = phrase.length; i < l; i++) { + if (!allowedChildrenMap.hasOwnProperty(phrase[i])) { + allowedChildrenMap[phrase[i]] = phraseMap; + } + } + + var flowMap = createObjectFrom(flow); + for (var i = 0, l = flow.length; i < l; i++) { + if (!allowedChildrenMap.hasOwnProperty(flow[i])) { + allowedChildrenMap[flow[i]] = flowMap; + } + } + var nodeCanContainNode = function(parentNodeName, childNodeName) { - parentNodeName = parentNodeName.toLowerCase(); - childNodeName = childNodeName.toLowerCase(); - - if (childNodeName === 'p') { - return parentNodeName !== 'p'; - } else if (childNodeName === 'tr') { - return ['tbody', 'tfoot', 'thead'].indexOf(parentNodeName) !== -1; - } else if (['tbody', 'tfoot', 'thead'].indexOf(childNodeName) !== -1) { - return parentNodeName === 'table'; + var allowed = allowedChildrenMap[parentNodeName]; + if (allowed == null) { + return true; } - return true; + + var result = allowed[childNodeName] || allowed['*']; + return !!result; }; validateNodeNesting = function(parentNodeName, childNodeName) { if (__DEV__) { + parentNodeName = parentNodeName.toLowerCase(); + childNodeName = childNodeName.toLowerCase(); if (!nodeCanContainNode(parentNodeName, childNodeName)) { console.warn( - 'validateNodeNesting(...): Node of type ' + - parentNodeName.toLowerCase() + ' cannot contain node of type ' + - childNodeName.toLowerCase() + '.' + 'validateNodeNesting(...): Node of type ' + parentNodeName + + ' cannot contain node of type ' + childNodeName + '.' ); } } From 94be04cab512cd861e00ccf57ede87face06ef1d Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 10 Feb 2014 23:55:41 -0800 Subject: [PATCH 5/6] Tweak invalid-nesting warning message --- src/core/__tests__/ReactDOMComponent-test.js | 3 +-- src/dom/validateNodeNesting.js | 13 +++++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/core/__tests__/ReactDOMComponent-test.js b/src/core/__tests__/ReactDOMComponent-test.js index f4b454cb48b0b..f46db3c831d91 100644 --- a/src/core/__tests__/ReactDOMComponent-test.js +++ b/src/core/__tests__/ReactDOMComponent-test.js @@ -214,8 +214,7 @@ describe('ReactDOMComponent', function() { expect(console.warn.argsForCall.length).toBe(1); expect(console.warn.argsForCall[0][0]).toBe( - 'validateNodeNesting(...): Node of type div cannot contain node of ' + - 'type tr.' + 'validateNodeNesting(...):
cannot contain a node.' ); }); }); diff --git a/src/dom/validateNodeNesting.js b/src/dom/validateNodeNesting.js index 7322fe4450849..19a777272bd41 100644 --- a/src/dom/validateNodeNesting.js +++ b/src/dom/validateNodeNesting.js @@ -180,10 +180,15 @@ if (__DEV__) { parentNodeName = parentNodeName.toLowerCase(); childNodeName = childNodeName.toLowerCase(); if (!nodeCanContainNode(parentNodeName, childNodeName)) { - console.warn( - 'validateNodeNesting(...): Node of type ' + parentNodeName + - ' cannot contain node of type ' + childNodeName + '.' - ); + var message = + 'validateNodeNesting(...): <' + parentNodeName + '> cannot ' + + 'contain a <' + childNodeName + '> node.'; + if (parentNodeName === 'table' && childNodeName === 'tr') { + message += + ' Add a to your code to match the DOM tree generated by ' + + 'the browser.'; + } + console.warn(message); } } }; From 6de250cb7be938f1f4df2427db33035f84280a11 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Tue, 11 Feb 2014 00:27:11 -0800 Subject: [PATCH 6/6] Remove joinAccumulated module; always use array --- src/browser/ReactDOMComponent.js | 25 ++++++++-------- src/core/__tests__/ReactDOMComponent-test.js | 3 +- src/utils/joinAccumulated.js | 30 -------------------- 3 files changed, 14 insertions(+), 44 deletions(-) delete mode 100644 src/utils/joinAccumulated.js diff --git a/src/browser/ReactDOMComponent.js b/src/browser/ReactDOMComponent.js index 9f0e9098bce7d..a8c94c100ef52 100644 --- a/src/browser/ReactDOMComponent.js +++ b/src/browser/ReactDOMComponent.js @@ -30,13 +30,11 @@ var ReactPerf = require('ReactPerf'); var escapeTextForBrowser = require('escapeTextForBrowser'); var invariant = require('invariant'); -var joinAccumulated = require('joinAccumulated'); var keyOf = require('keyOf'); var merge = require('merge'); var mixInto = require('mixInto'); if (__DEV__) { - var forEachAccumulated = require('forEachAccumulated'); var getNodeNameFromReactMarkup = require('getNodeNameFromReactMarkup'); var validateNodeNesting = require('validateNodeNesting'); } @@ -123,15 +121,18 @@ ReactDOMComponent.Mixin = { assertValidProps(this.props); var contentMarkup = this._createContentMarkup(transaction); if (__DEV__) { - var tagName = this.tagName; - forEachAccumulated(contentMarkup, function(markup) { - var nodeName = getNodeNameFromReactMarkup(markup); - validateNodeNesting(tagName, nodeName); - }); + if (contentMarkup) { + var tagName = this.tagName; + for (var i = 0, l = contentMarkup.length; i < l; i++) { + var markup = contentMarkup[i]; + var nodeName = getNodeNameFromReactMarkup(markup); + validateNodeNesting(tagName, nodeName); + } + } } return ( this._createOpenTagMarkupAndPutListeners(transaction) + - joinAccumulated(contentMarkup, '') + + (contentMarkup ? contentMarkup.join('') : '') + this._tagClose ); } @@ -187,21 +188,21 @@ ReactDOMComponent.Mixin = { * * @private * @param {ReactReconcileTransaction} transaction - * @return {string|array} Content markup or list of content markup. + * @return {array} Content markup or list of content markup. */ _createContentMarkup: function(transaction) { // Intentional use of != to avoid catching zero/false. var innerHTML = this.props.dangerouslySetInnerHTML; if (innerHTML != null) { if (innerHTML.__html != null) { - return innerHTML.__html; + return [innerHTML.__html]; } } else { var contentToUse = CONTENT_TYPES[typeof this.props.children] ? this.props.children : null; var childrenToUse = contentToUse != null ? null : this.props.children; if (contentToUse != null) { - return escapeTextForBrowser(contentToUse); + return [escapeTextForBrowser(contentToUse)]; } else if (childrenToUse != null) { var mountImages = this.mountChildren( childrenToUse, @@ -210,7 +211,7 @@ ReactDOMComponent.Mixin = { return mountImages; } } - return ''; + return null; }, receiveComponent: function(nextComponent, transaction) { diff --git a/src/core/__tests__/ReactDOMComponent-test.js b/src/core/__tests__/ReactDOMComponent-test.js index f46db3c831d91..a220f8377b741 100644 --- a/src/core/__tests__/ReactDOMComponent-test.js +++ b/src/core/__tests__/ReactDOMComponent-test.js @@ -276,7 +276,6 @@ describe('ReactDOMComponent', function() { var ReactDOMComponent = require('ReactDOMComponent'); var ReactReconcileTransaction = require('ReactReconcileTransaction'); - var joinAccumulated = require('joinAccumulated'); var mixInto = require('mixInto'); var NodeStub = function(initialProps) { @@ -288,7 +287,7 @@ describe('ReactDOMComponent', function() { genMarkup = function(props) { var transaction = new ReactReconcileTransaction(); var markup = (new NodeStub(props))._createContentMarkup(transaction); - return joinAccumulated(markup, ''); + return markup.join(''); }; this.addMatchers({ diff --git a/src/utils/joinAccumulated.js b/src/utils/joinAccumulated.js deleted file mode 100644 index ea2cbdb02e2d3..0000000000000 --- a/src/utils/joinAccumulated.js +++ /dev/null @@ -1,30 +0,0 @@ -/** - * Copyright 2013 Facebook, Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - * @providesModule joinAccumulated - */ - -"use strict"; - -var joinAccumulated = function(arr, sep) { - if (Array.isArray(arr)) { - return arr.join(sep); - } else if (arr != null) { - return String(arr); - } - return ''; -}; - -module.exports = joinAccumulated;