From f015dc9169aa64ac32f2d3207d4ca1bfeb3ded25 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Tue, 7 Nov 2023 10:28:01 -0500 Subject: [PATCH 1/4] 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 ``. The `` 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, `` 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 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(`
`); + 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(`
`); + 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 From 52214198160544212e7a3c61ee345f6de536a804 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Tue, 7 Nov 2023 10:39:57 -0500 Subject: [PATCH 2/4] Fix form controls entering / leaving a form (#1708) * fix dynamic form controls * update comment * add form.checkValidity() * prettier --- docs/pages/resources/changelog.md | 2 + src/internal/form.test.ts | 21 +++++++++ src/internal/form.ts | 72 ++++++++++++++++++++++++++++--- 3 files changed, 90 insertions(+), 5 deletions(-) create mode 100644 src/internal/form.test.ts diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 9714628b..f8e84706 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -14,6 +14,8 @@ 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 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 ``. The `` event now only fires when a user stops dragging a slider or stops dragging on the color canvas. [#1689] diff --git a/src/internal/form.test.ts b/src/internal/form.test.ts new file mode 100644 index 00000000..3ce21e2e --- /dev/null +++ b/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(html` +
+ + +
+ `); + + 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); +}); diff --git a/src/internal/form.ts b/src/internal/form.ts index b981b25d..1533f31e 100644 --- a/src/internal/form.ts +++ b/src/internal/form.ts @@ -14,6 +14,7 @@ export const formCollections: WeakMap> // restore the original behavior when they disconnect. // const reportValidityOverloads: WeakMap boolean> = new WeakMap(); +const checkValidityOverloads: WeakMap 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; /** @@ -81,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 @@ -150,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); @@ -169,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) => { @@ -230,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('*'); + + 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 From e786aa86b58de2cf7c334d4d4e31d2ca52fa067f Mon Sep 17 00:00:00 2001 From: Coridyn Date: Tue, 14 Nov 2023 02:09:11 +1000 Subject: [PATCH 3/4] Fix React .d.ts files to import from valid path; fixes #1713 (#1714) --- scripts/make-react.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/make-react.js b/scripts/make-react.js index d8c2b0ee..f016669c 100644 --- a/scripts/make-react.js +++ b/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 || []) From 35c2ad886db6b7b227d0ad4dccb08eb5860e3b1b Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Mon, 13 Nov 2023 14:13:42 -0500 Subject: [PATCH 4/4] Fix nested dialogs (#1711) * fix nested dialog focus * fix nested dialog focus * fix nested dialog focus * prettier * remove index.html * fix tests * prettier --- docs/pages/resources/changelog.md | 1 + src/internal/modal.ts | 2 ++ src/internal/tabbable.test.ts | 47 +++++++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index f8e84706..54802890 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -15,6 +15,7 @@ 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] diff --git a/src/internal/modal.ts b/src/internal/modal.ts index 9c507d90..c61a7276 100644 --- a/src/internal/modal.ts +++ b/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'; diff --git a/src/internal/tabbable.test.ts b/src/internal/tabbable.test.ts index c6705ffc..5a1b8d4d 100644 --- a/src/internal/tabbable.test.ts +++ b/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) { 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` +
+ dialogOne().show()}> + + dialogTwo().show()} id="open-dialog-2">Open Dialog 2 + Close + + + + + + Close + +
+ `); + /* 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); +});