From 3151fd8d904f0ac529f3a4f872a9dac537a65dc0 Mon Sep 17 00:00:00 2001
From: Elizabeth Mitchell <lizmitchell@google.com>
Date: Thu, 11 Jan 2024 15:05:48 -0800
Subject: [PATCH] fix(textfield): error styles not removing when an unrelated
 control is invalid

The bug: given a form with two required text fields,
1. Try to submit the form, both fields show error.
2. Add a value to the first field.
3. Try to submit the form, the first field does not remove its error.

This is fixed by listening to form submits and clearing the error state if the control is valid.

I refactored `injectFormReportValidityHooks()` into `addFormReportValidListener()` to keep the `OnReportValidity` class cleaner and better identify the problem we're trying to solve.

PiperOrigin-RevId: 597664564
---
 labs/behaviors/on-report-validity.ts      | 214 ++++++++++++++--------
 labs/behaviors/on-report-validity_test.ts |  54 ++++++
 textfield/demo/stories.ts                 |   1 +
 3 files changed, 193 insertions(+), 76 deletions(-)

diff --git a/labs/behaviors/on-report-validity.ts b/labs/behaviors/on-report-validity.ts
index 92584bbda..137cc3cf9 100644
--- a/labs/behaviors/on-report-validity.ts
+++ b/labs/behaviors/on-report-validity.ts
@@ -168,101 +168,163 @@ export function mixinOnReportValidity<
         super.formAssociatedCallback(form);
       }
 
-      // Clean up previous submit listener
+      // Clean up previous form listeners.
       this[privateCleanupFormListeners].abort();
       if (!form) {
         return;
       }
 
       this[privateCleanupFormListeners] = new AbortController();
-      // If the element's form submits, then all controls are valid. This lets
-      // the element remove its error styles that may have been set when
-      // `reportValidity()` was called.
-      form.addEventListener(
-        'submit',
+
+      // Add a listener that fires when the form runs constraint validation and
+      // the control is valid, so that it may remove its error styles.
+      //
+      // This happens on `form.reportValidity()` and `form.requestSubmit()`
+      // (both when the submit fails and passes).
+      addFormReportValidListener(
+        this,
+        form,
         () => {
           this[onReportValidity](null);
         },
-        {
-          signal: this[privateCleanupFormListeners].signal,
-        },
+        this[privateCleanupFormListeners].signal,
       );
-
-      // Inject a callback when `form.reportValidity()` is called and the form
-      // is valid. There isn't an event that is dispatched to alert us (unlike
-      // the 'invalid' event), and we need to remove error styles when
-      // `form.reportValidity()` is called and returns true.
-      let reportedInvalidEventFromForm = false;
-      let formReportValidityCleanup = new AbortController();
-      injectFormReportValidityHooks({
-        form,
-        cleanup: this[privateCleanupFormListeners].signal,
-        beforeReportValidity: () => {
-          reportedInvalidEventFromForm = false;
-          this.addEventListener(
-            'invalid',
-            () => {
-              reportedInvalidEventFromForm = true;
-              // Constructor's invalid listener will handle reporting invalid
-              // events.
-            },
-            {signal: formReportValidityCleanup.signal},
-          );
-        },
-        afterReportValidity: () => {
-          formReportValidityCleanup.abort();
-          formReportValidityCleanup = new AbortController();
-          if (reportedInvalidEventFromForm) {
-            reportedInvalidEventFromForm = false;
-            return;
-          }
-
-          // Report successful form validation if an invalid event wasn't
-          // fired.
-          this[onReportValidity](null);
-        },
-      });
     }
   }
 
   return OnReportValidityElement;
 }
 
-const FORM_REPORT_VALIDITY_HOOKS = new WeakMap<HTMLFormElement, EventTarget>();
+/**
+ * Add a listener that fires when a form runs constraint validation on a control
+ * and it is valid. This is needed to clear previously invalid styles.
+ *
+ * @param control The control of the form to listen for valid events.
+ * @param form The control's form that can run constraint validation.
+ * @param onControlValid A listener that is called when the form runs constraint
+ *     validation and the control is valid.
+ * @param cleanup A cleanup signal to remove the listener.
+ */
+function addFormReportValidListener(
+  control: Element,
+  form: HTMLFormElement,
+  onControlValid: () => void,
+  cleanup: AbortSignal,
+) {
+  const validateHooks = getFormValidateHooks(form);
 
-function injectFormReportValidityHooks({
-  form,
-  beforeReportValidity,
-  afterReportValidity,
-  cleanup,
-}: {
-  form: HTMLFormElement;
-  beforeReportValidity: () => void;
-  afterReportValidity: () => void;
-  cleanup: AbortSignal;
-}) {
-  if (!FORM_REPORT_VALIDITY_HOOKS.has(form)) {
-    // Patch form.reportValidity() to add an event target that can be used to
-    // react when the method is called.
-    // We should only patch this method once, since multiple controls and other
-    // forces may want to patch this method. We cannot reliably clean it up by
-    // resetting the method to "superReportValidity", which may be a patched
-    // function.
-    // Instead, we never clean up the patch but add and clean up event listener
-    // hooks once it's patched.
+  // When a form validates its controls, check if an invalid event is dispatched
+  // on the control. If it is not, then inform the control to report its valid
+  // state.
+  let controlFiredInvalid = false;
+  let cleanupInvalidListener: AbortController | undefined;
+  let isNextSubmitFromHook = false;
+  validateHooks.addEventListener(
+    'before',
+    () => {
+      isNextSubmitFromHook = true;
+      cleanupInvalidListener = new AbortController();
+      controlFiredInvalid = false;
+      control.addEventListener(
+        'invalid',
+        () => {
+          controlFiredInvalid = true;
+        },
+        {
+          signal: cleanupInvalidListener.signal,
+        },
+      );
+    },
+    {signal: cleanup},
+  );
+
+  validateHooks.addEventListener(
+    'after',
+    () => {
+      isNextSubmitFromHook = false;
+      cleanupInvalidListener?.abort();
+      if (controlFiredInvalid) {
+        return;
+      }
+
+      onControlValid();
+    },
+    {signal: cleanup},
+  );
+
+  // The above hooks handle imperatively submitting the form, but not
+  // declaratively submitting the form. This happens when:
+  // 1. A non-custom element `<button type="submit">` is clicked.
+  // 2. Enter is pressed on a non-custom element text editable `<input>`.
+  form.addEventListener(
+    'submit',
+    () => {
+      // This submit was from `form.requestSubmit()`, which already calls the
+      // listener.
+      if (isNextSubmitFromHook) {
+        return;
+      }
+
+      onControlValid();
+    },
+    {
+      signal: cleanup,
+    },
+  );
+
+  // Note: it is a known limitation that we cannot detect if a form tries to
+  // submit declaratively, but fails to do so because an unrelated sibling
+  // control failed its constraint validation.
+  //
+  // Since we cannot detect when that happens, a previously invalid control may
+  // not clear its error styling when it becomes valid again.
+  //
+  // To work around this, call `form.reportValidity()` when submitting a form
+  // declaratively. This can be down on the `<button type="submit">`'s click or
+  // the text editable `<input>`'s 'Enter' keydown.
+}
+
+const FORM_VALIDATE_HOOKS = new WeakMap<HTMLFormElement, EventTarget>();
+
+/**
+ * Get a hooks `EventTarget` that dispatches 'before' and 'after' events that
+ * fire before a form runs constraint validation and immediately after it
+ * finishes running constraint validation on its controls.
+ *
+ * This happens during `form.reportValidity()` and `form.requestSubmit()`.
+ *
+ * @param form The form to get or set up hooks for.
+ * @return A hooks `EventTarget` to add listeners to.
+ */
+function getFormValidateHooks(form: HTMLFormElement) {
+  if (!FORM_VALIDATE_HOOKS.has(form)) {
+    // Patch form methods to add event listener hooks. These are needed to react
+    // to form behaviors that do not dispatch events, such as a form asking its
+    // controls to report their validity.
+    //
+    // We should only patch the methods once, since multiple controls and other
+    // forces may want to patch this method. We cannot reliably clean it up if
+    // there are multiple patched and re-patched methods referring holding
+    // references to each other.
+    //
+    // Instead, we never clean up the patch but add and clean up event listeners
+    // added to the hooks after the patch.
     const hooks = new EventTarget();
-    const superReportValidity = form.reportValidity;
-    form.reportValidity = function (this: HTMLFormElement) {
-      hooks.dispatchEvent(new Event('before'));
-      const valid = superReportValidity.call(this);
-      hooks.dispatchEvent(new Event('after'));
-      return valid;
-    };
+    FORM_VALIDATE_HOOKS.set(form, hooks);
 
-    FORM_REPORT_VALIDITY_HOOKS.set(form, hooks);
+    // Add hooks to support notifying before and after a form has run constraint
+    // validation on its controls.
+    // Note: `form.submit()` does not run constraint validation per spec.
+    for (const methodName of ['reportValidity', 'requestSubmit'] as const) {
+      const superMethod = form[methodName];
+      form[methodName] = function (this: HTMLFormElement) {
+        hooks.dispatchEvent(new Event('before'));
+        const result = Reflect.apply(superMethod, this, arguments);
+        hooks.dispatchEvent(new Event('after'));
+        return result;
+      };
+    }
   }
 
-  const hooks = FORM_REPORT_VALIDITY_HOOKS.get(form)!;
-  hooks.addEventListener('before', beforeReportValidity, {signal: cleanup});
-  hooks.addEventListener('after', afterReportValidity, {signal: cleanup});
+  return FORM_VALIDATE_HOOKS.get(form)!;
 }
diff --git a/labs/behaviors/on-report-validity_test.ts b/labs/behaviors/on-report-validity_test.ts
index 4d319bceb..118a3ba2e 100644
--- a/labs/behaviors/on-report-validity_test.ts
+++ b/labs/behaviors/on-report-validity_test.ts
@@ -158,6 +158,17 @@ describe('mixinOnReportValidity()', () => {
           form.remove();
           expect(control[onReportValidity]).toHaveBeenCalledOnceWith(null);
         });
+
+        it('should be called with null when form submits declaratively and it is valid, but another sibling is invalid', () => {
+          // This is a known limitation. If a form is using an MWC control and
+          // declaratively submits it with a native `<button>` or `<input>`, then
+          // error styles will not clear if the form fails to submit.
+          // The workaround is to call `form.reportValidity()` when clicking the
+          // native `<button type="submit">` or pressing enter in an `<input>`.
+          //
+          // Leaving this test here for documentation and a possible future fix.
+          expect().nothing();
+        });
       });
 
       describe('for valid to invalid controls', () => {
@@ -348,6 +359,44 @@ describe('mixinOnReportValidity()', () => {
           expect(control[onReportValidity]).toHaveBeenCalledOnceWith(null);
         });
 
+        it('should be called with null when form.requestSubmit() is called after fixing invalid, but another sibling is invalid', () => {
+          const control = new TestOnReportValidity();
+          const onReportValiditySpy = jasmine.createSpy('onReportValidity');
+          control[onReportValidity] = onReportValiditySpy;
+          const form = document.createElement('form');
+          form.appendChild(control);
+
+          const invalidSibling = document.createElement('input');
+          invalidSibling.required = true;
+          form.appendChild(invalidSibling);
+
+          document.body.appendChild(form);
+          form.addEventListener(
+            'submit',
+            (event) => {
+              // Prevent the test page from actually reloading.
+              event.preventDefault();
+            },
+            {capture: true},
+          );
+
+          control.required = true;
+          form.reportValidity();
+          onReportValiditySpy.calls.reset();
+
+          // Fix invalid
+          control.checked = true;
+
+          // Submit imperatively
+          form.requestSubmit();
+          form.remove();
+
+          expect(invalidSibling.validity.valid)
+            .withContext('sibling is invalid')
+            .toBeFalse();
+          expect(control[onReportValidity]).toHaveBeenCalledWith(null);
+        });
+
         it('should be called with null when form submits declaratively after fixing invalid', () => {
           const control = new TestOnReportValidity();
           const onReportValiditySpy = jasmine.createSpy('onReportValidity');
@@ -379,6 +428,11 @@ describe('mixinOnReportValidity()', () => {
 
           expect(control[onReportValidity]).toHaveBeenCalledOnceWith(null);
         });
+
+        it('should be called with null when form submits declaratively after fixing invalid, but another sibling is invalid', () => {
+          // See above "This is a known limitation" for explanation.
+          expect().nothing();
+        });
       });
 
       it('should clean up when form is unassociated and not call when non-parent form.reportValidity() is called', () => {
diff --git a/textfield/demo/stories.ts b/textfield/demo/stories.ts
index 2dad110c0..6785f33e1 100644
--- a/textfield/demo/stories.ts
+++ b/textfield/demo/stories.ts
@@ -216,6 +216,7 @@ const forms: MaterialStoryInit<StoryKnobs> = {
             ?disabled=${knobs.disabled}
             label="Last name"
             name="last-name"
+            required
             autocomplete="family-name"></md-filled-text-field>
         </div>
         <div class="row buttons">