Handle className correctly for SVG nodes

[fforw]

Bring back the DOM mutator to check if the className we're writing
to is actually a string.

For SVG nodes, the className property is not of type string but
and SVGAnimatedString and writing to the property will cause
a "TypeError: setting a property that has only a getter"
(See http://jsfiddle.net/ca47V/ )

This first checks if the className property is indeed a string
and uses setAttribute() / removeAttribute() otherwise.

[sophiebits]

Thanks! Do you think you could add a couple tests and comments explaining the purpose and to make sure this works?

[fforw]

How would I go about to prove above code does indeed work on SVG nodes with your test system?

[sophiebits]

You should be able to render an SVG node then set its className and verify that the class was changed properly. See DOMPropertyOperations-test.js for our existing tests.

[fforw]

Ok?

[sophiebits]

Did you run the tests? I would think expect(svgNode.className).toBe('foo'); would fail because .className should be an object, not a string -- right?

[fforw]

Yes, I ran the tests :\

In a real browser, svgNode.className is of course not the string foo.

The tests seem rather superflous here. Removing the original patch, but keepin the tests executes, too :\

In short: we're testing a wrong browser emulation..

[fforw]

Both my firefox and my chrome agree to not do anything if I set the className property. Firefox will only give out the warning in strict mode (!).

setAttribute works in both. ( http://fforw.de/static/files/svgcls.html )

[fforw]

Again, the tests describe what a real browser does and your test environment obviously does not.

[sophiebits]

I understand. I guess it's not practical to test this in phantomjs, so I'm fine landing without tests.

[fforw]

So should I remove the test completely?

[sophiebits]

I think that's best unless you have a better idea.

[fforw]

Well.. I guess we could just try to file a bug against phantomjs.. I mean there's no reason they shouldn't emulate this kind of thing correctly like a browser does. The behaviour being a direct result of the SVG 1.1 specs and the integration of that into HTML.

I'm kind of surprised react doesn't already have some kind of "real browser test" setup. Is it because this is basically done indirectly by internal facebook testing resources being used?

Apart from introducing such a thing for the react project itself I see little we can do here.

[sophiebits]

phantomjs is actually using a really old version of webkit so I'm not that hopeful. We do have a way to run tests in real browsers via saucelabs but right now all the tests are meant to pass in phantom.

[fforw]

At least we can try ariya/phantomjs#12072

[sophiebits]

For consistency with our handling for other DOM properties, this should be value != null ? value : ''.

[sophiebits]

I think we might be best just leaving off the tests here.

[barnabyc]

+1 for getting this into the next release if possible as I ran into the same issue on several projects this week

[fforw]

Rewrote PR to honor commented on style issues and removed tests again.

Test was:

diff --git a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
index 83e4416..62a329c 100644
--- a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
+++ b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
@@ -219,6 +219,32 @@ describe('DOMPropertyOperations', function() {
       expect(stubNode.className).toBe('');
     });

+    it('should set className properly on SVG nodes', function() {
+
+      var svgNode = document.createElement("g");
+      DOMPropertyOperations.setValueForProperty(
+        svgNode,
+        'className',
+        'foo'
+      );
+
+      expect(typeof svgNode.className).toBe('object');
+      expect(svgNode.className.baseVal).toBe('foo');
+    });
+
+    it('should set className to empty string instead of null on SVG nodes', function() {
+
+      var svgNode = document.createElement("g");
+      DOMPropertyOperations.setValueForProperty(
+        svgNode,
+        'className',
+        null
+      );
+
+      expect(typeof svgNode.className).toBe('object');
+      expect(svgNode.className.baseVal).toBe('');
+    });
+
   });

   describe('injectDOMPropertyConfig', function() {

[sophiebits]

Looks good to me, thanks.

[fforw]

Since we haven't received any reaction from the phantomjs guys in two weeks, maybe we should just give up on that for now. We can always readd the tests if either React's test-strategy or phantomjs handling of this issue change.

[zpao]

nit: we use single quotes for strings

Let's also add a comment explaining why we're doing this detection. And while you're doing a comment, try to do sentences as much as possible.

[fforw]

Added more comment, changed to single quotes.

[fforw]

So.. how do we go on here?

[barnabyc]

@spicyj any chance of a merge as Travis is green? /cc @zpao

[freiksenet]

I've made a version that merges to current master:
freiksenet@bdc427a

Should I submit new pull request? Should someone merge this? This fixes important bug in SVG support, I think.

[davidaurelio]

Please merge this.

[syranide]

I propose escalating #1449 as it would solve all of these SVG-issues, once and for all, if the devs agree that it's a good idea.

[zpao]

Ok, so after some more thoughts, I think what we have here is not going to be great. It touches the DOM on every update before actually doing the update. I ran a bunch of perf tests and using the attribute seems to be marginally slower for setting, faster for deleting. I think that's probably the best option, however that doesn't work in IE8, so we need to do something different there. Perhaps we can default to MUST_USE_ATTRIBUTE and if we're in IE8 change it? @yungsters, what do you think?

[yungsters]

I pretty much agree with @syranide. Whatever solution we come up with — as long as it is not horrible — will probably be good enough to tie us over until we have per-node configurations. Then we can do whatever needs to be done for SVG (or any other unique) elements without impacting the 90% use case.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sophiebits]

Just put up #1590 which is an alternative approach.

[zpao]

Closing in favor of #1590. Thanks for taking a stab at this @fforw and I'm sorry we stalled on it!