Form control may be associated with the wrong HTML Form element after form id change
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Apr 2015 00:03:55 +0000 (00:03 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Apr 2015 00:03:55 +0000 (00:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=133456
<rdar://problem/17095055>

Reviewed by Andy Estes.

Source/WebCore:

Fixes an issue where a form control may be associated with the wrong HTML Form element
after the id of the HTML Form element associated with the form control is changed when
there is more than one HTML Form element with the same id in the document. Specifically,
a form control that has an HTML form attribute value X will always be associated with
some HTML Form element f where f.id = X regardless of whether f.id is subsequently
changed.

Tests: fast/forms/change-form-id-to-be-unique-then-submit-form.html
       fast/forms/change-form-id-to-be-unique.html

* dom/Element.cpp:
(WebCore::Element::attributeChanged): Notify observers when the id of an element changed.
(WebCore::Element::updateId): Added parameter NotifyObservers (defaults to NotifyObservers::Yes),
as to whether we should notify observers of the id change.
(WebCore::Element::updateIdForTreeScope): Ditto.
(WebCore::Element::willModifyAttribute): Do not notify observers of the id change immediately. As
indicated by the name of this method, we plan to modify the DOM attribute id of the element, but
we have not actually modified it when this method is called. Instead we will notify observers
in Element::attributeChanged(), which is called after the DOM attribute id is modified.
(WebCore::Element::cloneAttributesFromElement): Ditto.
* dom/Element.h: Defined enum class NotifyObservers.
* dom/TreeScope.cpp:
(WebCore::TreeScope::addElementById): Added boolean parameter notifyObservers (defaults to true)
as to whether we should dispatch a notification to all observers.
(WebCore::TreeScope::removeElementById): Ditto.
* dom/TreeScope.h:

LayoutTests:

Add tests to ensure that we associate the correct HTML Form element with a
<select> after changing the id of its associated HTML form element.

* fast/forms/change-form-id-to-be-unique-expected.txt: Added.
* fast/forms/change-form-id-to-be-unique-then-submit-form-expected.txt: Added.
* fast/forms/change-form-id-to-be-unique-then-submit-form.html: Added.
* fast/forms/change-form-id-to-be-unique.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@183436 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/forms/change-form-id-to-be-unique-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form.html [new file with mode: 0644]
LayoutTests/fast/forms/change-form-id-to-be-unique.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/TreeScope.cpp
Source/WebCore/dom/TreeScope.h

index 0d35512..cbc2ecc 100644 (file)
@@ -1,3 +1,19 @@
+2015-04-27  Daniel Bates  <dabates@apple.com>
+
+        Form control may be associated with the wrong HTML Form element after form id change
+        https://bugs.webkit.org/show_bug.cgi?id=133456
+        <rdar://problem/17095055>
+
+        Reviewed by Andy Estes.
+
+        Add tests to ensure that we associate the correct HTML Form element with a
+        <select> after changing the id of its associated HTML form element.
+
+        * fast/forms/change-form-id-to-be-unique-expected.txt: Added.
+        * fast/forms/change-form-id-to-be-unique-then-submit-form-expected.txt: Added.
+        * fast/forms/change-form-id-to-be-unique-then-submit-form.html: Added.
+        * fast/forms/change-form-id-to-be-unique.html: Added.
+
 2015-04-27  Jer Noble  <jer.noble@apple.com>
 
         Add a setting & restriction which prevents non-interactivte playback of audible media elements.
diff --git a/LayoutTests/fast/forms/change-form-id-to-be-unique-expected.txt b/LayoutTests/fast/forms/change-form-id-to-be-unique-expected.txt
new file mode 100644 (file)
index 0000000..bdc24c4
--- /dev/null
@@ -0,0 +1 @@
+PASS did not cause an assertion failure.
diff --git a/LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form-expected.txt b/LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form-expected.txt
new file mode 100644 (file)
index 0000000..8e3e8bd
--- /dev/null
@@ -0,0 +1,4 @@
+This tests that we submit the form element associated with id "a" after changing the id of one of the <form id="a">s in a document that contains two such HTML Form elements.
+
+PASS submitted second <form>.
+
diff --git a/LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form.html b/LayoutTests/fast/forms/change-form-id-to-be-unique-then-submit-form.html
new file mode 100644 (file)
index 0000000..772b942
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+#test-container { visibility: hidden; }
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+function logMessageAndDone(message)
+{
+    document.getElementById("console").textContent = message;
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+function runTest()
+{
+    var search = document.location.search;
+    if (search === "?submitted=secondFormElement")
+        logMessageAndDone("PASS submitted second <form>.");
+    else if (search === "?submitted=firstFormElement")
+        logMessageAndDone("FAIL should have submitted second <form>, but submitted first <form>.");
+    else {
+        document.getElementById("a").id = "y"; // Changes the id of the first <form> (traversing the DOM from top-to-bottom).
+        document.getElementById("submit").click();
+    }
+}
+
+window.onload = runTest;
+</script>
+</head>
+<body>
+<p>This tests that we submit the form element associated with id &quot;a&quot; after changing the id of one of the &lt;form id=&quot;a&quot;&gt;s in a document that contains two such HTML Form elements.</p>
+<div id="console"></div>
+<div id="test-container">
+    <form id="a"><input type="hidden" name="submitted" value="firstFormElement"></form>
+    <form id="a"><input type="hidden" name="submitted" value="secondFormElement"></form>
+    <input id="submit" type="submit" form="a" value="Submit">
+</div>
+</body>
+</html>
diff --git a/LayoutTests/fast/forms/change-form-id-to-be-unique.html b/LayoutTests/fast/forms/change-form-id-to-be-unique.html
new file mode 100644 (file)
index 0000000..53de539
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+</head>
+<body onload="document.body.removeChild(document.getElementById('x'));">
+<p>PASS did not cause an assertion failure.</p>
+<div id="x">
+    <form id="a"></form>
+    <form id="a">
+        <select form="a"></select>
+    </form>
+</div>
+<script>
+document.getElementById("a").setAttribute("id", "y");
+</script>
+</body>
+</html>
index 0482e74..b796349 100644 (file)
@@ -1,3 +1,38 @@
+2015-04-27  Daniel Bates  <dabates@apple.com>
+
+        Form control may be associated with the wrong HTML Form element after form id change
+        https://bugs.webkit.org/show_bug.cgi?id=133456
+        <rdar://problem/17095055>
+
+        Reviewed by Andy Estes.
+
+        Fixes an issue where a form control may be associated with the wrong HTML Form element
+        after the id of the HTML Form element associated with the form control is changed when
+        there is more than one HTML Form element with the same id in the document. Specifically,
+        a form control that has an HTML form attribute value X will always be associated with
+        some HTML Form element f where f.id = X regardless of whether f.id is subsequently
+        changed.
+
+        Tests: fast/forms/change-form-id-to-be-unique-then-submit-form.html
+               fast/forms/change-form-id-to-be-unique.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::attributeChanged): Notify observers when the id of an element changed.
+        (WebCore::Element::updateId): Added parameter NotifyObservers (defaults to NotifyObservers::Yes),
+        as to whether we should notify observers of the id change.
+        (WebCore::Element::updateIdForTreeScope): Ditto.
+        (WebCore::Element::willModifyAttribute): Do not notify observers of the id change immediately. As
+        indicated by the name of this method, we plan to modify the DOM attribute id of the element, but
+        we have not actually modified it when this method is called. Instead we will notify observers
+        in Element::attributeChanged(), which is called after the DOM attribute id is modified.
+        (WebCore::Element::cloneAttributesFromElement): Ditto.
+        * dom/Element.h: Defined enum class NotifyObservers.
+        * dom/TreeScope.cpp:
+        (WebCore::TreeScope::addElementById): Added boolean parameter notifyObservers (defaults to true)
+        as to whether we should dispatch a notification to all observers.
+        (WebCore::TreeScope::removeElementById): Ditto.
+        * dom/TreeScope.h:
+
 2015-04-27  Alex Christensen  <achristensen@webkit.org>
 
         Reduce allocations and memory usage when compiling content extensions.
index b238d81..d4d8417 100644 (file)
@@ -56,6 +56,7 @@
 #include "HTMLSelectElement.h"
 #include "HTMLTableRowsCollection.h"
 #include "HTMLTemplateElement.h"
+#include "IdTargetObserverRegistry.h"
 #include "InsertionPoint.h"
 #include "KeyboardEvent.h"
 #include "MutationObserverInterestGroup.h"
@@ -1227,6 +1228,11 @@ void Element::attributeChanged(const QualifiedName& name, const AtomicString& ol
     bool shouldInvalidateStyle = false;
 
     if (name == HTMLNames::idAttr) {
+        if (!oldValue.isEmpty())
+            treeScope().idTargetObserverRegistry().notifyObservers(*oldValue.impl());
+        if (!newValue.isEmpty())
+            treeScope().idTargetObserverRegistry().notifyObservers(*newValue.impl());
+
         AtomicString oldId = elementData()->idForStyleResolution();
         AtomicString newId = makeIdForStyleResolution(newValue, document().inQuirksMode());
         if (newId != oldId) {
@@ -2968,7 +2974,7 @@ void Element::updateNameForDocument(HTMLDocument& document, const AtomicString&
     }
 }
 
-inline void Element::updateId(const AtomicString& oldId, const AtomicString& newId)
+inline void Element::updateId(const AtomicString& oldId, const AtomicString& newId, NotifyObservers notifyObservers)
 {
     if (!isInTreeScope())
         return;
@@ -2976,7 +2982,7 @@ inline void Element::updateId(const AtomicString& oldId, const AtomicString& new
     if (oldId == newId)
         return;
 
-    updateIdForTreeScope(treeScope(), oldId, newId);
+    updateIdForTreeScope(treeScope(), oldId, newId, notifyObservers);
 
     if (!inDocument())
         return;
@@ -2985,15 +2991,15 @@ inline void Element::updateId(const AtomicString& oldId, const AtomicString& new
     updateIdForDocument(downcast<HTMLDocument>(document()), oldId, newId, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute);
 }
 
-void Element::updateIdForTreeScope(TreeScope& scope, const AtomicString& oldId, const AtomicString& newId)
+void Element::updateIdForTreeScope(TreeScope& scope, const AtomicString& oldId, const AtomicString& newId, NotifyObservers notifyObservers)
 {
     ASSERT(isInTreeScope());
     ASSERT(oldId != newId);
 
     if (!oldId.isEmpty())
-        scope.removeElementById(*oldId.impl(), *this);
+        scope.removeElementById(*oldId.impl(), *this, notifyObservers == NotifyObservers::Yes);
     if (!newId.isEmpty())
-        scope.addElementById(*newId.impl(), *this);
+        scope.addElementById(*newId.impl(), *this, notifyObservers == NotifyObservers::Yes);
 }
 
 void Element::updateIdForDocument(HTMLDocument& document, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition condition)
@@ -3037,7 +3043,7 @@ void Element::updateLabel(TreeScope& scope, const AtomicString& oldForAttributeV
 void Element::willModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
 {
     if (name == HTMLNames::idAttr)
-        updateId(oldValue, newValue);
+        updateId(oldValue, newValue, NotifyObservers::No); // Will notify observers after the attribute is actually changed.
     else if (name == HTMLNames::nameAttr)
         updateName(oldValue, newValue);
     else if (name == HTMLNames::forAttr && hasTagName(labelTag)) {
@@ -3266,7 +3272,7 @@ void Element::cloneAttributesFromElement(const Element& other)
     const AtomicString& newID = other.getIdAttribute();
 
     if (!oldID.isNull() || !newID.isNull())
-        updateId(oldID, newID);
+        updateId(oldID, newID, NotifyObservers::No); // Will notify observers after the attribute is actually changed.
 
     const AtomicString& oldName = getNameAttribute();
     const AtomicString& newName = other.getNameAttribute();
index 5a687b0..d7fd199 100644 (file)
@@ -544,8 +544,11 @@ private:
     void updateName(const AtomicString& oldName, const AtomicString& newName);
     void updateNameForTreeScope(TreeScope&, const AtomicString& oldName, const AtomicString& newName);
     void updateNameForDocument(HTMLDocument&, const AtomicString& oldName, const AtomicString& newName);
-    void updateId(const AtomicString& oldId, const AtomicString& newId);
-    void updateIdForTreeScope(TreeScope&, const AtomicString& oldId, const AtomicString& newId);
+
+    enum class NotifyObservers { No, Yes };
+    void updateId(const AtomicString& oldId, const AtomicString& newId, NotifyObservers = NotifyObservers::Yes);
+    void updateIdForTreeScope(TreeScope&, const AtomicString& oldId, const AtomicString& newId, NotifyObservers = NotifyObservers::Yes);
+
     enum HTMLDocumentNamedItemMapsUpdatingCondition { AlwaysUpdateHTMLDocumentNamedItemMaps, UpdateHTMLDocumentNamedItemMapsOnlyIfDiffersFromNameAttribute };
     void updateIdForDocument(HTMLDocument&, const AtomicString& oldId, const AtomicString& newId, HTMLDocumentNamedItemMapsUpdatingCondition);
     void updateLabel(TreeScope&, const AtomicString& oldForAttributeValue, const AtomicString& newForAttributeValue);
index a9249d6..42ad921 100644 (file)
@@ -127,20 +127,22 @@ const Vector<Element*>* TreeScope::getAllElementsById(const AtomicString& elemen
     return m_elementsById->getAllElementsById(*elementId.impl(), *this);
 }
 
-void TreeScope::addElementById(const AtomicStringImpl& elementId, Element& element)
+void TreeScope::addElementById(const AtomicStringImpl& elementId, Element& element, bool notifyObservers)
 {
     if (!m_elementsById)
         m_elementsById = std::make_unique<DocumentOrderedMap>();
     m_elementsById->add(elementId, element, *this);
-    m_idTargetObserverRegistry->notifyObservers(elementId);
+    if (notifyObservers)
+        m_idTargetObserverRegistry->notifyObservers(elementId);
 }
 
-void TreeScope::removeElementById(const AtomicStringImpl& elementId, Element& element)
+void TreeScope::removeElementById(const AtomicStringImpl& elementId, Element& element, bool notifyObservers)
 {
     if (!m_elementsById)
         return;
     m_elementsById->remove(elementId, element);
-    m_idTargetObserverRegistry->notifyObservers(elementId);
+    if (notifyObservers)
+        m_idTargetObserverRegistry->notifyObservers(elementId);
 }
 
 Element* TreeScope::getElementByName(const AtomicString& name) const
index 6b7f9ea..39416fa 100644 (file)
@@ -59,8 +59,8 @@ public:
     const Vector<Element*>* getAllElementsById(const AtomicString&) const;
     bool hasElementWithId(const AtomicStringImpl&) const;
     bool containsMultipleElementsWithId(const AtomicString& id) const;
-    void addElementById(const AtomicStringImpl& elementId, Element&);
-    void removeElementById(const AtomicStringImpl& elementId, Element&);
+    void addElementById(const AtomicStringImpl& elementId, Element&, bool notifyObservers = true);
+    void removeElementById(const AtomicStringImpl& elementId, Element&, bool notifyObservers = true);
 
     Element* getElementByName(const AtomicString&) const;
     bool hasElementWithName(const AtomicStringImpl&) const;