Bug 148589 - DOMTokenList update steps for classList don't follow the spec
Summary: DOMTokenList update steps for classList don't follow the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#domtoken...
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2015-08-28 14:14 PDT by Michał Gołębiowski-Owczarek
Modified: 2016-04-07 10:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (86.76 KB, patch)
2015-09-10 16:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (92.74 KB, patch)
2015-09-10 16:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (92.82 KB, patch)
2015-09-10 16:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (94.01 KB, patch)
2015-09-11 13:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michał Gołębiowski-Owczarek 2015-08-28 14:14:46 PDT
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
Comment 1 Michał Gołębiowski-Owczarek 2015-08-28 14:17:50 PDT
Firefox issue: https://bugzilla.mozilla.org/show_bug.cgi?id=869788
Comment 2 Chris Dumez 2015-09-02 15:28:43 PDT
22547443
Comment 3 Chris Dumez 2015-09-02 15:29:02 PDT
rdar://problem/22547443
Comment 4 Chris Dumez 2015-09-10 16:13:52 PDT
Created attachment 260961 [details]
Patch
Comment 5 WebKit Commit Bot 2015-09-10 16:17:17 PDT
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.
Comment 6 Chris Dumez 2015-09-10 16:24:18 PDT
Created attachment 260963 [details]
Patch
Comment 7 WebKit Commit Bot 2015-09-10 16:27:19 PDT
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.
Comment 8 Chris Dumez 2015-09-10 16:45:15 PDT
Created attachment 260969 [details]
Patch
Comment 9 WebKit Commit Bot 2015-09-10 16:46:36 PDT
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.
Comment 10 Darin Adler 2015-09-11 09:36:07 PDT
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.
Comment 11 Chris Dumez 2015-09-11 11:02:12 PDT
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.
Comment 12 Chris Dumez 2015-09-11 13:22:57 PDT
Created attachment 261012 [details]
Patch
Comment 13 Chris Dumez 2015-09-11 13:26:47 PDT
It looks like this may be performance-positive on Dromaeo Selector tests:
http://dromaeo.com/?id=241285,241294
Comment 14 WebKit Commit Bot 2015-09-11 14:33:28 PDT
Comment on attachment 261012 [details]
Patch

Clearing flags on attachment: 261012

Committed r189632: <http://trac.webkit.org/changeset/189632>
Comment 15 WebKit Commit Bot 2015-09-11 14:33:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Michał Gołębiowski-Owczarek 2015-09-11 14:53:08 PDT
Thanks! \o/
Comment 17 Michał Gołębiowski-Owczarek 2016-04-07 08:43:49 PDT
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).
Comment 18 Chris Dumez 2016-04-07 09:24:54 PDT
(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.
Comment 19 Chris Dumez 2016-04-07 10:33:19 PDT
(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.