search

jwilsson / domtokenlist

A super duper strict shim/polyfill for DOMTokenList, classList and relList.
MIT License
12 stars 16 forks source link

Unreliable behaviour when passed by reference #8

Open Alhadis opened 9 years ago

Alhadis commented 9 years ago

Hi gents,

Consider the following:

<!DOCTYPE html>
<html class="sample nonsense">
var root = document.documentElement;
var rootClasses = root.classList;

console.log(rootClasses[0]);
/** Outputs "sample" */

/** Modify the className attribute directly */
root.className = "just more " + root.className;

console.log(rootClasses[0]);
/**
    Expected result: "just"
    Actual result: "sample"
*/

The catalyst of the issue is obvious: because your polyfill's creating a new DOMTokenList instance every time a property's accessed (which can't possibly be good for the browser's memory, BTW) you're forcing developers to avoid modifying class attributes through className (which might be desirable when resetting added classes, and an element's classList property's been aliased outside the scope for quicker look-up).

I'll admit this is a bit of an edge case, and probably not one you're willing to fix if you're to maintain your existing filesize, but there it is.

For the record, I found this issue when comparing your polyfill with mine, as I was interested in seeing how you managed to get yours slightly smaller. Found the reason, and you've just finished reading through it...

TrySound commented 9 years ago

@Alhadis Sorry, didn't understand. Do you have the dicision? And yes, you should avoid using className for reset, because it's unsafe, DOM is global thing. Every part of script can have access to it, and your reset can break some of them.

Alhadis commented 9 years ago

@TrySound Uhm, that makes no sense. className is already part of the DOM. Using it isn't "unsafe", it's simply the earlier, pre-established method of manipulating an element's "class" attribute (as in, the one dating back way before classList was introduced).

Furthermore, this is less to do with good practices and more to do with general compatibility: a good polyfill shouldn't require an author to alter their workflow.

TrySound commented 9 years ago

@Alhadis

avoid using className for RESET

You can use it with .split(' '), but why if you already have API.

TrySound commented 9 years ago

@Alhadis Anyway, native classList works fast, so initializing on every call isn't very slow. For old browsers it's not so matter.

Alhadis commented 9 years ago

Erh no, that's not quite what I meant. Assume a developer's added several classes to an element and wants to ensure they're all removed, without necessarily knowing what they are.

Of course, you're free to pass this off as unimportant... take it or leave it, as I said. :p

TrySound commented 9 years ago

@Alhadis Do you have any suggestions to fix this?

Alhadis commented 9 years ago

Well first of all, you'll need to create a DOMTokenList reference per property only once, and have that redefine indexable properties when any of its external methods are called. You can see here how I did it, by storing a reference to the last value of the targeted attribute, and updating the range of defined indexed properties.

It's not something that's an easy fix, I guess. XD

Alhadis commented 9 years ago

So in other words, rather than assigning tokens to indexes of the token-list directly, you use Object.defineProperty to define new getters/setters for indexes whenever a change is detected. E.g., instead of doing this:

inst[i] = values[i];

You might do something like this instead (I'm not taking scope into account, this is just to give you a rough approximation of what I'm talking about):

Object.defineProperty(inst, i, {
    get: function(){ return values[i]; },
    set: function(i){ values[i] = i; update(); }
});

Feel free to refer to my polyfill for elaboration on how I did it, if you prefer. :)

jwilsson commented 9 years ago

Hi @Alhadis, Thanks for the report!

I agree with you that it's a bit of a edge case and I think we're going to leave it as-is since we'd like to keep the current file size.

However, if you want to work on it and submit a PR, be my guest! (I'm guessing you don't since you've got your own polyfill working :smile:)

Thank you again for a very well researched report!

Alhadis commented 9 years ago

Nah it's fine, sorry. Got more than enough on my plate, and I'd like to sharpen and improve my own polyfill instead. Just thought I'd bring this to your attention in case it was of any value to you. :)