Bug 178280 – DOMTokenList shouldn't add empty attributes

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

DOMTokenList shouldn't add empty attributes after:
- https://github.com/whatwg/dom/pull/488

Firefox and Chrome follow the latest spec.

[Chris Dumez]

Created attachment 323729 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 323729 [details]
Patch

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

> Source/WebCore/html/DOMTokenList.cpp:252
> +    if (!m_element.hasAttribute(m_attributeName) && tokens().isEmpty())
> +        return;

It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice
but I guess we don't have a mechanism to update the attributes using Attribute easily.

[WebKit Commit Bot]

Comment on attachment 323729 [details]
Patch

Clearing flags on attachment: 323729

Committed r223306: <https://trac.webkit.org/changeset/223306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34987431>

[Darin Adler]

Comment on attachment 323729 [details]
Patch

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

>> Source/WebCore/html/DOMTokenList.cpp:252
>> +        return;
> 
> It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice
> but I guess we don't have a mechanism to update the attributes using Attribute easily.

Because hasAttribute is a hash table lookup it is more expensive, so we should check tokens().isEmpty() first.

[Darin Adler]

Comment on attachment 323729 [details]
Patch

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

>>> Source/WebCore/html/DOMTokenList.cpp:252
>>> +        return;
>> 
>> It's a bit inefficient to check this and then later call setAttribute since that would find the attribute twice
>> but I guess we don't have a mechanism to update the attributes using Attribute easily.
> 
> Because hasAttribute is a hash table lookup it is more expensive, so we should check tokens().isEmpty() first.

I am not so happy with the specification here: It is inconsistent that an empty token list will replace an *existing* attribute's value with empty string rather than deleting the attribute, but if there is no existing attribute, it will make no change. It would be more logical if it always removed the attribute. If that was the design, for our implementation could simply serialize an empty set of tokens as the null string instead of the empty string and otherwise let the normal algorithm run.

I’d really prefer that we make a solution that looks up the attribute only once as Ryosuke says. But sadly there is no variation of setAttribute that will set an existing attribute to empty string but not create a new one. We could create that new function in Element I suppose.

[Chris Dumez]

Reopening for follow-up.

[Chris Dumez]

Created attachment 323848 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323848 [details]
Patch

Clearing flags on attachment: 323848

Committed r223330: <https://trac.webkit.org/changeset/223330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.