501257 - Implement HTML 5's HTMLElement.classList property

Status: RESOLVED FIXED

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

Description

[Sylvain Pasche]

This should make class manipulation easier and faster.

Comment 1

[Sylvain Pasche]

Attachment: wip

work in progress. Some things that are left to do:
- scriptable helpers for [IndexGetter] and [Stringifies]
- Handling of case insensitivity (after clarification from spec)
- How to deal with empty tokens (waiting for spec clarification)
- Cycle collection

Comment 2

[Sylvain Pasche]

Attachment: wip, v2

- toString() implementation.
 - indexing operator.

Getting an item from an out of bounds index returns an empty string. I guess it should return undefined instead, I'm not sure how to address that yet. (the same thing happens with nsIDOMDOMStringList attributes, for instance document.styleSheetSets[1000] === "" -> true).

Comment 3

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

(In reply to comment #2)

> Getting an item from an out of bounds index returns an empty string. I guess it
> should return undefined instead,
Why undefined, why not null?
HTML5 says "Returns null if index is out of range."
To null a string use SetDOMStringToNull.

Comment 4

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

Oh, you even seem to do something like that.

Btw, when you add support for cycle collector, please make sure to change
the nsGenericElement* to nsRefPtr<nsGenericElement>

Comment 5

[Sylvain Pasche]

(In reply to comment #3)
> Why undefined, why not null?
> HTML5 says "Returns null if index is out of range."

Yeah, that should be the right thing to do. By the way, I noticed that other array-like DOM properties don't behave that way:

The DOM 3 Core spec mentions to return null on out of bounds indexing, but we get:
document.body.childNodes[1000] === undefined
true
document.body.childNodes.item(1000) === null
true

WebKit and Opera do the same. IE throws when using [].

The same happens with document.styleSheets apparently (on Firefox at least).

(In reply to comment #4)
> Btw, when you add support for cycle collector, please make sure to change
> the nsGenericElement* to nsRefPtr<nsGenericElement>

ok, will do. Thanks for the feedback.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to comment #5)
> (In reply to comment #3)
> > Why undefined, why not null?
> > HTML5 says "Returns null if index is out of range."
> 
> Yeah, that should be the right thing to do. By the way, I noticed that other
> array-like DOM properties don't behave that way:
> 
> The DOM 3 Core spec mentions to return null on out of bounds indexing, but we
> get:
> document.body.childNodes[1000] === undefined
> true
> document.body.childNodes.item(1000) === null
> true

That behaviour (undefined when fetching the "1000" property and null when invoking the operation directly with .item(1000)) is what Web IDL requires.  Corresponding index properties (such as one named "1000") don't exist on the object until that index is a valid one.  So in this example, the "1000" property doesn't exist, and the 'Host object [[Get]] method' doesn't invoke the index getter operation item() at all.  See:

  http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties
  http://dev.w3.org/2006/webapi/WebIDL/#get

HTML 5 defines that DOMTokenList::item() returns null when passed an invalid index, so that is what is returned when calling it explicitly.

Comment 7

[Sylvain Pasche]

Thanks for clarifying, that makes sense now.

Comment 8

[Sylvain Pasche]

(In reply to comment #4)
> Btw, when you add support for cycle collector, please make sure to change
> the nsGenericElement* to nsRefPtr<nsGenericElement>

Actually, cycle collector support may not be needed if we follow the same approach as what is done for element.style (implemented as a tear off in nsDOMCSSAttributeDeclaration): it keeps a non owning reference to the nsIContent, which is cleared when needed in DropReference().

Using a nsRefPtr would create a cycle between nsDOMTokenList and nsGenericElement (through its slot). I guess the cycle collector would be able to detect and free that, but that would be more costly. Or is there a good reason to go that way?

Comment 9

[Sylvain Pasche]

Attachment: wip, v3

- Added DropReference() to clear the reference when the underlying element is gone.
 - Added checks to return early when mElement is null.
 - Added tests for case sensitivity. The current spec is case sensitive, and it seems that it's going to stay that way (after some discussions with Hixie).

This version shouldn't leak nor crash.

Comment 10

[Fabian (Crash) Grutschus]

Attachment: Testcase for classList.add, classList.remove, classList.contains, classList.toogle

Comment 11

[Sylvain Pasche]

Thanks for sharing your testcase. I tested it with the current patch, and it passes after a typo fix: s/toogle/toggle/ (and by the way s/sensetive/sensitive/).

For your information, you can also find a testcase in the patch (look for the test_classList.html file), which uses the Mozilla testing framework. There are also a few tests on http://simon.html5.org/test/html/dom/reflecting/DOMTokenList/. Note that the spec changed recently and some of these tests need to be updated.

Comment 12

[Sylvain Pasche]

Attachment: v4

- do not remove duplicate in .length/item()
 - throw SYNTAX_ERR with empty strings
 - misc other stuff

There's still one feature that doesn't match the spec when getting a nonexistent indexed property: it returns an empty string instead of undefined.

Comment 13

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

(In reply to comment #12)
> There's still one feature that doesn't match the spec when getting a
> nonexistent indexed property: it returns an empty string instead of undefined.
Well, there is no spec, there is just a draft ;)

Comment 14

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

Comment on attachment 392290 [details] [diff] [review]
v4

>+nsDOMTokenList::Item(PRUint32 aIndex, nsAString& aResult)
...
>+  if (!attr) {
>+    SetDOMStringToNull(aResult);
>+    return NS_OK;
>+  }
>+
>+  if (aIndex >= static_cast<PRUint32>(attr->GetAtomCount())) {
>+    SetDOMStringToNull(aResult);
>+    return NS_OK;
>+  }
Maybe
  if (!attr || aIndex >= static_cast<PRUint32>(attr->GetAtomCount())) {
    SetDOMStringToNull(aResult);
    return NS_OK;
  }

>+nsDOMTokenList::CheckToken(const nsAString& aStr)
>+nsDOMTokenList::ContainsInternal(const nsAttrValue* aAttr,
>+                                 const nsAString& aToken)
...
>+  nsIAtom* atom = NS_NewAtom(aToken);
>+  PRBool result = aAttr->Contains(atom, eCaseMatters);
>+  NS_RELEASE(atom);
No need for the manual NS_RELEASE;
nsCOMPtr<nsIAtom> atom = do_GetAtom(aToken);
return aAttr->Contains(atom, eCaseMatters);

>+nsDOMTokenList::Remove(const nsAString& aToken)
...
>+  nsresult rv = this->CheckToken(aToken);
No need for 'this->' here and elsewhere.
 
>+  const nsAttrValue* GetParsedAttr(nsIAtom* aAttr) const
>+  {
>+    return mAttrsAndChildren.GetAttr(aAttr);
>+  }
>+
There is DoGetClasses(). Is that not enough?
(Perhaps you need to remove NS_NOTREACHED in nsGenericElement)

Please add at least some tests for XHTML, SVG, XUL, MathML and plain XML elements.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 392290 [details] [diff] [review]
v4

Hope it's ok that I'm stealing this review...

General comment:
Node need to use |this->SomeFunction(...)|, just do |SomeFunction(...)| instead.

>+nsDOMTokenList::GetLength(PRUint32 *aLength)
>+{
>+  const nsAttrValue* attr = mElement ?
>+                            mElement->GetParsedAttr(nsGkAtoms::_class) :
>+                            nsnull;

It'd probably be worth adding an inline function that performs this line. Everywhere you call GetPasedAttr you effectively do exactly this.

>+PRBool
>+nsDOMTokenList::ContainsInternal(const nsAttrValue* aAttr,
>+                                 const nsAString& aToken)
>+{
>+  if (!aAttr) {
>+    return PR_FALSE;
>+  }
>+
>+  nsIAtom* atom = NS_NewAtom(aToken);
>+  PRBool result = aAttr->Contains(atom, eCaseMatters);
>+  NS_RELEASE(atom);

Don't use NS_NewAtom. Use do_GetAtom with an nsCOMPtr instead. That way you can get rid of the |result| temporary too.

>+nsDOMTokenList::AddInternal(const nsAttrValue* aAttr,
>+                            const nsAString& aToken)
>+{
>+  nsAutoString resultStr;
>+
>+  if (aAttr) {
>+    aAttr->ToString(resultStr);
>+  }
>+
>+  if (!resultStr.IsEmpty() &&
>+      !nsContentUtils::IsHTMLWhitespace(
>+          resultStr.CharAt(resultStr.Length() - 1)))
>+  {

Put the '{' at the end of the line above to follow convention in the rest of this file.

>+    resultStr.Append(PRUnichar(' '));
>+  }
>+  resultStr.Append(aToken);

It'd be slightly faster to do:

if (...) {
  resultStr.Append(NS_LITERAL_STRING(" ") + aToken)
}
else {
  resultStr.Append(aToken)
}

The reason is that basically every time you call Append we need to resize the string buffer and thus reallocate it. However the + operator creates a temporary stack-based object without any allocations happening. We then append that object in a single operation.

With that fixed, looks great. Though please attach a new patch with the above fixed.

Leaving Smaugs review-request in case you want his review too.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 392290 [details] [diff] [review]
v4

Restoring Ollis r-

Comment 17

[Sylvain Pasche]

Thanks for the reviews.

(In reply to comment #14)
> (From update of attachment 392290 [details] [diff] [review])
> >+nsDOMTokenList::Item(PRUint32 aIndex, nsAString& aResult)
> ...
> >+  if (!attr) {
> >+    SetDOMStringToNull(aResult);
> >+    return NS_OK;
> >+  }
> >+
> >+  if (aIndex >= static_cast<PRUint32>(attr->GetAtomCount())) {
> >+    SetDOMStringToNull(aResult);
> >+    return NS_OK;
> >+  }
> Maybe
>   if (!attr || aIndex >= static_cast<PRUint32>(attr->GetAtomCount())) {
>     SetDOMStringToNull(aResult);
>     return NS_OK;
>   }

done

> >+nsDOMTokenList::CheckToken(const nsAString& aStr)
> >+nsDOMTokenList::ContainsInternal(const nsAttrValue* aAttr,
> >+                                 const nsAString& aToken)
> ...
> >+  nsIAtom* atom = NS_NewAtom(aToken);
> >+  PRBool result = aAttr->Contains(atom, eCaseMatters);
> >+  NS_RELEASE(atom);
> No need for the manual NS_RELEASE;
> nsCOMPtr<nsIAtom> atom = do_GetAtom(aToken);
> return aAttr->Contains(atom, eCaseMatters);
> 
> >+nsDOMTokenList::Remove(const nsAString& aToken)
> ...
> >+  nsresult rv = this->CheckToken(aToken);
> No need for 'this->' here and elsewhere.

done

> >+  const nsAttrValue* GetParsedAttr(nsIAtom* aAttr) const
> >+  {
> >+    return mAttrsAndChildren.GetAttr(aAttr);
> >+  }
> >+
> There is DoGetClasses(). Is that not enough?
> (Perhaps you need to remove NS_NOTREACHED in nsGenericElement)

Right, I found about nsIContent::GetClasses(), so I didn't need the GetParsedAttr change.

> Please add at least some tests for XHTML, SVG, XUL, MathML and plain XML
> elements.

done.

(In reply to comment #15)
> (From update of attachment 392290 [details] [diff] [review])
> Hope it's ok that I'm stealing this review...
> 
> General comment:
> Node need to use |this->SomeFunction(...)|, just do |SomeFunction(...)|
> instead.

done

> >+nsDOMTokenList::GetLength(PRUint32 *aLength)
> >+{
> >+  const nsAttrValue* attr = mElement ?
> >+                            mElement->GetParsedAttr(nsGkAtoms::_class) :
> >+                            nsnull;
> 
> It'd probably be worth adding an inline function that performs this line.
> Everywhere you call GetPasedAttr you effectively do exactly this.

I've added a GetParsedAttr() function.

> >+PRBool
> >+nsDOMTokenList::ContainsInternal(const nsAttrValue* aAttr,
> >+                                 const nsAString& aToken)
> >+{
> >+  if (!aAttr) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  nsIAtom* atom = NS_NewAtom(aToken);
> >+  PRBool result = aAttr->Contains(atom, eCaseMatters);
> >+  NS_RELEASE(atom);
> 
> Don't use NS_NewAtom. Use do_GetAtom with an nsCOMPtr instead. That way you can
> get rid of the |result| temporary too.

done

> >+nsDOMTokenList::AddInternal(const nsAttrValue* aAttr,
> >+                            const nsAString& aToken)
> >+{
> >+  nsAutoString resultStr;
> >+
> >+  if (aAttr) {
> >+    aAttr->ToString(resultStr);
> >+  }
> >+
> >+  if (!resultStr.IsEmpty() &&
> >+      !nsContentUtils::IsHTMLWhitespace(
> >+          resultStr.CharAt(resultStr.Length() - 1)))
> >+  {
> 
> Put the '{' at the end of the line above to follow convention in the rest of
> this file.

done

> >+    resultStr.Append(PRUnichar(' '));
> >+  }
> >+  resultStr.Append(aToken);
> 
> It'd be slightly faster to do:
> 
> if (...) {
>   resultStr.Append(NS_LITERAL_STRING(" ") + aToken)
> }
> else {
>   resultStr.Append(aToken)
> }
> 
> The reason is that basically every time you call Append we need to resize the
> string buffer and thus reallocate it. However the + operator creates a
> temporary stack-based object without any allocations happening. We then append
> that object in a single operation.

done

I've also replaced the
  NS_ENSURE_SUCCESS(rv, rv);
by
  if (NS_FAILED(rv)) {
    return rv;
  }
in order to avoid tons of warnings when running the tests.


Otherwise, there's one thing that might need to be sorted out regarding on which element the classList property should live, and how it should behave with non styled elements.

The spec says that classList is "only defined on HTML nodes; what happens on non-HTML nodes isn't in HTML's purview" (quoting Ian).

On the other hand, it may be nice to have it for SVG and XUL for instance. As currently implemented, the property is available on all kind of nodes. It works well for HTML/XHTML/XUL/SVG/MathML.
However, for plain XML nodes it behaves as if the class attribute was not  there when using getters (length/item/contains). The explanation is that the class attribute is retrieved through nsIContent::GetClasses, which returns null if the node doesn't have the NODE_MAY_HAVE_CLASS flag, which is the case on plain XML nodes.
Some of the setters in that case still modify the class attribute, sometimes overwriting what was already there.

Some possible options:
- leave things as is.
- hide the classList property for non style elements (could live in the same place as the className properties).
- don't allow class attribute mutation on non styled elements (so in the case above, do nothing or raise when calling add/remove/toggle).

Comment 18

[Sylvain Pasche]

Attachment: v5

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> I've also replaced the
>   NS_ENSURE_SUCCESS(rv, rv);
> by
>   if (NS_FAILED(rv)) {
>     return rv;
>   }
> in order to avoid tons of warnings when running the tests.

Ugh, please don't do this. It makes the code harder to read. Don't use test code as an indication that too many warnings are created as test code is very different from what the normal average code will do.

> Otherwise, there's one thing that might need to be sorted out regarding on
> which element the classList property should live, and how it should behave
> with non styled elements.
> 
> The spec says that classList is "only defined on HTML nodes; what happens on
> non-HTML nodes isn't in HTML's purview" (quoting Ian).

I'd say make it available on nsStyledElement, that will get SVG and mathml too. Unfortunately it won't get XUL, even though that's probably something that should be fixed in the XUL code, so you'll have to add XUL separately as well.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I would also recomment using GetClassAttributeName() instead of hardcoding "class".  That's what the function's for.

Comment 21

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

As bz said yesterday on IRC, plain XML could return null for .classList.
So basically if the element returns non-null GetClassAttributeName(), .classList
should be non-null. That way also XTF-implemented elements would get
.classList automatically, if they have been marked to have a class attribute.
I'd guess adding a simple check to nsNSElementTearoff::GetClassList would be enough for that.
 
  *aResult = nsnull;
  ...  
  if (!slots->mClassList) {
    NS_ENSURE_TRUE(mContent->GetClassAttributeName(), NS_OK);
    slots->mClassList = new nsDOMTokenList(mContent);
    NS_ENSURE_TRUE(slots->mClassList, NS_ERROR_OUT_OF_MEMORY);
  }

Comment 22

[Sylvain Pasche]

Sounds like a good plan, Jonas what do you think?

(by the way, the DOMTokenList interface is used in several other places in HTML 5 for modifying other attributes. I don't remember if that's why I used GetParsedAttr() instead of GetClasses() so it could be more easily generalized to any attribute. Anyway there's no need to prematurely generalize for the moment, but that could be something to keep in mind.)

Comment 23

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

If you want to generalize, perhaps nsDOMTokenList could take the attribute name as a parameter and store that as a member variable.
And in case of .classList, it should be the attribute name
returned by GetClassAttributeName()

Comment 24

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

And ofc in that case GetClasses can't be used, but your original GetParsedAttr()

Comment 25

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

Comment on attachment 393307 [details] [diff] [review]
v5

I assume you're going to upload a new patch.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good. Not sure it's worth generalizing yet either. Seems just as easy to do later.

Comment 27

[Sylvain Pasche]

Attachment: v6

In the end I did something like comment 23, which seemed reasonable after the GetClassAttributeName refactoring.
So now the nsDOMTokenList constructor is taking a nsIAtom parameter.

Comment 28

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

Comment on attachment 393521 [details] [diff] [review]
v6

Would be great to get even some reftests which check that correct class is used for the elements in the layout after modifying .classList.

Comment 29

[Sylvain Pasche]

Attachment: v7

I've added a few reftests for html/xhtml.

Comment 30

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

http://hg.mozilla.org/mozilla-central/rev/6470caf066b2

Comment 31

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/DOM/element.classList

Comment 32

[Boris Zbarsky [:bzbarsky]]

Regression: DHTML increase 0.51% on XP Firefox

Previous results: 1035.06 from build 20090812045356 of revision 269e64bb5421 at 2009-08-12 04:53:00 on talos-rev2-xp16 run # 0

New results: 1040.38 from build 20090812045916 of revision 6470caf066b2 at 2009-08-12 04:59:00 on qm-pxp-fast03 run # 0

Suspected checkin range: from revision 269e64bb5421 to revision 6470caf066b2

Comment 33

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

I wonder what could have caused the regression. The extra 4/8 bytes in slots?

Comment 34

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

The test result varies a lot in qm-pxp-fast03.
The last few days have been between 1034 and 1042.

talos-rev2-xp03 shows some regression, maybe, but the range is wider.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=505615255899&tochange=ca46dd208b99

Comment 35

[Sylvain Pasche]

Attachment: Benchmark comparing speed of native element.classList vs JQuery/YUI/Dojo

I was curious to see how much we can gain with the native implementation.
Performance improvements range from about 2 to 90 depending on the library used and the test performed. It could be interesting to profile cases where the speedup is "only" of 2.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Profiling which?  The native call or the library call?

I'm happy to profile if you give me a testcase that has a simple test to profile (basically a button that when clicked calls a function).

Comment 37

[Sylvain Pasche]

(In reply to comment #36)
> Profiling which?  The native call or the library call?

I was thinking of profiling both and then compare the results. But just profiling the native call might already give valuable information if something could be improved in the new native implementation.

> I'm happy to profile if you give me a testcase that has a simple test to
> profile (basically a button that when clicked calls a function).

Cool, I'll try to come with something like that.

Comment 38

[Tobi Reif]

On http://tddbin.com/#?kata=es6/language/array-api/from , the second test is green right away (in FF, and BTW not in Chrome).

Here's the test:

  document.body.classList.add('some');
  document.body.classList.add('other');
  const classList = document.body.classList;

  assert.deepEqual(classList, ['some', 'other']);

It seems that Firefox uses (something resembling) an array for http://www.w3.org/TR/dom/#domtokenlist , but I might be wrong.

Comment 39

[Josh Matthews [:jdm]]

Tobi, could you file that as a new bug please?

Comment 40

[Boris Zbarsky [:bzbarsky]]

I have no idea what comment 38 is trying to say.  What the code in that comment will do depends on what deepEqual does, obviously...

Comment 41

[Tobi Reif]

The thing is that the result is different in FF vs Chrome - in case that's of interest.

This version

  document.body.classList.add('some');
  document.body.classList.add('other');
  var classList = document.body.classList;
  console.log(classList.__proto__);

yields DOMTokenListPrototype (respectively DOMTokenList in Chrome), not Array as the first version implies, so there seems to be a difference regarding how Firefox/Chrome run assert.deepEqual . Thus for here and now: Nevermind.

Comment 42

[Boris Zbarsky [:bzbarsky]]

OK.  Fwiw, I see the "a DOM node`s classList object can be converted" test green in Chrome dev build on that page...

Comment 43

[Tobi Reif]

Thanks for that info! So it seems the issue will be gone in the future.