RESOLVED FIXED 178280
DOMTokenList shouldn't add empty attributes
https://bugs.webkit.org/show_bug.cgi?id=178280
Summary DOMTokenList shouldn't add empty attributes
Chris Dumez
Reported 2017-10-13 13:01:01 PDT
DOMTokenList shouldn't add empty attributes after: - https://github.com/whatwg/dom/pull/488 Firefox and Chrome follow the latest spec.
Attachments
Patch (10.43 KB, patch)
2017-10-13 13:05 PDT, Chris Dumez
no flags
Patch (1.47 KB, patch)
2017-10-15 13:01 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2017-10-13 13:05:19 PDT
Ryosuke Niwa
Comment 2 2017-10-13 13:15:01 PDT
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 3 2017-10-13 15:36:31 PDT
Comment on attachment 323729 [details] Patch Clearing flags on attachment: 323729 Committed r223306: <https://trac.webkit.org/changeset/223306>
WebKit Commit Bot
Comment 4 2017-10-13 15:36:33 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 5 2017-10-13 15:37:25 PDT
Darin Adler
Comment 6 2017-10-14 21:32:48 PDT
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 7 2017-10-14 21:37:41 PDT
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
Comment 8 2017-10-15 11:29:42 PDT
Reopening for follow-up.
Chris Dumez
Comment 9 2017-10-15 13:01:35 PDT
WebKit Commit Bot
Comment 10 2017-10-15 14:58:46 PDT
Comment on attachment 323848 [details] Patch Clearing flags on attachment: 323848 Committed r223330: <https://trac.webkit.org/changeset/223330>
WebKit Commit Bot
Comment 11 2017-10-15 14:58:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.