Merge r183436 - Form control may be associated with the wrong HTML Fo…

…rm element after form id change 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.
WebKit · Feb 28, 2016 · 8431ec4 · 8431ec4
1 parent 80b7d6c
commit 8431ec4
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 15 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -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-30  Brady Eidson  <beidson@apple.com>
 
         Javascript using WebSQL can create their own WebKit info table.

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
@@ -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
@@ -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
@@ -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
@@ -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>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -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:
+
 2014-02-27  Ryosuke Niwa  <rniwa@webkit.org>
 
         Element::attributeChanged shouldn't do any work when attribute value didn't change

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -54,6 +54,7 @@
 #include "HTMLParserIdioms.h"
 #include "HTMLSelectElement.h"
 #include "HTMLTableRowsCollection.h"
+#include "IdTargetObserverRegistry.h"
 #include "InsertionPoint.h"
 #include "KeyboardEvent.h"
 #include "MutationObserverInterestGroup.h"
@@ -1105,6 +1106,11 @@ void Element::attributeChanged(const QualifiedName& name, const AtomicString& ol
     bool shouldInvalidateStyle = false;
 
     if (isIdAttributeName(name)) {
+        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) {
@@ -2735,15 +2741,15 @@ 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;
 
     if (oldId == newId)
         return;
 
-    updateIdForTreeScope(treeScope(), oldId, newId);
+    updateIdForTreeScope(treeScope(), oldId, newId, notifyObservers);
 
     if (!inDocument())
         return;
@@ -2752,15 +2758,15 @@ inline void Element::updateId(const AtomicString& oldId, const AtomicString& new
     updateIdForDocument(toHTMLDocument(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)
@@ -2804,7 +2810,7 @@ void Element::updateLabel(TreeScope& scope, const AtomicString& oldForAttributeV
 void Element::willModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
 {
     if (isIdAttributeName(name))
-        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)) {
@@ -3023,7 +3029,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();

diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
@@ -621,8 +621,11 @@ class Element : public ContainerNode {
     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);

diff --git a/Source/WebCore/dom/TreeScope.cpp b/Source/WebCore/dom/TreeScope.cpp
@@ -150,20 +150,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 = adoptPtr(new 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

diff --git a/Source/WebCore/dom/TreeScope.h b/Source/WebCore/dom/TreeScope.h
@@ -59,8 +59,8 @@ class TreeScope {
     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;