DOMTokenList shouldn't add empty attributes after: - https://github.com/whatwg/dom/pull/488 Firefox and Chrome follow the latest spec.
Created attachment 323729 [details] Patch
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.
Comment on attachment 323729 [details] Patch Clearing flags on attachment: 323729 Committed r223306: <https://trac.webkit.org/changeset/223306>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34987431>
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.
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.
Reopening for follow-up.
Created attachment 323848 [details] Patch
Comment on attachment 323848 [details] Patch Clearing flags on attachment: 323848 Committed r223330: <https://trac.webkit.org/changeset/223330>