Closed Bug 814014 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: julienw, Assigned: julienw)

References

()

Details

(Keywords: doc-bug-filed)

Attachments

(1 file, 5 obsolete files)

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).
Just tested in current Chrome unstable (v25) and this seems implemented there.
OS: Linux → All
Hardware: x86_64 → All
FYI: toggle() was changed since you implemented it. The second argument was recently added.
Anne> Opened Bug 814090 to track this.
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.
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 ?
I filed bug 815502.
Depends on: 815502
Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=20104 on what looks like a bug in the add() spec.
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.
Boris, now that the spec issues have been RESOLVED FIXED, can we go forward here? Does your patch follow the spec resolutions?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
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.
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.
Or should we consider that in the most usual case we won't have so many tokens that we need a real set implementation ?
> 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 17 more or less sums up my feelings, fwiw.
It's not clear to me if adding 2 (new) classes should trigger 2 mutation events, or just 1 ?
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.
Ok, thanks a lot !
Attached patch wip patch v1 (obsolete) — Splinter Review
Here is a WIP patch. Not all tests are passing yet, but I wanted to give an update.
Assignee: nobody → felash
Attachment #685722 - Attachment is obsolete: true
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()
Attached patch wip patch v2 (obsolete) — Splinter Review
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
Attachment #792708 - Attachment is obsolete: true
> 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.
Depends on: 814090
Depends on: 885279
Attached patch wip patch v3 (obsolete) — Splinter Review
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
Attachment #793408 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — Splinter Review
previous try failed because of missing parentheses, here is my new try: https://tbpl.mozilla.org/?tree=Try&rev=02a5071ee8fb
Attachment #795881 - Attachment is obsolete: true
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.
Attachment #795943 - Attachment description: wip patch v4 → patch v1
Attachment #795943 - Flags: review?(bugs)
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
Attachment #795943 - Flags: review?(bugs) → review-
Blocks: 910121
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.
Attached patch patch v2Splinter Review
Fixed all comments except the whitespace handling comment (bug 910121).

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

Thanks !
Attachment #795943 - Attachment is obsolete: true
Attachment #796491 - Flags: review?(bugs)
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.
Attachment #796491 - Flags: review?(bugs) → review+
yep, will work on this asap
Keywords: checkin-needed
(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 !
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.
Flags: needinfo?(felash)
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.
Ah, jsperf link is just fine.  Thanks!
Flags: needinfo?(felash)
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.
Keywords: checkin-needed
(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 ;)
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();
  }
right, I'm dumb.

Filed bug 910795 about the variadic arguments slowdown.
https://hg.mozilla.org/mozilla-central/rev/73c39de65fe1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
> 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.
My plan is to update the documentation of DOMTokenList before the release of Firefox 26.
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.
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.
Okay, thanks for the explanation.
@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.
You need to log in before you can comment on or make changes to this bug.