Bug 148589 – DOMTokenList update steps for classList don't follow the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michał Gołębiowski-Owczarek]

Steps to reproduce the problem:
```
var div = document.createElement('div');
div.className = '   a   b   c   ';
div.classList.remove('b');

console.log(div.className); // "  a c  "
```


`DOMTokenList` [update steps](https://dom.spec.whatwg.org/#domtokenlist)' second point is:

"Set an attribute value for the associated element using associated attribute’s local name and the result of running the ordered set serializer for tokens."

For `classList` the "associated attribute's local name" is `class`. The ["ordered set serializer" algorithm](https://dom.spec.whatwg.org/#concept-ordered-set-serializer) is:
"The ordered set serializer takes a set and returns the concatenation of the strings in set, separated from each other by U+0020". This means that after updating the context object's classes via the classList API, className value (that reflects the "class" attribute value) should be "a c", not "   a c   ".

Edge is the only browser that returns "a c" correctly for the above test case.


https://code.google.com/p/chromium/issues/detail?id=526282

[Michał Gołębiowski-Owczarek]

Firefox issue: https://bugzilla.mozilla.org/show_bug.cgi?id=869788

[Chris Dumez]

22547443

[Chris Dumez]

rdar://problem/22547443

[Chris Dumez]

Created attachment 260961 [details]
Patch

[WebKit Commit Bot]

Attachment 260961 [details] did not pass style-queue:


ERROR: Source/WebCore/html/DOMTokenList.h:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/DOMTokenList.h:55:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 260963 [details]
Patch

[WebKit Commit Bot]

Attachment 260963 [details] did not pass style-queue:


ERROR: Source/WebCore/html/DOMTokenList.h:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/DOMTokenList.h:55:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 260969 [details]
Patch

[WebKit Commit Bot]

Attachment 260969 [details] did not pass style-queue:


ERROR: Source/WebCore/html/DOMTokenList.h:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/DOMTokenList.h:55:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 260969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260969&action=review

> Source/WebCore/html/AttributeDOMTokenList.h:42
> +    virtual void ref() override { m_element.ref(); }
> +    virtual void deref() override { m_element.deref(); }
> +
> +    virtual Element* element() const override { return &m_element; }

Can any of these be private?

> Source/WebCore/html/AttributeDOMTokenList.h:48
> +    const WebCore::QualifiedName m_attributeName;

Could consider making this a reference; no need to churn the reference counts on the global immortal attribute names.

> Source/WebCore/html/DOMTokenList.cpp:73
> +    Vector<AtomicString> uniqueTokens;

This is a temporary that’s only used on the stack. I suggest some inline capacity to get rid of one heap allocation.

> Source/WebCore/html/DOMTokenList.cpp:136
> +        for (size_t i = 0; i < m_tokens.size(); ++i) {
> +            builder.append(m_tokens[i]);
> +            if (i != m_tokens.size() - 1)
> +                builder.append(' ');
> +        }

I liked the old way with a boolean because then we could use a modern for loop. Also might be nice to teach StringBuilder how to join and append, since this seems like a common idiom.

> Source/WebCore/html/DOMTokenList.cpp:162
> +        // We do a simple linear search as there are usually not many tokens.
> +        if (!m_tokens.contains(token))
> +            m_tokens.append(token);

Seems a little dangerous to assume there’s never a giant one of these.

> Source/WebCore/html/DOMTokenList.h:53
> +    void add(const AtomicString& token, ExceptionCode& ec) { add(Vector<String>{token.string()}, ec); }

Seems wasteful to allocate a vector just to do this. There are multiple ways to avoid that; one is to have the worker function that takes a “view” on a vector, just a pointer plus a length.

I also think we are going too far in putting function bodies in the class. I’d like to see interface and implementation a bit more separate by putting the inline function bodies after the class.

> Source/WebCore/html/DOMTokenList.h:72
>  protected:
> -    virtual AtomicString value() const = 0;
> -    virtual void setValue(const AtomicString&) = 0;
> +    const AtomicString& value() const;
> +    void setValue(const String&);
> +    virtual void updateAfterTokenChange() { m_cachedValue = nullAtom; }
>  
> -    void addInternal(const AtomicString&);
> -    virtual bool containsInternal(const AtomicString&) const = 0;
> -    void removeInternal(const AtomicString&);
> -
> -    static bool validateToken(const AtomicString&, ExceptionCode&);
> +    static bool validateToken(const String&, ExceptionCode&);
>      static bool validateTokens(const Vector<String>&, ExceptionCode&);
> -    static String addToken(const AtomicString&, const AtomicString&);
> -    static String addTokens(const AtomicString&, const Vector<String>&);
> -    static String removeToken(const AtomicString&, const AtomicString&);
> -    static String removeTokens(const AtomicString&, const Vector<String>&);
> +
> +    Vector<AtomicString> m_tokens;
> +    mutable AtomicString m_cachedValue;

Do derived classes need access to all of these? I think at least some of this should be private rather than protected.

[Chris Dumez]

Comment on attachment 260969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260969&action=review

>> Source/WebCore/html/AttributeDOMTokenList.h:42
>> +    virtual Element* element() const override { return &m_element; }
> 
> Can any of these be private?

I believe so, will fix.

>> Source/WebCore/html/AttributeDOMTokenList.h:48
>> +    const WebCore::QualifiedName m_attributeName;
> 
> Could consider making this a reference; no need to churn the reference counts on the global immortal attribute names.

Ok.

>> Source/WebCore/html/DOMTokenList.cpp:73
>> +    Vector<AtomicString> uniqueTokens;
> 
> This is a temporary that’s only used on the stack. I suggest some inline capacity to get rid of one heap allocation.

Ok, I will browse a little bit to determine a decent inline capacity.

>> Source/WebCore/html/DOMTokenList.cpp:136
>> +        }
> 
> I liked the old way with a boolean because then we could use a modern for loop. Also might be nice to teach StringBuilder how to join and append, since this seems like a common idiom.

Ok.

>> Source/WebCore/html/DOMTokenList.cpp:162
>> +            m_tokens.append(token);
> 
> Seems a little dangerous to assume there’s never a giant one of these.

Hmm. I will think a bit about this. We could allocate a HashMap on the stack to make this faster.

>> Source/WebCore/html/DOMTokenList.h:53
>> +    void add(const AtomicString& token, ExceptionCode& ec) { add(Vector<String>{token.string()}, ec); }
> 
> Seems wasteful to allocate a vector just to do this. There are multiple ways to avoid that; one is to have the worker function that takes a “view” on a vector, just a pointer plus a length.
> 
> I also think we are going too far in putting function bodies in the class. I’d like to see interface and implementation a bit more separate by putting the inline function bodies after the class.

Hmm. Agreed. I did not bother because the existing code was doing this and I am pretty sure this is only used by legacy ObjC bindings. However, I will look into the change you're suggesting.

>> Source/WebCore/html/DOMTokenList.h:72
>> +    mutable AtomicString m_cachedValue;
> 
> Do derived classes need access to all of these? I think at least some of this should be private rather than protected.

Ok.

[Chris Dumez]

Created attachment 261012 [details]
Patch

[Chris Dumez]

It looks like this may be performance-positive on Dromaeo Selector tests:
http://dromaeo.com/?id=241285,241294

[WebKit Commit Bot]

Comment on attachment 261012 [details]
Patch

Clearing flags on attachment: 261012

Committed r189632: <http://trac.webkit.org/changeset/189632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michał Gołębiowski-Owczarek]

Thanks! \o/

[Michał Gołębiowski-Owczarek]

It seems the algorithm used in Safari Technology Preview serializes too aggressively; see the spec issue at https://github.com/whatwg/dom/issues/105; the Tech Preview follows the old behavior, the current Safari 9 follows the new one.

Edge 13 which implemented DOMTokenList update steps properly applies them on add/remove/toggle but not just on a regular stringification if those are not involved (so it follows the current spec correctly, I think).

[Chris Dumez]

(In reply to comment #17)
> It seems the algorithm used in Safari Technology Preview serializes too
> aggressively; see the spec issue at
> https://github.com/whatwg/dom/issues/105; the Tech Preview follows the old
> behavior, the current Safari 9 follows the new one.
> 
> Edge 13 which implemented DOMTokenList update steps properly applies them on
> add/remove/toggle but not just on a regular stringification if those are not
> involved (so it follows the current spec correctly, I think).

Would you mind filing a new bug? I can probably fix this soon.

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #17)
> > It seems the algorithm used in Safari Technology Preview serializes too
> > aggressively; see the spec issue at
> > https://github.com/whatwg/dom/issues/105; the Tech Preview follows the old
> > behavior, the current Safari 9 follows the new one.
> > 
> > Edge 13 which implemented DOMTokenList update steps properly applies them on
> > add/remove/toggle but not just on a regular stringification if those are not
> > involved (so it follows the current spec correctly, I think).
> 
> Would you mind filing a new bug? I can probably fix this soon.

Although, since we were matching the spec until they changed it recently and considering that our new behavior is nicer, I think we should try and push back on the spec.