Bug 20104 - Algorithm for DOMTokenList.add seems wrong if tokens are present multiple times in the list
Summary: Algorithm for DOMTokenList.add seems wrong if tokens are present multiple tim...
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: PC Windows 3.1
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-27 18:35 UTC by Boris Zbarsky
Modified: 2012-12-04 15:55 UTC (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2012-11-27 18:35:31 UTC
Consider this script:

  var d = document.createElement("div");
  d.classList.add("a", "a");

If I read the spec right, the result of this will serialize as:

  <div class="a a"></div>

which seems slightly odd.
Comment 1 Anne 2012-11-27 18:43:11 UTC
Do we want to catch this? Is the extra validation step negligible?
Comment 2 Boris Zbarsky 2012-11-27 19:29:18 UTC
I don't know.  None of this stuff is exactly super-fast, as currently specced.  How UAs optimize it is an interesting question; Gecko doesn't try very hard at the moment afaict, though I might change that.

One related question is whether we want:

  d.classList.add.apply(d.classList, args);

to have different behavior from:

  args.map(function(x) { d.classList.add(x); });

and in general what invariants we want this token list stuff to preserve.
Comment 3 Jonas Sicking 2012-11-28 04:15:26 UTC
Optimizing the existing stuff for performance seems better to me than slowing things down even more. I don't have any concrete suggestions though.
Comment 4 Anne 2012-11-28 11:40:51 UTC
Would there be a problem with changing the model of DOMTokenList to an ordered set instead?

The moment you start modifying class="" through DOMTokenList, class="" would simply become a simple serialization of DOMTokenList rather than preserving all the things it tries to do now.
Comment 5 Erik Arvidsson 2012-11-28 18:44:53 UTC
The whole idea of trying to preserve the whitespaces of the class attribute when we mutate the DOMTokenList is something that has been bothered all along.

Let's remove duplicates and simply serialize by adding a space between each token.

This is what JS libs did for years and polyfills still do so it should be safe.
Comment 6 Anne 2012-11-28 19:35:03 UTC
The following is my plan:

DOMTokenList represents an ordered set (with set guaranteeing uniqueness, of course).

Manipulating its associated attribute (e.g. class="") seeds DOMTokenList.

Manipulating DOMTokenList seeds its associated attribute (a simple serialization of the ordered set, tokens separated by U+0020).

Somehow we avoid loops. DOMSettableTokenList does the same.
Comment 7 Olli Pettay 2012-11-29 15:15:58 UTC
To have sane toggle handling we need to remove duplicates.
Comment 8 Anne 2012-12-04 15:55:21 UTC
DOMTokenList is now defined as representing an ordered set. See bug 20105 for how that serializes to a string and parses from a string.