From 9bc3ee56ae4b0034c2284e458e6f864a06a4d9a5 Mon Sep 17 00:00:00 2001 From: Andreas Svensson Date: Sat, 27 Sep 2014 18:22:19 +0200 Subject: [PATCH] ReactMount should not optimistically traverse unknown nodes --- perf/lib/BrowserPerfRunnerApp.react.js | 6 ++- perf/lib/perf-test-runner.browser.js | 16 +++--- src/browser/ui/ReactMount.js | 50 +++++++++++-------- .../__tests__/ReactInstanceHandles-test.js | 31 +++--------- 4 files changed, 48 insertions(+), 55 deletions(-) diff --git a/perf/lib/BrowserPerfRunnerApp.react.js b/perf/lib/BrowserPerfRunnerApp.react.js index 5b34f3bcaaec5..9d25e700b8843 100644 --- a/perf/lib/BrowserPerfRunnerApp.react.js +++ b/perf/lib/BrowserPerfRunnerApp.react.js @@ -197,8 +197,10 @@ var GridViewTable = React.createClass({ render: function(){ return React.DOM.table(null, - this._renderRow(null, 0), - this.props.rows.map(this._renderRow, this) + React.DOM.tbody(null, + this._renderRow(null, 0), + this.props.rows.map(this._renderRow, this) + ) ); } diff --git a/perf/lib/perf-test-runner.browser.js b/perf/lib/perf-test-runner.browser.js index 26e059f8c38c7..0579c2e7fe3a3 100644 --- a/perf/lib/perf-test-runner.browser.js +++ b/perf/lib/perf-test-runner.browser.js @@ -195,10 +195,14 @@ perfRunner.ViewObject = function(props){ if (typeof value != 'object') return React.DOM.span(props, [JSON.stringify(value), " ", typeof value]); - return React.DOM.table(props, Object.keys(value).map(function(key){ - return React.DOM.tr(null, - React.DOM.th(null, key), - React.DOM.td(null, perfRunner.ViewObject({key:key, value:value[key]})) - ); - })); + return React.DOM.table(props, + React.DOM.tbody(null, + Object.keys(value).map(function(key){ + return React.DOM.tr(null, + React.DOM.th(null, key), + React.DOM.td(null, perfRunner.ViewObject({key:key, value:value[key]})) + ); + }) + ) + ); } diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 65eb9902cc901..891bcdca9191f 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -610,29 +610,35 @@ var ReactMount = { while (child) { var childID = ReactMount.getID(child); - if (childID) { - // Even if we find the node we're looking for, we finish looping - // through its siblings to ensure they're cached so that we don't have - // to revisit this node again. Otherwise, we make n^2 calls to getID - // when visiting the many children of a single node in order. - - if (targetID === childID) { - targetChild = child; - } else if (ReactInstanceHandles.isAncestorIDOf(childID, targetID)) { - // If we find a child whose ID is an ancestor of the given ID, - // then we can be sure that we only want to search the subtree - // rooted at this child, so we can throw out the rest of the - // search state. - firstChildren.length = childIndex = 0; - firstChildren.push(child.firstChild); - } - } else { - // If this child had no ID, then there's a chance that it was - // injected automatically by the browser, as when a `` - // element sprouts an extra `` child as a side effect of - // `.innerHTML` parsing. Optimistically continue down this - // branch, but not before examining the other siblings. + if (!childID) { + invariant( + false, + 'findComponentRoot(..., %s): Encountered element without React ' + + 'ID. ' + + 'This probably means the DOM was unexpectedly mutated (e.g., by ' + + 'the browser), usually due to forgetting a when using ' + + 'tables, nesting tags like ,

, or , or using non-SVG ' + + 'elements in an parent. ' + + 'Try inspecting the child nodes of the element with React ID `%s`.', + targetID, + ReactMount.getID(ancestorNode) + ); + } + + // Even if we find the node we're looking for, we finish looping + // through its siblings to ensure they're cached so that we don't have + // to revisit this node again. Otherwise, we make n^2 calls to getID + // when visiting the many children of a single node in order. + + if (targetID === childID) { + targetChild = child; + } else if (ReactInstanceHandles.isAncestorIDOf(childID, targetID)) { + // If we find a child whose ID is an ancestor of the given ID, + // then we can be sure that we only want to search the subtree + // rooted at this child, so we can throw out the rest of the + // search state. + firstChildren.length = childIndex = 0; firstChildren.push(child.firstChild); } diff --git a/src/core/__tests__/ReactInstanceHandles-test.js b/src/core/__tests__/ReactInstanceHandles-test.js index bd8a4ff6debc1..b4b4ea373c0ee 100644 --- a/src/core/__tests__/ReactInstanceHandles-test.js +++ b/src/core/__tests__/ReactInstanceHandles-test.js @@ -103,25 +103,6 @@ describe('ReactInstanceHandles', function() { ).toBe(childNodeB); }); - it('should work around unidentified nodes', function() { - var parentNode = document.createElement('div'); - var childNodeA = document.createElement('div'); - var childNodeB = document.createElement('div'); - parentNode.appendChild(childNodeA); - parentNode.appendChild(childNodeB); - - ReactMount.setID(parentNode, '.0'); - // No ID on `childNodeA`. - ReactMount.setID(childNodeB, '.0.0:1'); - - expect( - ReactMount.findComponentRoot( - parentNode, - ReactMount.getID(childNodeB) - ) - ).toBe(childNodeB); - }); - it('should throw if a rendered element cannot be found', function() { var parentNode = document.createElement('table'); var childNodeA = document.createElement('tbody'); @@ -130,8 +111,8 @@ describe('ReactInstanceHandles', function() { childNodeA.appendChild(childNodeB); ReactMount.setID(parentNode, '.0'); - // No ID on `childNodeA`, it was "rendered by the browser". - ReactMount.setID(childNodeB, '.0.1:0'); + ReactMount.setID(childNodeA, '.0.1:0'); + ReactMount.setID(childNodeB, '.0.1:0.0'); expect(ReactMount.findComponentRoot( parentNode, @@ -141,7 +122,7 @@ describe('ReactInstanceHandles', function() { expect(function() { ReactMount.findComponentRoot( parentNode, - ReactMount.getID(childNodeB) + ":junk" + ReactMount.getID(childNodeA) + ":junk" ); }).toThrow( 'Invariant Violation: findComponentRoot(..., .0.1:0:junk): ' + @@ -326,7 +307,7 @@ describe('ReactInstanceHandles', function() { it("should return next descendent from window", function() { var parent = renderParentIntoDocument(); expect( - ReactInstanceHandles._getNextDescendantID( + ReactInstanceHandles.getNextDescendantID( '', parent.refs.P_P1._rootNodeID ) @@ -334,13 +315,13 @@ describe('ReactInstanceHandles', function() { }); it("should return window for next descendent towards window", function() { - expect(ReactInstanceHandles._getNextDescendantID('', '')).toBe(''); + expect(ReactInstanceHandles.getNextDescendantID('', '')).toBe(''); }); it("should return self for next descendent towards self", function() { var parent = renderParentIntoDocument(); expect( - ReactInstanceHandles._getNextDescendantID( + ReactInstanceHandles.getNextDescendantID( parent.refs.P_P1._rootNodeID, parent.refs.P_P1._rootNodeID )