Form-association for <img> is inconsistent

https://bugs.webkit.org/show_bug.cgi?id=129742 Reviewed by Darin Adler. This patch updates the way HTMLImageElement associates itself with HTMLFormElement to be more consistent with FormAssociatedElement. New behavior matches that of Gecko and Blink. * LayoutTests/fast/forms/image/image-named-access-expected.txt: Added. * LayoutTests/fast/forms/image/image-named-access.html: Added. * Source/WebCore/html/HTMLFormElement.cpp: (WebCore::HTMLFormElement::removedFromAncestor): * Source/WebCore/html/HTMLImageElement.cpp: (WebCore::HTMLImageElement::~HTMLImageElement): (WebCore::HTMLImageElement::form const): Added. Returns m_form instead of returning the form ancestor. (WebCore::HTMLImageElement::setForm): Extracted from insertedIntoAncestor. Automatically calls removeImgElement and registerImgElement. (WebCore::HTMLImageElement::formOwnerRemovedFromTree): Added. Clear m_form when the associated form element is no longer in the same subtree as this element. (WebCore::HTMLImageElement::insertedIntoAncestor): Don't associate this element with a from element set by the parser if the form had been disconnected like FormAssociatedElement. (WebCore::HTMLImageElement::removedFromAncestor): Don't remove this element from the form element if the root node didn't change like FormAssociatedElement. * Source/WebCore/html/HTMLImageElement.h: Canonical link: https://commits.webkit.org/256629@main
WebKit · Nov 14, 2022 · 36537bb39ed82312d1d971018950910fac8b8b6a · 36537bb
1 parent 285aa4f
commit 36537bb39ed82312d1d971018950910fac8b8b6a
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 16 deletions.
diff --git a/LayoutTests/fast/forms/image/image-named-access-expected.txt b/LayoutTests/fast/forms/image/image-named-access-expected.txt
@@ -0,0 +1,12 @@
+This tests removing an image element with a form element ancestor during parsing
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS form["img1"] is img1
+PASS form["img2"] is img2
+PASS form["img3"] is undefined.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/forms/image/image-named-access.html b/LayoutTests/fast/forms/image/image-named-access.html
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<body>
+<div id="willBeRemoved">
+    <div>
+        <form id="form">
+        <img id="img1">
+    </div>
+    <img id="img2">
+</div>
+<img id="img3">
+<script src="../../../resources/js-test.js"></script>
+<script>
+description('This tests removing an image element with a form element ancestor during parsing');
+const form = document.getElementById('form');
+const img1 = document.getElementById('img1');
+const img2 = document.getElementById('img2');
+const img3 = document.getElementById('img3');
+const willBeRemoved = document.getElementById('willBeRemoved');
+willBeRemoved.remove();
+shouldBe('form["img1"]', 'img1');
+shouldBe('form["img2"]', 'img2');
+shouldBeUndefined('form["img3"]');
+</script>
diff --git a/Source/WebCore/html/HTMLFormElement.cpp b/Source/WebCore/html/HTMLFormElement.cpp
@@ -174,6 +174,11 @@ void HTMLFormElement::removedFromAncestor(RemovalType removalType, ContainerNode
     auto associatedElements = copyAssociatedElementsVector();
     for (auto& associatedElement : associatedElements)
         associatedElement->formOwnerRemovedFromTree(root);
+    auto imageElements = WTF::compactMap(m_imageElements, [](auto& weakPtr) {
+        return RefPtr { weakPtr.get() };
+    });
+    for (auto& imageElement : imageElements)
+        imageElement->formOwnerRemovedFromTree(root);
     HTMLElement::removedFromAncestor(removalType, oldParentOfRemovedTree);
 }
 

diff --git a/Source/WebCore/html/HTMLImageElement.cpp b/Source/WebCore/html/HTMLImageElement.cpp
@@ -102,9 +102,30 @@ Ref<HTMLImageElement> HTMLImageElement::create(const QualifiedName& tagName, Doc
 HTMLImageElement::~HTMLImageElement()
 {
     document().removeDynamicMediaQueryDependentImage(*this);
+    setForm(nullptr);
+}
 
+HTMLFormElement* HTMLImageElement::form() const
+{
+    return m_form.get();
+}
+
+void HTMLImageElement::setForm(HTMLFormElement* newForm)
+{
+    if (m_form == newForm)
+        return;
     if (m_form)
         m_form->removeImgElement(this);
+    m_form = newForm;
+    if (newForm)
+        newForm->registerImgElement(this);
+}
+
+void HTMLImageElement::formOwnerRemovedFromTree(const Node& formRoot)
+{
+    Node& rootNode = traverseToRootNode(); // Do not rely on rootNode() because our IsInTreeScope can be outdated.
+    if (&rootNode != &formRoot)
+        setForm(nullptr);
 }
 
 Ref<HTMLImageElement> HTMLImageElement::createForLegacyFactoryFunction(Document& document, std::optional<unsigned> width, std::optional<unsigned> height)
@@ -433,21 +454,16 @@ void HTMLImageElement::didAttachRenderers()
 Node::InsertedIntoAncestorResult HTMLImageElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
 {
     if (m_formSetByParser) {
-        m_form = WTFMove(m_formSetByParser);
-        m_form->registerImgElement(this);
+        if (m_formSetByParser->isConnected())
+            setForm(m_formSetByParser.get());
+        m_formSetByParser = nullptr;
     }
 
-    if (m_form && rootElement() != m_form->rootElement()) {
-        m_form->removeImgElement(this);
-        m_form = nullptr;
-    }
+    if (m_form && rootElement() != m_form->rootElement())
+        setForm(nullptr);
 
-    if (!m_form) {
-        if (auto* newForm = HTMLFormElement::findClosestFormAncestor(*this)) {
-            m_form = newForm;
-            newForm->registerImgElement(this);
-        }
-    }
+    if (!m_form)
+        setForm(HTMLFormElement::findClosestFormAncestor(*this));
 
     // Insert needs to complete first, before we start updating the loader. Loader dispatches events which could result
     // in callbacks back to this node.
@@ -474,9 +490,6 @@ Node::InsertedIntoAncestorResult HTMLImageElement::insertedIntoAncestor(Insertio
 
 void HTMLImageElement::removedFromAncestor(RemovalType removalType, ContainerNode& oldParentOfRemovedTree)
 {
-    if (m_form)
-        m_form->removeImgElement(this);
-
     if (removalType.treeScopeChanged && !m_parsedUsemap.isNull())
         oldParentOfRemovedTree.treeScope().removeImageElementByUsemap(*m_parsedUsemap.impl(), *this);
 
@@ -486,8 +499,11 @@ void HTMLImageElement::removedFromAncestor(RemovalType removalType, ContainerNod
         selectImageSource(RelevantMutation::Yes);
     }
 
-    m_form = nullptr;
     HTMLElement::removedFromAncestor(removalType, oldParentOfRemovedTree);
+
+    // Do not rely on rootNode() because IsInTreeScope can be outdated.
+    if (m_form && &traverseToRootNode() != &m_form->traverseToRootNode())
+        setForm(HTMLFormElement::findClosestFormAncestor(*this));
 }
 
 HTMLPictureElement* HTMLImageElement::pictureElement() const

diff --git a/Source/WebCore/html/HTMLImageElement.h b/Source/WebCore/html/HTMLImageElement.h
@@ -56,6 +56,8 @@ class HTMLImageElement : public HTMLElement, public FormNamedItem, public Active
 
     virtual ~HTMLImageElement();
 
+    void formOwnerRemovedFromTree(const Node& formRoot);
+
     WEBCORE_EXPORT unsigned width(bool ignorePendingStylesheets = false);
     WEBCORE_EXPORT unsigned height(bool ignorePendingStylesheets = false);
 
@@ -165,6 +167,9 @@ class HTMLImageElement : public HTMLElement, public FormNamedItem, public Active
     void didMoveToNewDocument(Document& oldDocument, Document& newDocument) override;
 
 private:
+    HTMLFormElement* form() const final;
+    void setForm(HTMLFormElement*);
+
     void attributeChanged(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason) final;
     void parseAttribute(const QualifiedName&, const AtomString&) override;
     bool hasPresentationalHintsForAttribute(const QualifiedName&) const override;