Pass parent tag to text component for SVG

[nhunzaker]

I realize this is quite a non-trivial change to React internals, however this PR adds an additional argument to mountComponent that passes in the parent tag name. This fixes issues where <span> tags are injected into <text> svg elements, nuking SVG graphics as demonstrated here:

http://jsfiddle.net/22oow6ed/1/

After this change I updated the basic example and the result is as one would expect:

Frankly, passing in an additional argument feels wrong. I am curious if there is a better way to pass the required contextual information to TextComponent. Thoughts?

---

Fix #1236

[chicoxyzzy]

template strings are ES6 feature so this will not work for some target browsers

[sophiebits]

We compile the code down to ES3 before shipping it, so this syntax is fine.

[zpao]

Hmm, I'm ok with this in principal, but I'm not wild about putting DOM specific things into core like this. This definitely isn't the first thing but we're also trying to move in the direction where core is pure and reusable between web and react-native. @spicyj you've worked on both so you might have a better perspective on if this sort of change would set back that goal or if it's manageable. (also @sebmarkbage)

[nhunzaker]

@zpao If this doesn't make it in, I'd lobby for a warning of some kind. It's sort of alarming and confusing when you see the text from your SVG graphic appear below the containing element!

[sophiebits]

We could use context for this:

diff --git a/src/browser/ui/ReactDOMComponent.js b/src/browser/ui/ReactDOMComponent.js
index 4df78e8..7f4fd9e 100644
--- a/src/browser/ui/ReactDOMComponent.js
+++ b/src/browser/ui/ReactDOMComponent.js
@@ -301,7 +301,7 @@ ReactDOMComponent.Mixin = {
         var mountImages = this.mountChildren(
           childrenToUse,
           transaction,
-          context
+          assign({}, context, {svgTextChild: this._tag === 'text'})
         );
         ret = mountImages.join('');
       }
@@ -342,7 +342,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,
+      assign({}, context, {svgTextChild: this._tag === 'text'})
+    );
   },

   /**
diff --git a/src/browser/ui/ReactDOMTextComponent.js b/src/browser/ui/ReactDOMTextComponent.js
index d561646..46f8817 100644
--- a/src/browser/ui/ReactDOMTextComponent.js
+++ b/src/browser/ui/ReactDOMTextComponent.js
@@ -75,10 +75,12 @@ assign(ReactDOMTextComponent.prototype, {
       return escapedText;
     }

+    var tag = context.svgTextChild ? 'tspan' : 'span';
+
     return (
-      '<span ' + DOMPropertyOperations.createMarkupForID(rootID) + '>' +
+      '<' + tag + ' ' + DOMPropertyOperations.createMarkupForID(rootID) + '>' +
         escapedText +
-      '</span>'
+      '</' + tag + '>'
     );
   },

though we'd need to make the name not clash with user-defined context keys and change the code to be smarter about not copying. Works as a proof of concept though.

[nhunzaker]

@spicyj I rather quite like that. I'm still getting grips on context, but I like it!

[nhunzaker]

I went ahead with that diff to keep the ball rolling. I'll also experiment with eliminating the unnecessary copies with assign.

[nhunzaker]

With the new updates in React 15.x, this PR seems no longer valid. I'm going to close it.

[sophiebits]

Yeah, I ended up solving this a different way. Thanks for sending this in though – though we didn't merge it, I appreciate the discussion and we may not have fixed this in 15.0 otherwise.

[nhunzaker]

No worries! I'm glad the fix is in!