Switch to using classList instead of className #448

horyd · 2016-06-09T14:57:47Z

The property className returns an object on some (possibly all?) SVG elements (despite stating in the linked documentation that it always returns a string. Maybe there is a different SVG implementation that I missed...). The object behaves under the interface SVGAnimatedString, which does not have method replace. If you have an SVG element somewhere within the mounted component then, as it iterates during a find, it processes the SVG element and throws TypeError: classes.replace is not a function.

I tested classList and it behaves correctly for SVG elements and regular elements alike. It also has a handy contains method too.

aweary · 2016-06-09T15:04:32Z

src/MountedTraversal.js

-  let classes = findDOMNode(inst).className || '';
-  classes = classes.replace(/\s/g, ' ');
-  return ` ${classes} `.indexOf(` ${className} `) > -1;
+  let classes = findDOMNode(inst).classList;


please use const instead of let if the value is never reassigned.

❌ This will break in browsers that do not support classList, which includes IE 8 and 9, IE 10 and 11 for SVG (according to http://caniuse.com/#search=classlist), etc.

We simply must not use classList unconditionally - feature-detecting its support and falling back would be fine though.

Ah, yep good find, didn't think to check the browser support. Given that, and also in the interest of merging prior to #421 since this is still something that currently breaks testing, would just adding a check that classes is of type String, and returning false if it isn't, be enough?

That would cripple existing functionality in those browsers where classList is unsupported. enzyme needs to not just "not break" in older browsers, it needs to "work as perfectly as possible".

So the question is, does implementing classList provide any benefits over the current implementation? Since we'll have to implement a method to check className for SVGs as well, is there a sufficient advantage here to include a check and multiple code paths?

The only advantages I can see are a) if the className approach can't be made to work, or b) in the far future when we could drop className support, we'd be able to cleanly delete a branch of code.

Sorry, I mean in my previous comment to add the type checking whilst keeping the old className approach. Updated in latest commit.

aweary · 2016-06-09T15:10:10Z

👍 I think this is a good approach, very simple. Ideally, we'd add some additional tests to verify this fixes SVG support, but until #421 is merged we can't test with actual SVG instances, so I think this is OK for now if it doesn't break any existing behavior.

nfcampos · 2016-06-09T18:09:59Z

#421 should be merged before we can merge something like this, so that we don't pull in something we can't test

aweary · 2016-06-11T16:00:46Z

src/MountedTraversal.js

@@ -50,6 +50,9 @@ export function instHasClassName(inst, className) {
    return false;
  }
  let classes = findDOMNode(inst).className || '';
+  if (typeof classes !== 'string') {
+    return false;


This would just cause SVGs to silently fail, I don't think that's what we want. What we should do is branch on classList and use the old code path when it doesn't exist.

export function instHasClassName(inst, className) { if (!isDOMComponent(inst)) { return false; } const node = findDOMNode(inst); if (node.classList) { return node.classList.contains(className) } let classes = node.className || ''; if (typeof classes === 'object') { classes = classes.baseVal; } classes = classes.replace(/\s/g, ' '); return ` ${classes} `.indexOf(` ${className} `) > -1; }

That snippet above should introduce support for SVGs in all cases as well. We could also invert the if block if we wanted to be able to cleanly delete the className code in the future.

(Note that node.classList could be an empty string, so checking it for truthiness is not sufficient - also, rather than using string operations, why not the equivalent of .split(/\s*/g).includes(className.trim())?)

The classList property is a DOMTokenList (docs), so I think the truthiness test is okay?

@ljharb the string operations are the current implementation, I just added the classList check. We can definitely look at implementing the className branch differently though.

I have committed your suggested code @aweary. Kept the string operations as it stands, turns out Array.includes isn't supported by IE/Edge

ljharb · 2016-06-13T14:45:06Z

This will need a rebase before merging, to get rid of the merge commit.

horyd · 2016-06-14T17:25:18Z

Rebased 👍

ljharb · 2016-06-19T15:59:22Z

Can we add some tests that would fail (even if it's only in a few browsers) without this change?

horyd · 2016-06-20T10:51:33Z

Okay, test added and commits squashed.

I've confirmed that the test fails on branch karma (at ad37536), and passes when the fix is applied. The test passes regularly within the current test framework, with and without the fix, since the error is specific to browser environments.

I wanted to leave a quick note on IE; seeing that the SVGAnimatedString docs state no support for Internet Explorer. I checked on MSDN for details of their implementation of the className property (see here) and it states that it is also an Object, but nothing further. I'm not sure what the interface for this object is.

But seeing that the current implementation doesn't really change anything (instead just adding the capability to handle it in environments where an issue has been reported), I think this is good to go?

horyd · 2016-06-27T09:20:00Z

Anything else I need to add to this?

aweary · 2016-06-27T16:35:20Z

@horyd looks like you need to merge the changes from the master branch.

aweary · 2016-06-27T16:45:04Z

@horyd I tested className for svgs in IE9/10/11 and className returned an SVGAnimatedString object, so it seems it is supported.

horyd · 2016-06-27T21:56:12Z

@aweary Great, thanks for checking that! Should be good to go now :)

horyd · 2016-07-04T17:21:55Z

Hey guys, rebased this maybe half a dozen times now.. would love to get it merged :)

aweary · 2016-07-04T17:47:04Z

@horyd LGTM, but our tests seem to be failing in Node 0.10, so we'll need to address that first

tb · 2016-07-05T09:37:08Z

@aweary I have downloaded @horyd branch locally and have all tests pass on v0.10.37.

The error on CI is:

/home/travis/build/airbnb/enzyme/node_modules/eslint-plugin-import/lib/index.js:6
const rules = exports.rules = {
^^^^^

The CI have used eslint-plugin-import 1.10.1 (I checked the logs). On my local machine I have 1.10.2.

I think setting: "eslint-plugin-import": "^1.10.2" in package.json will fix the CI.

BTW I have also tested the @horyd PR in my app - it fixed my issue with mount and material-ui SelectField (which includes SVG icon element).

aweary · 2016-07-05T14:32:35Z

Thanks @tb, we actually cleared up the linting issue in #482

@horyd hate to do this to you, but if you could update this with master once more I think we can get this merged afterwards.

The property className returns an object on some (possibly all?) SVG elements. The object behaves under the interface SVGAnimatedString, which does not have method "replace". typeof check conditional use of classList, object check on className Test added for SVG find by class

horyd · 2016-07-05T19:49:40Z

@aweary Done! :)

aweary · 2016-07-05T19:56:28Z

Thanks again @horyd!

horyd · 2016-07-06T07:21:24Z

No worries! 😃

horyd mentioned this pull request Jun 9, 2016

Implement SVG support #375

Closed

aweary reviewed Jun 9, 2016
View reviewed changes

aweary reviewed Jun 11, 2016
View reviewed changes

trmjoa mentioned this pull request Jun 13, 2016

hasClassName throws Error if component has property className which is not a string #453

Closed

aweary mentioned this pull request Jun 27, 2016

createElementNS not implementing SVG interface correctly jsdom/jsdom#1480

Closed

aweary merged commit e6ee9d7 into enzymejs:master Jul 5, 2016

ljharb added enhancement semver: minor New stuff. API: mount labels Jul 6, 2016

tatyshev mentioned this pull request Nov 23, 2017

Not throw an error if element.className is SVGAnimatedString eddyerburgh/avoriaz#167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using classList instead of className #448

Switch to using classList instead of className #448

horyd commented Jun 9, 2016

aweary Jun 9, 2016

ljharb Jun 9, 2016

horyd Jun 9, 2016

ljharb Jun 10, 2016

aweary Jun 10, 2016 •

edited

Loading

ljharb Jun 10, 2016

horyd Jun 10, 2016

aweary commented Jun 9, 2016

nfcampos commented Jun 9, 2016

aweary Jun 11, 2016 •

edited

Loading

aweary Jun 11, 2016

ljharb Jun 12, 2016

horyd Jun 12, 2016

aweary Jun 12, 2016

horyd Jun 13, 2016

ljharb commented Jun 13, 2016

horyd commented Jun 14, 2016

ljharb commented Jun 19, 2016

horyd commented Jun 20, 2016

horyd commented Jun 27, 2016

aweary commented Jun 27, 2016

aweary commented Jun 27, 2016

horyd commented Jun 27, 2016

horyd commented Jul 4, 2016

aweary commented Jul 4, 2016

tb commented Jul 5, 2016 •

edited

Loading

aweary commented Jul 5, 2016

horyd commented Jul 5, 2016

aweary commented Jul 5, 2016

horyd commented Jul 6, 2016

Switch to using classList instead of className #448

Switch to using classList instead of className #448

Conversation

horyd commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Jun 9, 2016

nfcampos commented Jun 9, 2016

aweary Jun 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jun 13, 2016

horyd commented Jun 14, 2016

ljharb commented Jun 19, 2016

horyd commented Jun 20, 2016

horyd commented Jun 27, 2016

aweary commented Jun 27, 2016

aweary commented Jun 27, 2016

horyd commented Jun 27, 2016

horyd commented Jul 4, 2016

aweary commented Jul 4, 2016

tb commented Jul 5, 2016 • edited Loading

aweary commented Jul 5, 2016

horyd commented Jul 5, 2016

aweary commented Jul 5, 2016

horyd commented Jul 6, 2016

aweary Jun 10, 2016 •

edited

Loading

aweary Jun 11, 2016 •

edited

Loading

tb commented Jul 5, 2016 •

edited

Loading