Merge branch 'next' of https://github.com/shoelace-style/shoelace into next

docs/pages/resources/changelog.md

@@ -14,6 +14,10 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 ## Next
 
+- Added the ability to call `form.checkValidity()` and it will use Shoelace's custom `checkValidity()` handler. [#1708]
+- Fixed a bug where nested dialogs were not properly trapping focus. [#1711]
+- Fixed a bug with form controls removing the custom validity handlers from the form. [#1708]
+- 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]

scripts/make-react.js

@@ -25,10 +25,10 @@ for await (const component of components) {
   const componentFile = path.join(componentDir, 'index.ts');
   const importPath = component.path.replace(/\.js$/, '.component.js');
   const eventImports = (component.events || [])
-    .map(event => `import type { ${event.eventName} } from '../../../src/events/events';`)
+    .map(event => `import type { ${event.eventName} } from '../../events/events';`)
     .join('\n');
   const eventExports = (component.events || [])
-    .map(event => `export type { ${event.eventName} } from '../../../src/events/events';`)
+    .map(event => `export type { ${event.eventName} } from '../../events/events';`)
     .join('\n');
   const eventNameImport = (component.events || []).length > 0 ? `import { type EventName } from '@lit/react';` : ``;
   const events = (component.events || [])

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);
 

src/internal/form.test.ts

@@ -0,0 +1,21 @@
+import '../../../dist/shoelace.js';
+
+import { expect, fixture, html } from '@open-wc/testing';
+
+// Reproduction of this issue: https://github.com/shoelace-style/shoelace/issues/1703
+it('Should still run form validations if an element is removed', async () => {
+  const form = await fixture<HTMLFormElement>(html`
+    <form>
+      <sl-input name="name" label="Name" required></sl-input>
+      <sl-textarea name="comment" label="Comment" required></sl-textarea>
+    </form>
+  `);
+
+  expect(form.checkValidity()).to.equal(false);
+  expect(form.reportValidity()).to.equal(false);
+
+  form.querySelector('sl-input')!.remove();
+
+  expect(form.checkValidity()).to.equal(false);
+  expect(form.reportValidity()).to.equal(false);
+});

src/internal/form.ts

@@ -14,6 +14,7 @@ export const formCollections: WeakMap<HTMLFormElement, Set<ShoelaceFormControl>>
 // restore the original behavior when they disconnect.
 //
 const reportValidityOverloads: WeakMap<HTMLFormElement, () => boolean> = new WeakMap();
+const checkValidityOverloads: WeakMap<HTMLFormElement, () => boolean> = new WeakMap();
 
 //
 // We store a Set of controls that users have interacted with. This allows us to determine the interaction state
@@ -42,6 +43,12 @@ export interface FormControlControllerOptions {
    * prevent submission and trigger the browser's constraint violation warning.
    */
   reportValidity: (input: ShoelaceFormControl) => boolean;
+
+  /**
+   * A function that maps to the form control's `checkValidity()` function. When the control is invalid, this will return false.
+   *   this is helpful is you want to check validation without triggering the native browser constraint violation warning.
+   */
+  checkValidity: (input: ShoelaceFormControl) => boolean;
   /** A function that sets the form control's value */
   setValue: (input: ShoelaceFormControl, value: unknown) => void;
   /**
@@ -61,12 +68,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;
           }
         }
 
@@ -77,6 +88,7 @@ export class FormControlController implements ReactiveController {
       defaultValue: input => input.defaultValue,
       disabled: input => input.disabled ?? false,
       reportValidity: input => (typeof input.reportValidity === 'function' ? input.reportValidity() : true),
+      checkValidity: input => (typeof input.checkValidity === 'function' ? input.checkValidity() : true),
       setValue: (input, value: string) => (input.value = value),
       assumeInteractionOn: ['sl-input'],
       ...options
@@ -146,16 +158,34 @@ export class FormControlController implements ReactiveController {
         reportValidityOverloads.set(this.form, this.form.reportValidity);
         this.form.reportValidity = () => this.reportFormValidity();
       }
+
+      // Overload the form's checkValidity() method so it looks at Shoelace form controls
+      if (!checkValidityOverloads.has(this.form)) {
+        checkValidityOverloads.set(this.form, this.form.checkValidity);
+        this.form.checkValidity = () => this.checkFormValidity();
+      }
     } else {
       this.form = undefined;
     }
   }
 
   private detachForm() {
-    if (this.form) {
-      // Remove this element from the form's collection
-      formCollections.get(this.form)?.delete(this.host);
+    if (!this.form) return;
 
+    const formCollection = formCollections.get(this.form);
+
+    if (!formCollection) {
+      return;
+    }
+
+    // Remove this host from the form's collection
+    formCollection.delete(this.host);
+
+    // Check to make sure there's no other form controls in the collection. If we do this
+    // without checking if any other controls are still in the collection, then we will wipe out the
+    // validity checks for all other elements.
+    // see: https://github.com/shoelace-style/shoelace/issues/1703
+    if (formCollection.size <= 0) {
       this.form.removeEventListener('formdata', this.handleFormData);
       this.form.removeEventListener('submit', this.handleFormSubmit);
       this.form.removeEventListener('reset', this.handleFormReset);
@@ -165,9 +195,17 @@ export class FormControlController implements ReactiveController {
         this.form.reportValidity = reportValidityOverloads.get(this.form)!;
         reportValidityOverloads.delete(this.form);
       }
-    }
 
-    this.form = undefined;
+      if (checkValidityOverloads.has(this.form)) {
+        this.form.checkValidity = checkValidityOverloads.get(this.form)!;
+        checkValidityOverloads.delete(this.form);
+      }
+
+      // So it looks weird here to not always set the form to undefined. But I _think_ if we unattach this.form here,
+      // we end up in this fun spot where future validity checks don't have a reference to the form validity handler.
+      // First form element in sets the validity handler. So we can't clean up `this.form` until there are no other form elements in the form.
+      this.form = undefined;
+    }
   }
 
   private handleFormData = (event: FormDataEvent) => {
@@ -226,6 +264,34 @@ export class FormControlController implements ReactiveController {
     }
   };
 
+  private checkFormValidity = () => {
+    //
+    // This is very similar to the `reportFormValidity` function, but it does not trigger native constraint validation
+    // Allow the user to simply check if the form is valid and handling validity in their own way.
+    //
+    // We preserve the original method in a WeakMap, but we don't call it from the overload because that would trigger
+    // validations in an unexpected order. When the element disconnects, we revert to the original behavior. This won't
+    // be necessary once we can use ElementInternals.
+    //
+    // Note that we're also honoring the form's novalidate attribute.
+    //
+    if (this.form && !this.form.noValidate) {
+      // This seems sloppy, but checking all elements will cover native inputs, Shoelace inputs, and other custom
+      // elements that support the constraint validation API.
+      const elements = this.form.querySelectorAll<HTMLInputElement>('*');
+
+      for (const element of elements) {
+        if (typeof element.checkValidity === 'function') {
+          if (!element.checkValidity()) {
+            return false;
+          }
+        }
+      }
+    }
+
+    return true;
+  };
+
   private reportFormValidity = () => {
     //
     // Shoelace form controls work hard to act like regular form controls. They support the Constraint Validation API

src/internal/modal.ts

@@ -63,11 +63,13 @@ export default class Modal {
   }
 
   private handleFocusIn = () => {
+    if (!this.isActive()) return;
     this.checkFocus();
   };
 
   private handleKeyDown = (event: KeyboardEvent) => {
     if (event.key !== 'Tab' || this.isExternalActivated) return;
+    if (!this.isActive()) return;
 
     if (event.shiftKey) {
       this.tabDirection = 'backward';

src/internal/tabbable.test.ts

@@ -1,9 +1,12 @@
-import { elementUpdated, expect, fixture } from '@open-wc/testing';
+import { aTimeout, elementUpdated, expect, fixture } from '@open-wc/testing';
 
-import '../../dist/shoelace.js';
 import { activeElements, getDeepestActiveElement } from './active-elements.js';
+import { clickOnElement } from './test.js';
 import { html } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
+import type { SlDialog } from '../shoelace.js';
+
+import '../../../dist/shoelace.js';
 
 async function holdShiftKey(callback: () => Promise<void>) {
   await sendKeys({ down: 'Shift' });
@@ -174,3 +177,43 @@ it('Should account for when focus is changed from outside sources (like clicking
   await holdShiftKey(async () => await sendKeys({ press: tabKey }));
   expect(activeElementsArray()).to.include(closeButton);
 });
+
+// https://github.com/shoelace-style/shoelace/issues/1710
+it('Should respect nested modal instances', async () => {
+  const dialogOne = (): SlDialog => document.querySelector('#dialog-1')!;
+  const dialogTwo = (): SlDialog => document.querySelector('#dialog-2')!;
+
+  // lit-a11y doesn't like the "autofocus" attribute.
+  /* eslint-disable */
+  await fixture(html`
+    <div>
+      <sl-button id="open-dialog-1" @click=${() => dialogOne().show()}></sl-button>
+      <sl-dialog id="dialog-1" label="Dialog 1">
+        <sl-button @click=${() => dialogTwo().show()} id="open-dialog-2">Open Dialog 2</sl-button>
+        <sl-button slot="footer" variant="primary">Close</sl-button>
+      </sl-dialog>
+
+      <sl-dialog id="dialog-2" label="Dialog 2">
+        <sl-input id="focus-1" autofocus="" placeholder="I will have focus when the dialog is opened"></sl-input>
+        <sl-input id="focus-2" placeholder="Second input"></sl-input>
+        <sl-button slot="footer" variant="primary" class="close-2">Close</sl-button>
+      </sl-dialog>
+    </div>
+  `);
+  /* eslint-enable */
+
+  const firstFocusedEl = document.querySelector('#focus-1');
+  const secondFocusedEl = document.querySelector('#focus-2');
+
+  // So we can trigger auto-focus stuff
+  await clickOnElement(document.querySelector('#open-dialog-1')!);
+  // These clicks need a ~100ms timeout. I'm assuming for animation reasons?
+  await aTimeout(100);
+  await clickOnElement(document.querySelector('#open-dialog-2')!);
+  await aTimeout(100);
+
+  expect(activeElementsArray()).to.include(firstFocusedEl);
+
+  await sendKeys({ press: tabKey });
+  expect(activeElementsArray()).to.include(secondFocusedEl);
+});

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