From f015dc9169aa64ac32f2d3207d4ca1bfeb3ded25 Mon Sep 17 00:00:00 2001
From: Konnor Rogers <konnor5456@gmail.com>
Date: Tue, 7 Nov 2023 10:28:01 -0500
Subject: [PATCH] fix form controls to read from property instead of attribute
 (#1707)

* fix form controls to read from properties and attributes

* update changelog

* prettier

* update changelog

* prettier

* small comment fix
---
 docs/pages/resources/changelog.md            |  1 +
 src/components/button/button.component.ts    | 13 +-----------
 src/internal/form.ts                         | 14 ++++++++-----
 src/internal/test/form-control-base-tests.ts | 22 ++++++++++++++++++++
 4 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index c2909cbb..9714628b 100644
--- a/docs/pages/resources/changelog.md
+++ b/docs/pages/resources/changelog.md
@@ -14,6 +14,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 ## Next
 
+- Fixed a bug in form control components that used a `form` property, but not an attribute. [#1707]
 - Fixed a bug with bundled components using CDN builds not having translations on initial connect [#1696]
 - Fixed a bug where the `"sl-change"` event would always fire simultaneously with `"sl-input"` event in `<sl-color-picker>`. The `<sl-change>` event now only fires when a user stops dragging a slider or stops dragging on the color canvas. [#1689]
 - Updated the copy icon in the system library [#1702]
diff --git a/src/components/button/button.component.ts b/src/components/button/button.component.ts
index 2d2ec653..262dcd72 100644
--- a/src/components/button/button.component.ts
+++ b/src/components/button/button.component.ts
@@ -45,20 +45,9 @@ export default class SlButton extends ShoelaceElement implements ShoelaceFormCon
   };
 
   private readonly formControlController = new FormControlController(this, {
-    form: input => {
-      // Buttons support a form attribute that points to an arbitrary form, so if this attribute is set we need to query
-      // the form from the same root using its id
-      if (input.hasAttribute('form')) {
-        const doc = input.getRootNode() as Document | ShadowRoot;
-        const formId = input.getAttribute('form')!;
-        return doc.getElementById(formId) as HTMLFormElement;
-      }
-
-      // Fall back to the closest containing form
-      return input.closest('form');
-    },
     assumeInteractionOn: ['click']
   });
+
   private readonly hasSlotController = new HasSlotController(this, '[default]', 'prefix', 'suffix');
   private readonly localize = new LocalizeController(this);
 
diff --git a/src/internal/form.ts b/src/internal/form.ts
index e19b2f94..b981b25d 100644
--- a/src/internal/form.ts
+++ b/src/internal/form.ts
@@ -61,12 +61,16 @@ export class FormControlController implements ReactiveController {
     this.options = {
       form: input => {
         // If there's a form attribute, use it to find the target form by id
-        if (input.hasAttribute('form') && input.getAttribute('form') !== '') {
-          const root = input.getRootNode() as Document | ShadowRoot;
-          const formId = input.getAttribute('form');
+        // Controls may not always reflect the 'form' property. For example, `<sl-button>` doesn't reflect.
+        const formId = input.form;
 
-          if (formId) {
-            return root.getElementById(formId) as HTMLFormElement;
+        if (formId) {
+          const root = input.getRootNode() as Document | ShadowRoot;
+
+          const form = root.getElementById(formId);
+
+          if (form) {
+            return form as HTMLFormElement;
           }
         }
 
diff --git a/src/internal/test/form-control-base-tests.ts b/src/internal/test/form-control-base-tests.ts
index b503974c..34be5a6d 100644
--- a/src/internal/test/form-control-base-tests.ts
+++ b/src/internal/test/form-control-base-tests.ts
@@ -44,6 +44,7 @@ export function runFormControlBaseTests<T extends ShoelaceFormControl = Shoelace
 //   - `.checkValidity()`
 //   - `.reportValidity()`
 //   - `.setCustomValidity(msg)`
+//   - `.getForm()`
 //
 function runAllValidityTests(
   tagName: string, //
@@ -124,6 +125,27 @@ function runAllValidityTests(
         const emittedEvents = checkEventEmissions(control, 'sl-invalid', () => control.reportValidity());
         expect(emittedEvents.length).to.equal(0);
       });
+
+      it('Should find the correct form when given a form property', async () => {
+        const formId = 'test-form';
+        const form = await fixture(`<form id='${formId}'></form>`);
+        const control = await createControl();
+        expect(control.getForm()).to.equal(null);
+        control.form = 'test-form';
+        await control.updateComplete;
+        expect(control.getForm()).to.equal(form);
+      });
+
+      it('Should find the correct form when given a form attribute', async () => {
+        const formId = 'test-form';
+        const form = await fixture(`<form id='${formId}'></form>`);
+        const control = await createControl();
+        expect(control.getForm()).to.equal(null);
+        control.setAttribute('form', 'test-form');
+
+        await control.updateComplete;
+        expect(control.getForm()).to.equal(form);
+      });
     }
 
     // Run special tests depending on component type