Element.setAttributeNode() should not treat attribute names case inse…

…nsitively

https://bugs.webkit.org/show_bug.cgi?id=265597

Reviewed by Brent Fulgham.

Stop treating the attribute names insensitively in Element.setAttributeNode() as this didn't
match the DOM specification [1] or the behavior of Chrome / Firefox.

Thanks to Claudio Saavedra from Igalia for finding the bug and starting the investigation
on this.

[1] https://dom.spec.whatwg.org/#dom-element-setattributenode

* LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes.html:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::setAttributeNode):
* Source/WebCore/dom/Element.h:

Canonical link: https://commits.webkit.org/271363@main

LayoutTests/fast/dom/Attr/make-unique-element-data-while-replacing-attr.html

@@ -17,7 +17,7 @@
 
 element.addEventListener('DOMSubtreeModified', () => { element.cloneNode(); }, true);
 
-let newAttr = document.createAttributeNS('http://www.w3.org/1999/xhtml','width');
+let newAttr = document.createAttributeNS('http://www.w3.org/XML/1998/namespace','width');
 newAttr.value = 'b';
 element.setAttributeNode(newAttr);
 

LayoutTests/fast/dom/Element/setAttributeNode-overriding-lowercase-values-2-expected.txt

@@ -16,13 +16,13 @@ PASS testElement.getAttributeNS("", "foobar") is null
 PASS testElement.hasAttributeNS("", "foobar") is false
 PASS testElement.getAttributeNS("", "FooBar") is null
 PASS testElement.hasAttributeNS("", "FooBar") is false
-PASS testElement.attributes.length is 1
-PASS testElement.getAttribute("foobar") is null
-PASS testElement.hasAttribute("foobar") is false
-PASS testElement.getAttribute("FooBar") is null
-PASS testElement.hasAttribute("FooBar") is false
-PASS testElement.getAttributeNS("ns1", "foobar") is null
-PASS testElement.hasAttributeNS("ns1", "foobar") is false
+PASS testElement.attributes.length is 2
+PASS testElement.getAttribute("foobar") is "WebKit"
+PASS testElement.hasAttribute("foobar") is true
+PASS testElement.getAttribute("FooBar") is "WebKit"
+PASS testElement.hasAttribute("FooBar") is true
+PASS testElement.getAttributeNS("ns1", "foobar") is "WebKit"
+PASS testElement.hasAttributeNS("ns1", "foobar") is true
 PASS testElement.getAttributeNS("ns1", "FooBar") is "Rocks!"
 PASS testElement.hasAttributeNS("ns1", "FooBar") is true
 PASS testElement.getAttributeNS("", "foobar") is null

LayoutTests/fast/dom/Element/setAttributeNode-overriding-lowercase-values-2.html

@@ -31,20 +31,18 @@
 shouldBe('testElement.getAttributeNS("", "FooBar")', 'null');
 shouldBeFalse('testElement.hasAttributeNS("", "FooBar")');
 
-// Setting this node through setAttributeNode() instead of setAttributeNodeNS()
-// erases the lowercase "foobar".
 var b = document.createAttributeNS('ns1', 'FooBar');
 b.value = "Rocks!";
 testElement.setAttributeNode(b);
 
-shouldBe('testElement.attributes.length', '1');
-shouldBe('testElement.getAttribute("foobar")', 'null');
-shouldBeFalse('testElement.hasAttribute("foobar")');
-shouldBe('testElement.getAttribute("FooBar")', 'null');
-shouldBeFalse('testElement.hasAttribute("FooBar")');
+shouldBe('testElement.attributes.length', '2');
+shouldBeEqualToString('testElement.getAttribute("foobar")', 'WebKit');
+shouldBeTrue('testElement.hasAttribute("foobar")');
+shouldBeEqualToString('testElement.getAttribute("FooBar")', 'WebKit');
+shouldBeTrue('testElement.hasAttribute("FooBar")');
 
-shouldBe('testElement.getAttributeNS("ns1", "foobar")', 'null');
-shouldBeFalse('testElement.hasAttributeNS("ns1", "foobar")');
+shouldBeEqualToString('testElement.getAttributeNS("ns1", "foobar")', 'WebKit');
+shouldBeTrue('testElement.hasAttributeNS("ns1", "foobar")');
 shouldBeEqualToString('testElement.getAttributeNS("ns1", "FooBar")', 'Rocks!');
 shouldBeTrue('testElement.hasAttributeNS("ns1", "FooBar")');
 

LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes-expected.txt

@@ -50,7 +50,9 @@ PASS Attribute loses its owner when removed
 PASS Basic functionality of getAttributeNode/getAttributeNodeNS
 PASS Basic functionality of setAttributeNode
 PASS setAttributeNode should distinguish attributes with same local name and different namespaces
-PASS setAttributeNode doesn't have case-insensitivity even with an HTMLElement
+PASS setAttributeNode doesn't have case-insensitivity even with an HTMLElement 1
+PASS setAttributeNode doesn't have case-insensitivity even with an HTMLElement 2
+PASS setAttributeNode doesn't have case-insensitivity even with an HTMLElement 3
 PASS Basic functionality of setAttributeNodeNS
 PASS If attr’s element is neither null nor element, throw an InUseAttributeError.
 PASS Replacing an attr by itself

LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes.html

@@ -588,7 +588,32 @@
   el.setAttributeNode(attr2);
   assert_equals(el.getAttributeNodeNS("ns1", "name").value, "value1");
   assert_equals(el.getAttributeNodeNS("ns1", "NAME").value, "VALUE2");
-}, "setAttributeNode doesn't have case-insensitivity even with an HTMLElement")
+}, "setAttributeNode doesn't have case-insensitivity even with an HTMLElement 1")
+
+test(function() {
+  var el = document.createElement("div");
+  var attr1 = document.createAttributeNS("ns1", "FOOBAR");
+  var attr2 = document.createAttributeNS("ns1", "FOOBAR");
+  assert_equals(el.setAttributeNode(attr1), null);
+  assert_equals(attr1.ownerElement, el);
+  assert_equals(attr2.ownerElement, null);
+  var oldAttr = el.setAttributeNode(attr2);
+  assert_equals(oldAttr, attr1);
+  assert_equals(attr1.ownerElement, null);
+  assert_equals(attr2.ownerElement, el);
+}, "setAttributeNode doesn't have case-insensitivity even with an HTMLElement 2")
+
+test(function() {
+  var el = document.createElement("div");
+  var attr1 = document.createAttributeNS("ns1", "foobar");
+  var attr2 = document.createAttributeNS("ns1", "FOOBAR");
+  assert_equals(el.setAttributeNode(attr1), null);
+  assert_equals(attr1.ownerElement, el);
+  assert_equals(attr2.ownerElement, null);
+  assert_equals(el.setAttributeNode(attr2), null);
+  assert_equals(attr1.ownerElement, el);
+  assert_equals(attr2.ownerElement, el);
+}, "setAttributeNode doesn't have case-insensitivity even with an HTMLElement 3")
 
 test(function() {
   var el = document.createElement("div")

Source/WebCore/dom/Element.cpp

@@ -208,16 +208,6 @@ static Attr* findAttrNodeInList(Vector<RefPtr<Attr>>& attrNodeList, const Qualif
     return nullptr;
 }
 
-static Attr* findAttrNodeInList(Vector<RefPtr<Attr>>& attrNodeList, const AtomString& localName, bool shouldIgnoreAttributeCase)
-{
-    const AtomString& caseAdjustedName = shouldIgnoreAttributeCase ? localName.convertToASCIILowercase() : localName;
-    for (auto& node : attrNodeList) {
-        if (node->qualifiedName().localName() == caseAdjustedName)
-            return node.get();
-    }
-    return nullptr;
-}
-
 static bool shouldAutofocus(const Element& element)
 {
     if (!element.hasAttributeWithoutSynchronization(HTMLNames::autofocusAttr))
@@ -3238,7 +3228,7 @@ void Element::attachAttributeNodeIfNeeded(Attr& attrNode)
 
 ExceptionOr<RefPtr<Attr>> Element::setAttributeNode(Attr& attrNode)
 {
-    RefPtr oldAttrNode = attrIfExists(attrNode.localName(), shouldIgnoreAttributeCase(*this));
+    RefPtr oldAttrNode = attrIfExists(attrNode.qualifiedName());
     if (oldAttrNode.get() == &attrNode)
         return oldAttrNode;
 
@@ -3254,7 +3244,7 @@ ExceptionOr<RefPtr<Attr>> Element::setAttributeNode(Attr& attrNode)
 
     auto& elementData = ensureUniqueElementData();
 
-    auto existingAttributeIndex = elementData.findAttributeIndexByName(attrNode.localName(), shouldIgnoreAttributeCase(*this));
+    auto existingAttributeIndex = elementData.findAttributeIndexByName(attrNode.qualifiedName());
 
     // Attr::value() will return its 'm_standaloneValue' member any time its Element is set to nullptr. We need to cache this value
     // before making changes to attrNode's Element connections.
@@ -4926,13 +4916,6 @@ void Element::setSavedLayerScrollPositionSlow(const IntPoint& position)
     ensureElementRareData().setSavedLayerScrollPosition(position);
 }
 
-RefPtr<Attr> Element::attrIfExists(const AtomString& localName, bool shouldIgnoreAttributeCase)
-{
-    if (auto* attrNodeList = attrNodeListForElement(*this))
-        return findAttrNodeInList(*attrNodeList, localName, shouldIgnoreAttributeCase);
-    return nullptr;
-}
-
 RefPtr<Attr> Element::attrIfExists(const QualifiedName& name)
 {
     if (auto* attrNodeList = attrNodeListForElement(*this))

Source/WebCore/dom/Element.h

@@ -272,7 +272,6 @@ class Element : public ContainerNode {
     WEBCORE_EXPORT ExceptionOr<Ref<Attr>> removeAttributeNode(Attr&);
 
     RefPtr<Attr> attrIfExists(const QualifiedName&);
-    RefPtr<Attr> attrIfExists(const AtomString& localName, bool shouldIgnoreAttributeCase);
     Ref<Attr> ensureAttr(const QualifiedName&);
 
     const Vector<RefPtr<Attr>>& attrNodeList();