search

ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

toggleClass appends existing classes to className #106

Open tboothman opened 11 years ago

tboothman commented 11 years ago

If the browser does not support classList toggleClass does not have the same behaviour. If the toggled class already exists on an element toggleClass will add that same class to the className string, which means the class can never be removed.

This is caused by the internal addClass function not checking for the new class existing in the className string before appending.

I suppose the fix would be something like:

addClass = function (el, c) {
    if (!hasClass(el,c) {
        el.className = trim(el.className + ' ' + c);
    }
}

That would unfortunately make the addClass API call hasClass twice. However, given that add/removeClass API functions now have the same functionality as their classList equivalents the hasClass checks are now unnecessary and can be removed.

ded commented 11 years ago

thanks for the bug report. i will figure something out

symmetriq commented 11 years ago

This is still happening in Safari (or maybe it's a new issue), but only when the class name contains capital letters. Instead of removing the class name, the same one just gets appended each time. Turns out hasClass('derpDerp') returns false even when the class derpDerp is present.

Here's the userAgent for the version I'm seeing this in: Mozilla/5.0 (iPad; CPU OS 6_1_2 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Version/6.0 Mobile/10B146 Safari/8536.25

I noticed the problem in Bonzo 1.2.3, but I updated to Bonzo 1.3.4 and it still does the same thing.

symmetriq commented 11 years ago

Figured it out, and it looks like it's a bug in Safari's DOM interface (occurs in both the desktop and mobile versions). Still broken in Safari 6.0.3 (which shipped with OS X 10.8.3).

Class names that are added either via setting the className property directly, or via .classList.add() (as Bonzo's addClass() method does) show up in the classList array in lowercase (regardless of capitalization). Capitalization is preserved correctly in class names that are present when the page loads.

Anyway, the end result is Bonzo's hasClass() test will then fail when it should succeed because the case doesn't match.

This obviously needs to be reported to Apple (which I will do after this), and I was able to work around the issue on my current project by just using lowercase class names in place of the ones that weren't working with toggleClass(), but we could avoid this issue in Bonzo by simply ignoring case when matching classes, since class names are effectively not case-sensitive. While case is usually preserved, as far as CSS is concerned a selector should (and as far as I know, always does) match, regardless of case.

symmetriq commented 11 years ago

Alrighty, here's the reduced test case I sent in my bug report (ID: 13446153 @ bugreport.apple.com)

(1) Create a new HTML document with the following contents:

<html>
<body>
  <div id="div1"></div>
  <div id="div2"></div>
  <div id="div3" class="fooBar"></div>
</body>
</html>

(2) Open the web inspector and type the following in the console:

// get the elements
var a = document.getElementById('div1')
  , b = document.getElementById('div2')
  , c = document.getElementById('div3');

// add the class `fooBar` to div1 via .className.add()
a.classList.add('fooBar');

// add the same class to div2 by setting className directly
b.className = 'fooBar';

// display the first class name of each element
// notice in div1, `foobar` is all lowercase; in div2 and div3, capitalization is preserved
console.log(a.id, 'className:', a.className, 'classList[0]:', a.classList[0]);
console.log(b.id, 'className:', b.className, 'classList[0]:', b.classList[0]);
console.log(c.id, 'className:', c.className, 'classList[0]:', c.classList[0]);
symmetriq commented 11 years ago

Interestingly, if className has any value before the first time classList.add() is called (either via the class attribute in the HTML itself, or by setting the className property), the classList.add() method will correctly preserve capitalization. This might be a better workaround than ignoring case in hasClass() (since that's just calling .classList.contains()).

HTML document contents:

<html>
<body>
  <div id="div1"></div>
  <div id="div2"></div>
  <div id="div3"></div>
  <div id="div4" class="fooBar"></div>
</body>
</html>

Paste in JS console:

// get the elements
var a = document.getElementById('div1')
  , b = document.getElementById('div2')
  , c = document.getElementById('div3')
  , d = document.getElementById('div4');

// add the class `fooBar` to div1 via .className.add()
a.classList.add('fooBar');

// add the same class to div2 by setting className directly
b.className = 'fooBar';

// set className to a blank string, then add the class via .classList.add()
c.className = '';
c.classList.add('fooBar');

// display the first class name of each element
// notice in div1, `foobar` is all lowercase; in all others, capitalization is preserved
console.log(a.id, 'className:', a.className, 'classList[0]:', a.classList[0]);
console.log(b.id, 'className:', b.className, 'classList[0]:', b.classList[0]);
console.log(c.id, 'className:', c.className, 'classList[0]:', c.classList[0]);
console.log(d.id, 'className:', d.className, 'classList[0]:', d.classList[0]);
ded commented 11 years ago

jesus! this is quite the findings. i'll have some closer reading. first things first is to add a test-case into the tests file and then begin running them in safari

ded commented 11 years ago

@symmetriq

rvagg commented 11 years ago

Might be a good case for some feature-detection at startup. make a new element, add a classname with capitals in it, check if it returns lowercased version, if so, swap out default methods for one that does a toLowerCase or a regex/i. It'd be a shame to have to do a toLowerCase or a regex on all browsers if this is only for one buggy browser, would it not?

symmetriq commented 11 years ago

This isn't a proper test case for Bonzo (not really Bonzo's fault), but you can load it up in any browser and see what happens. I'm just using plain old JavaScript (without any modules) to clarify that the bug is in the DOM API, and that it occurs when adding a class via .classList.add(), but only when both of the following are true: a) the element has no class attribute in the HTML itself, b) you haven't directly set a value for className first (any value will do).

As for Bonzo, you could work around it by setting an element's className to '' (empty string) if className is currently empty (after which .classList.add() should always work properly). You only need to do that once, but I can't think of a way to keep track of whether it's been done on a per-element basis that isn't likely to be slower than just doing it every time (which means checking if className is empty every time…ugh).

Anyway here's the thing that'll tell you if any given browser's .classList.add() is barsted.

<html>
<head>
  <style>
    body {
      font-family: Consolas, Courier, fixed-width;
      font-size: 12pt;
    }
    .element { color: blue }
    .good { color: green }
    .bad { color: red }
  </style>
</head>
<body>
  <p>Result:</p>
  <div id="output"></div>
  <p>View source for details.</p>
  <div id="div1"></div>
  <div id="div2"></div>
  <div id="div3"></div>
  <div id="div4" class="fooBar"></div>
  <script>
  // get the elements
  var a = document.getElementById('div1')
    , b = document.getElementById('div2')
    , c = document.getElementById('div3')
    , d = document.getElementById('div4')
    , o = document.getElementById('output')
    , output = [];

  // add the class `fooBar` to div1 via .className.add()
  a.classList.add('fooBar');

  // add the same class to div2 by setting className directly
  b.className = 'fooBar';

  // set className to a blank string, then add the class via .classList.add()
  c.className = '';
  c.classList.add('fooBar');

  // display the first class name of each element
  // notice in div1, `foobar` is all lowercase; in all others, capitalization is preserved

  [a,b,c,d].forEach(function (el) {
    console.log(el.id, 'className:', el.className, 'classList[0]:', el.classList[0]);
    output.push([
        '<span class="element">' + el.id + '</span>'
      , 'className:'
      , '<span class="' + (el.className === 'fooBar' ? 'good' : 'bad') + '">' + el.className + '</span>'
      , 'classList[0]:'
      , '<span class="' + (el.classList[0] === 'fooBar' ? 'good' : 'bad') + '">' + el.classList[0] + '</span>'
    ].join(' '));
  });

  o.innerHTML = output.join('<br>');
  </script>
</body>
</html>
symmetriq commented 11 years ago

@rvagg Yeah, definitely don't want to change how Bonzo works for every browser. I don't even like the idea of doing any kind of workaround at all; I worry about JS libraries slowly acquiring a mess of hacks to deal with browser bugs that only existed for a few versions, then being stuck with the hacks because someone might still be running one of those versions. On the other hand I guess some of the appeal of modules is to fix these things once and (hopefully) not have to worry about them again.

If you guys do want to try to handle this, I like the idea of a quick startup test; make a new element, do a .classList.add('aBc'), and see if it preserves the case.

Sadly, I think the hack I mentioned earlier (setting className to '' if it's currently empty) is probably not viable as a workaround. Problem is: as soon as .classList.add() has been called on any given element, that element is "tainted" and won't ever work properly. I have no idea what's going on under the hood, but if you set className first, the element behaves correctly until it's destroyed, and if you call .classList.add() first, it's broken until destroyed. I don't know if this proves anything, but I tried destroying a tainted element, then creating a new one with the same ID, and confirmed that it doesn't acquire the broken state (it can be primed with the empty string, and then it'll work properly).

Anyway, that's everything I've figured out so far.

symmetriq commented 11 years ago

Just noticed this bug is fixed in the current WebKit nightly (r152225). I kinda forgot about this until just now, so they might have fixed it awhile ago.