814014 - implement the new classList specification which permits adding/removing several classes with one call

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julien Wajsberg [:julienw]]

Bug 501257 was about implementing HTMLElement.classList.

The specification seems to have changed recently, and now you can have several arguments in "add" and "remove", which lets you add and remove several classes in one call.

see http://dom.spec.whatwg.org/#dfnReturnLink-1

"toggle" wasn't changed (as it could already take a second argument).

Comment 1

[Julien Wajsberg [:julienw]]

Just tested in current Chrome unstable (v25) and this seems implemented there.

Comment 2

[Anne (:annevk)]

FYI: toggle() was changed since you implemented it. The second argument was recently added.

Comment 3

[Julien Wajsberg [:julienw]]

Anne> Opened Bug 814090 to track this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Julien, this depends on adding support for variadic arguments to our WebIDL bindings.  Not sure whether there's a bug on that already....

I have a half baked patch for it lying around somewhere (where "half-baked" == "does not compile") that I could resurrect if this is a priority.

Comment 5

[Julien Wajsberg [:julienw]]

Boris, I don't think this is a priority :-) I wanted to use this in a B2G patch so I tracked it.

Couldn't find a bug about variadic arguments, and I'm not sure I could write the correct wording.. Could you please open it ?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I filed bug 815502.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=20104 on what looks like a bug in the add() spec.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And https://www.w3.org/Bugs/Public/show_bug.cgi?id=20105 on what looks like remove() being underdefined enough to not be implementable sanely.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Initial hack to test variadic support, pending the spec issues being resolved

Comment 12

[Mounir Lamouri (:mounir)]

Boris, now that the spec issues have been RESOLVED FIXED, can we go forward here? Does your patch follow the spec resolutions?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think we can go forward, yes.

The patch does not follow the spec resolutions at all, no.  In fact, I suspect that to do what the spec says we should significantly modify the DOMTokenList implementation; it's very much focused around working with one token at a time but should be recast in terms of token sets.

Comment 14

[Julien Wajsberg [:julienw]]

I'd be happy to contribute here but I'd need some directions on how to rework the implementation around token sets.

If I understand correctly, you'd want to move from concatenating strings and searching arrays to using sets to ensure the class unicity, and join this set to strings.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I haven't thought that much how to do that part, actually...  We may not need an actual (ordered) set data structure, possibly, though using something like that would be simplest in some ways.

Comment 16

[Julien Wajsberg [:julienw]]

Do we really need an ordered set ? Is it so that the order used by the Web author stays the same ?

Also, I don't exactly know the objects that already exist in gecko, do you know any existing structure that could be reused here ?

I've seen that |nsAttrValue| already computes a HashValue(), is it used by other structures ?

I'm actually not sure that I have the C++ skills to do all this, but I'm willing to try and learn.

Comment 17

[Julien Wajsberg [:julienw]]

Or should we consider that in the most usual case we won't have so many tokens that we need a real set implementation ?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Do we really need an ordered set ?

The spec talks in terms of one, no?

> Is it so that the order used by the Web author stays the same ?

It's so that if your className is "a" and you add("b") you don't end up with "b a".

> Also, I don't exactly know the objects that already exist in gecko, do you know any
> existing structure that could be reused here ?

The only obvious ordered data structure in gecko is arrays...

> I've seen that |nsAttrValue| already computes a HashValue(), is it used by other
> structures ?

I'd have to dig through the code to look it up, but it's not relevant to us here, right?  There's only one nsAttrValue involved, and it holds the whole class list.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment 17 more or less sums up my feelings, fwiw.

Comment 20

[Julien Wajsberg [:julienw]]

It's not clear to me if adding 2 (new) classes should trigger 2 mutation events, or just 1 ?

Comment 21

[Anne (:annevk)]

If you mean .add("one", "two") there should be one mutation record.

http://dom.spec.whatwg.org/#dom-domtokenlist-add

a. First you add the classes
b. Then you run the "update steps"
c. Which run http://dom.spec.whatwg.org/#concept-element-attributes-set which triggers a single attribute mutation.

Comment 22

[Julien Wajsberg [:julienw]]

Ok, thanks a lot !

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: wip patch v1

Here is a WIP patch. Not all tests are passing yet, but I wanted to give an update.

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 792708 [details] [diff] [review]
wip patch v1

Review of attachment 792708 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sensible already; a few nits below.

::: content/base/src/nsDOMTokenList.cpp
@@ +99,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsDOMTokenList::CheckTokens(const nsTArray<nsString>& aTokens) {

{ on the next line

@@ +100,5 @@
>  }
>  
> +nsresult
> +nsDOMTokenList::CheckTokens(const nsTArray<nsString>& aTokens) {
> +  PRInt32 i = 0, l = aTokens.Length();

uint32_t

@@ +101,5 @@
>  
> +nsresult
> +nsDOMTokenList::CheckTokens(const nsTArray<nsString>& aTokens) {
> +  PRInt32 i = 0, l = aTokens.Length();
> +  nsresult aError;

No a prefix for local variables; use rv.

@@ +103,5 @@
> +nsDOMTokenList::CheckTokens(const nsTArray<nsString>& aTokens) {
> +  PRInt32 i = 0, l = aTokens.Length();
> +  nsresult aError;
> +
> +  while (i < l) {

A for loop would be better, IMO.

@@ +145,5 @@
> +
> +  PRInt32 i = 0, l = aTokens.Length();
> +
> +  while (i < l) {
> +    aToken = &aTokens[i++];

|const nsString& token = aTokens[i];|, I think.

@@ +154,5 @@
> +
> +    if (oneWasAdded ||
> +        (!resultStr.IsEmpty() &&
> +        !nsContentUtils::IsHTMLWhitespace(
> +            resultStr.CharAt(resultStr.Length() - 1)))) {

.Last()

Comment 25

[Julien Wajsberg [:julienw]]

Attachment: wip patch v2

still not finished.

I adressed the previous comments (thanks a lot !) but I also added more tests and I see that I can add several times the same class, which is wrong of course.

Also, I tried to do perf runs using jsperf.com ([1]) to compare with a nightly (using an optimized build done by myself) and it looks like it's a lot slower. But maybe I'm not building correctly. How should we compare perf changes for patches ?

Especially, on nightly, using "add" is slightly faster than using "toggle" with true as second argument. With this patch, "add" is about 50% slower than toggle. Maybe variadic arguments are slow ?

Also, comparing runs on nightly and runs on build with this patch, even toggle-with-true looks notably slower (maybe because of the temporary array ?), but maybe different perf runs are not directly comparable with jsperf.com, and maybe my build has not the same build options as official builds.

[1] http://jsperf.com/classlist-tests

Comment 27

[Boris Zbarsky [:bzbarsky]]

> How should we compare perf changes for patches ?

Do a build with/without the patch and see?  And preferably not using jsperf, since it introduces all sorts of weirdness into the numbers.  :(

Variadic arguments would definitely be somewhat slower than non-variadics in a microbenchmark because they have to allocate the array, and doubly so for strings: a non-variadic string can share the JS buffer but a variadic ends up copying.  

If we care, we could make add() with one string faster and only fall back to the variadic for 2+ strings... but I'm not sure we care.

Comment 28

[Julien Wajsberg [:julienw]]

Attachment: wip patch v3

Changes with the previous patch: I add added tokens to an array, and check in this array if we already added the current token.

If try is ok, this is the first reviewable patch. Please tell me if I missed some obvious test cases too :)

try is https://tbpl.mozilla.org/?tree=Try&rev=beb498e51825

Comment 29

[Julien Wajsberg [:julienw]]

Attachment: patch v1

previous try failed because of missing parentheses, here is my new try: https://tbpl.mozilla.org/?tree=Try&rev=02a5071ee8fb

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 795943 [details] [diff] [review]
patch v1

asking review from :smaug which seems to have written most of this initially.

Some orange in the try, I've not enough experience to know if this is important, but it seems it's not.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795943 [details] [diff] [review]
patch v1

(The spec just got few fixes)

>+nsDOMTokenList::CheckTokens(const nsTArray<nsString>& aTokens)
>+{
>+  nsresult rv;
>+
>+  for (uint32_t i = 0, l = aTokens.Length(); i < l; i++) {
>+    rv = CheckToken(aTokens[i]);
>+    if (NS_FAILED(rv)) {
>+      return rv;
>+    }
Could you move nsresult rv inside the for
nsresult rv = CheckToken...

>-  if (!resultStr.IsEmpty() &&
>-      !nsContentUtils::IsHTMLWhitespace(
>-          resultStr.CharAt(resultStr.Length() - 1))) {
>-    resultStr.Append(NS_LITERAL_STRING(" ") + aToken);
>-  } else {
>-    resultStr.Append(aToken);
>+  bool oneWasAdded = false;
>+  nsTArray<nsString> addedClasses(aTokens.Length());
nsAutoTArray perhaps

>+
>+  for (uint32_t i = 0, l = aTokens.Length(); i < l; i++) {
++i

>+    const nsString& aToken = aTokens[i];
>+
>+    if ((aAttr && aAttr->Contains(aToken)) ||
>+        addedClasses.Contains(aToken)) {
>+      continue;
>+    }
>+
>+    if (oneWasAdded ||
>+        (!resultStr.IsEmpty() &&
>+        !nsContentUtils::IsHTMLWhitespace(resultStr.Last()))) {
>+      resultStr.Append(NS_LITERAL_STRING(" ") + aToken);
>+    } else {
>+      resultStr.Append(aToken);
>+    }
>+
>+    oneWasAdded = true;
>+    addedClasses.AppendElement(aToken);
>   }
>+
>   mElement->SetAttr(kNameSpaceID_None, mAttrAtom, resultStr, true);
> }
So if I read this correctly, this doesn't actually do what the current spec says.
element.setAttribute("class", "  foo"); // note extra spaces
element.classList.add("bar");
element.getAttribute("class") == "foo bar"; // per spec, but this patch would lead to "  foo bar"

>+nsDOMTokenList::Add(const nsAString& aToken, mozilla::ErrorResult& aError) {
{ goes to the next line

>+nsDOMTokenList::Remove(const nsAString& aToken, mozilla::ErrorResult& aError) {
{ goes to the next line

Comment 32

[Julien Wajsberg [:julienw]]

I filed Bug 910121 for the whitespace handling changes, I'd like to ask Anne if we read the spec correctly, and this seems really unrelated to this bug after all.

Addressed all the other comments.

Comment 33

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Fixed all comments except the whitespace handling comment (bug 910121).

New try is https://tbpl.mozilla.org/?tree=Try&rev=de1cc74cac5b

Thanks !

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 796491 [details] [diff] [review]
patch v2

Looks like the old Add()/Remove() which take just one string are still needed.

But please look at Bug 910121 too. I think we should have that fixed at the
same time as this one.

Comment 35

[Julien Wajsberg [:julienw]]

yep, will work on this asap

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 796491 [details] [diff] [review]
> patch v2
> 
> Looks like the old Add()/Remove() which take just one string are still
> needed.

Yes it is, it's used at least in the video element handling.

Thanks for the review !

Comment 37

[Boris Zbarsky [:bzbarsky]]

Julien, if you have a performance testcase for this, I'd really like to see it just to check whether there's something silly the bindings are doing with the rest parameter.

Comment 38

[Julien Wajsberg [:julienw]]

Boris, I don't have one right now besides http://jsperf.com/classlist-tests but I can do one on a plain html page if you wait a few hours.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Ah, jsperf link is just fine.  Thanks!

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73c39de65fe1

Comment 41

[Boris Zbarsky [:bzbarsky]]

OK, so performance.

The test is testing an operation that is a no-op.  Which means it can be very very fast, in theory.  Before these changes it's a pretty quick no-op.  With these changes, it's a more expensive no-op.  On my hardware, it's about 66ns/call before the change and about 435ns/call after the change for add().

The performance hits come from the following:

1)  Using a variadic means we have to copy the string, since the type is nsTArray<nsString>.  This involves an allocation of the copied data and then a deallocation.  In this testcase those make up, between the two of them, 42% of the post-change time (so about 180ns: 3x what the entire call used to take).  For the specific case of variadic string arguments, we _could_ try using an nsTArray<FakeDependentString> or some such.  It would be annoying, but...  Alternately we could try something where we use nsTArray<nsString> but rebind it as a dependent string.  Please file a followup bug on this?

2)  The old add() code had a fast path to make itself a fast no-op:

  if (attr && attr->Contains(aToken)) {
    return;
  }

but the new add() code always calls AddInternal() which always converts the nsAttrValue to a string and worse yet always calls SetAttr (which then optimizes away the same-value set, but it takes it a bit of work to do that).  So it's doing way more work in the no-op case.  On the face of it, that's what the spec calls for (and via mutation observers the difference is detectable); it's not clear to me whether the spec should in fact call for that.  In either case, it seems we could do better in the no-op case than parsing and then reserializing...

There are some other minor inefficiencies (e.g. the SetCapacity() call on addedClasses being a perf loss in the common case), but those are the major issues.

Comment 42

[Julien Wajsberg [:julienw]]

(In reply to Boris Zbarsky [:bz] from comment #41)
> OK, so performance.
> 
> The test is testing an operation that is a no-op. 

Mmm right, I wanted to reset the className in the teardown but actually never added it.

> Which means it can be
> very very fast, in theory.  Before these changes it's a pretty quick no-op. 
> With these changes, it's a more expensive no-op.  On my hardware, it's about
> 66ns/call before the change and about 435ns/call after the change for add().
> 
> The performance hits come from the following:
> 
> 1)  Using a variadic means we have to copy the string, since the type is
> nsTArray<nsString>.  This involves an allocation of the copied data and then
> a deallocation.  In this testcase those make up, between the two of them,
> 42% of the post-change time (so about 180ns: 3x what the entire call used to
> take).  For the specific case of variadic string arguments, we _could_ try
> using an nsTArray<FakeDependentString> or some such.  It would be annoying,
> but...  Alternately we could try something where we use nsTArray<nsString>
> but rebind it as a dependent string.  Please file a followup bug on this?

Will do.


> 2)  The old add() code had a fast path to make itself a fast no-op:
> 
>   if (attr && attr->Contains(aToken)) {
>     return;
>   }
> 
> but the new add() code always calls AddInternal() which always converts the
> nsAttrValue to a string and worse yet always calls SetAttr (which then
> optimizes away the same-value set, but it takes it a bit of work to do
> that).  So it's doing way more work in the no-op case.  On the face of it,
> that's what the spec calls for (and via mutation observers the difference is
> detectable); it's not clear to me whether the spec should in fact call for
> that.  In either case, it seems we could do better in the no-op case than
> parsing and then reserializing...


The current spec seems to want that we work with arrays (as you suggested in comment 13) and I might just do this in bug 910121. But this is not that easy for me ;)

Thanks for the analysis. I added a teardown in http://jsperf.com/classlist-tests/3 so that it's hopefully not a no-op anymore ;)

Comment 43

[Boris Zbarsky [:bzbarsky]]

It's still a no-op.  Teardown runs after each batch of iterations, not between them.  So the structure of a jsperf test is:

  function foo(count) {
    setup();
    while(count--) {
      your_test_here();
    }
    teardown();
  }

Comment 44

[Julien Wajsberg [:julienw]]

right, I'm dumb.

Filed bug 910795 about the variadic arguments slowdown.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73c39de65fe1

Comment 47

[jonobr1]

When will this be in stable of Firefox? While the bug might be fixed, the documentation is misleading because current stable doesn't adhere to multiple arguments.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> When will this be in stable of Firefox?

See "Target Milestone" field (Firefox 26) and <https://wiki.mozilla.org/RapidRelease/Calendar>.  So mid-December.

> the documentation is misleading because current stable doesn't adhere to multiple
> arguments.

The documentation at https://developer.mozilla.org/en-US/docs/Web/API/element.classList clearly says:

  Currently, Firefox does not implement the use of several arguments in the
  add/remove/toggle methods

The documentation at https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList doesn't mention multiple arguments at all.

Comment 49

[Jean-Yves Perrier [:teoli]]

My plan is to update the documentation of DOMTokenList before the release of Firefox 26.

Comment 50

[jonobr1]

Ahh, thanks for the clarification. I now see there is a footnote. This is rather minor, but I would still argue its misleading. I think its clearer to place the multiple arguments *as a footnote* and remove it completely from copy and paste-able code examples. Perhaps even the 4 duplicate bugs reinforce this misconception.

Comment 51

[Jean-Yves Perrier [:teoli]]

Nope, we are documenting the open web in that section, not Mozilla implementation of it. So Mozilla bugs and limitation shouldn't be the base text, they should be the footnote.

And in one year and a half from now, nobody will care any more that prior Fx 26 we supported only one argument.

Comment 52

[jonobr1]

Okay, thanks for the explanation.

Comment 54

[David Bruant]

@jonobr1 never forget MDN is a wiki, feel free to edit it if you feel something is unclear.

(In reply to Jean-Yves Perrier [:teoli] from comment #51)
> And in one year and a half from now, nobody will care any more that prior Fx
> 26 we supported only one argument.
FirefoxOS changes that. It still isn't clear to me how often FirefoxOS phone see their version of Gecko updated.
There is no rush now, but at some point, Mozilla will need to keep track of FirefoxOS version market shares. And depending on the behavior, we'll have to ask ourselves at some point what that means from a documentation perspective.