Various (message) dialog fixes (#1379)

.changeset/beige-elephants-perform.md

@@ -0,0 +1,8 @@
+---
+'@sl-design-system/message-dialog': patch
+---
+
+Various improvements:
+- Add `autofocus` attribute to `MessageDialogButton` config
+- Per the documentation, cancel buttons must use `fill="outline"` and `variant="primary"`
+- Improve API documentation

.changeset/unlucky-parrots-confess.md

@@ -0,0 +1,7 @@
+---
+'@sl-design-system/magister': patch
+'@sl-design-system/neon': patch
+'@sl-design-system/sanoma-learning': patch
+---
+
+Remove breaking extra whitespace in certain color tokens

.changeset/violet-jars-sit.md

@@ -0,0 +1,8 @@
+---
+'@sl-design-system/dialog': patch
+---
+
+Fix various issues:
+- Add focus outline for `<dialog>`; see https://adrianroselli.com/2020/10/dialog-focus-in-screen-readers.html#Update01
+- Add workaround for dialog focus behavior in combination with slots; see https://github.com/whatwg/html/issues/9245
+- Improve API documentation

packages/components/dialog/src/dialog.scss

@@ -6,6 +6,8 @@ $md-breakpoint: 600px;
   --_border-radius: var(--sl-border-radius-dialog-mobile);
   --_border: var(--sl-border-width-dialog-default) solid var(--sl-color-dialog-border);
   --_color: var(--sl-color-dialog-foreground);
+  --_focus-outline: var(--sl-color-focusring-default) solid var(--sl-border-width-focusring-default);
+  --_focus-outline-offset: calc(var(--sl-border-width-focusring-offset) * -2);
   --_header-gap: var(--sl-space-dialog-gap-header);
   --_heading-direction: var(--sl-dialog-header-flex-direction);
   --_heading-gap: var(--sl-space-dialog-gap-header);
@@ -39,6 +41,11 @@ dialog {
   opacity: 0;
   overflow: visible;
   padding: 0;
+
+  &:focus-visible {
+    outline: var(--_focus-outline);
+    outline-offset: var(--_focus-outline-offset);
+  }
 }
 
 dialog,

packages/components/dialog/src/dialog.spec.ts

@@ -41,8 +41,8 @@ describe('sl-dialog', () => {
       expect(dialog.querySelector('sl-button[aria-label="Close"]')).to.exist;
     });
 
-    it('should have a role of dialog', () => {
-      expect(dialog).to.have.attribute('role', 'dialog');
+    it('should not have a role of dialog', () => {
+      expect(dialog).not.to.have.attribute('role');
     });
 
     it('should have a dialog part', () => {

packages/components/dialog/src/dialog.stories.ts

@@ -93,7 +93,7 @@ export const All: Story = {
         <span slot="subtitle">Subtitle</span>
         Body text
         <sl-button slot="actions" fill="ghost" variant="default" sl-dialog-close autofocus>Cancel</sl-button>
-        <sl-button slot="actions" fill="solid" variant="primary" sl-dialog-close>Action</sl-button>
+        <sl-button slot="actions" variant="primary" sl-dialog-close>Action</sl-button>
       </sl-dialog>`;
   }
 };
@@ -113,7 +113,7 @@ export const FooterButtons: Story = {
     footerButtons: () => html`
       <sl-button fill="ghost" slot="actions" variant="default" sl-dialog-close autofocus>Cancel</sl-button>
       <sl-button fill="outline" slot="actions" variant="primary" sl-dialog-close>Action 2</sl-button>
-      <sl-button fill="solid" slot="actions" variant="primary" sl-dialog-close>Action</sl-button>
+      <sl-button slot="actions" variant="primary" sl-dialog-close>Action</sl-button>
     `,
     reverse: false,
     title: 'Dialog with extra footer buttons'

packages/components/dialog/src/dialog.ts

@@ -15,6 +15,7 @@ import {
   unsafeCSS
 } from 'lit';
 import { property, query } from 'lit/decorators.js';
+import { ifDefined } from 'lit/directives/if-defined.js';
 import styles from './dialog.scss.js';
 
 declare global {
@@ -103,7 +104,7 @@ export class Dialog extends ScopedElementsMixin(LitElement) {
         @click=${this.#onClick}
         @close=${this.#onClose}
         aria-labelledby="title"
-        role=${this.dialogRole}
+        role=${ifDefined(this.dialogRole === 'dialog' ? undefined : this.dialogRole)}
         part="dialog"
       >
         <div part="header">
@@ -140,6 +141,7 @@ export class Dialog extends ScopedElementsMixin(LitElement) {
     `;
   }
 
+  /** Show the dialog as a modal, in the top layer, with a backdrop. */
   showModal(): void {
     if (this.dialog?.open) {
       return;
@@ -165,8 +167,19 @@ export class Dialog extends ScopedElementsMixin(LitElement) {
 
     this.inert = false;
     this.dialog?.showModal();
+
+    // Workaround for broken focus behavior when using <slot> inside <dialog>
+    // See https://github.com/whatwg/html/issues/9245
+    requestAnimationFrame(() => {
+      const focusable = this.querySelector<HTMLElement>('[autofocus], [tabindex]:not([tabindex="-1"])');
+
+      if (focusable && this.shadowRoot?.activeElement !== focusable) {
+        focusable.focus();
+      }
+    });
   }
 
+  /** Close the dialog. */
   close(): void {
     if (this.dialog?.open) {
       this.#closeDialogOnAnimationend();

packages/components/message-dialog/src/message-dialog.spec.ts

@@ -153,7 +153,7 @@ describe('sl-message-dialog', () => {
 
       promise.then(callback);
 
-      dialog.querySelector<HTMLElement>('sl-button[variant="primary"]')?.click();
+      dialog.querySelector<HTMLElement>('sl-button:last-of-type')?.click();
       await new Promise(resolve => setTimeout(resolve));
 
       expect(callback).to.have.been.calledWith(true);
@@ -182,7 +182,7 @@ describe('sl-message-dialog', () => {
         subtitle: 'Subtitle',
         message: html`This is a message with <strong>HTML</strong>!`,
         buttons: [
-          { text: 'No, run away!', fill: 'outline', value: 'NO' },
+          { text: 'No, run away!', fill: 'outline', value: 'NO', variant: 'primary' },
           { text: "Yes, I don't care what it does", value: 'YES', variant: 'danger' }
         ],
         disableCancel: true
@@ -219,7 +219,7 @@ describe('sl-message-dialog', () => {
       expect(buttons).to.have.length(2);
       expect(buttons[0]).to.have.trimmed.text('No, run away!');
       expect(buttons[0]).to.have.attribute('fill', 'outline');
-      expect(buttons[0]).to.have.attribute('variant', 'default');
+      expect(buttons[0]).to.have.attribute('variant', 'primary');
       expect(buttons[1]).to.have.trimmed.text("Yes, I don't care what it does");
       expect(buttons[1]).to.have.attribute('variant', 'danger');
     });

packages/components/message-dialog/src/message-dialog.ts

@@ -22,18 +22,25 @@ export interface MessageDialogConfig<T = unknown> {
 
 export interface MessageDialogButton<T = unknown> {
   action?(): void;
+  autofocus?: boolean;
   fill?: ButtonFill;
   text: string;
   value?: T;
   variant?: ButtonVariant;
 }
 
 /**
- * A dialog for displaying messages to the user.
+ * Use this component to show alerts, confirmations, or custom dialogs.
+ *
+ * This component is meant to be used as a static class. Not as a declarative component. For example:
+ * ```js
+ * await MessageDialog.alert('Hello, world!');
+ * // Dialog has been closed or cancelled at this point
+ * ```
  */
 @localized()
 export class MessageDialog<T = unknown> extends ScopedElementsMixin(LitElement) {
-  /** @private */
+  /** @internal */
   static get scopedElements(): ScopedElementsMap {
     return {
       ...Dialog.scopedElements,
@@ -42,28 +49,47 @@ export class MessageDialog<T = unknown> extends ScopedElementsMixin(LitElement)
     };
   }
 
-  /** @private */
+  /** @internal */
   static override styles: CSSResultGroup = styles;
 
+  /**
+   * Shows an alert message to the user with an OK button by default.
+   *
+   * @param message - The message to display.
+   * @param title - The title of the dialog.
+   */
   static async alert(message: string, title = msg('Alert')): Promise<void> {
     return await this.show({
-      buttons: [{ text: msg('OK'), variant: 'primary' }],
+      buttons: [{ autofocus: true, text: msg('OK'), variant: 'primary' }],
       title,
       message
     });
   }
 
+  /**
+   * Shows a confirmation dialog to the user with OK and Cancel buttons by default.
+   *
+   * @param message - The message to display.
+   * @param title - The title of the dialog.
+   * @returns A promise that resolves with `true` if the user clicks OK, `false` if the user clicks Cancel, or `undefined` if the user closes the dialog.
+   */
   static async confirm(message: string, title = msg('Confirm')): Promise<boolean | undefined> {
     return await this.show<boolean>({
       buttons: [
-        { text: msg('Cancel'), value: false },
+        { text: msg('Cancel'), value: false, autofocus: true, fill: 'outline', variant: 'primary' },
         { text: msg('OK'), value: true, variant: 'primary' }
       ],
       title,
       message
     });
   }
 
+  /**
+   * Shows a message dialog to the user. Use this method to display custom dialogs with any number of buttons.
+   *
+   * @param config - The configuration for the dialog.
+   * @returns A promise that resolves with the value of the button that was clicked, or `undefined` if the dialog was closed.
+   */
   static async show<T = unknown>(config: MessageDialogConfig<T>): Promise<T | undefined> {
     return await new Promise<T | undefined>(resolve => {
       config.buttons = config.buttons?.map(button => {
@@ -87,6 +113,7 @@ export class MessageDialog<T = unknown> extends ScopedElementsMixin(LitElement)
     });
   }
 
+  /** The configuration of the message dialog. */
   @property({ attribute: false }) config?: MessageDialogConfig<T>;
 
   override render(): TemplateResult {
@@ -101,6 +128,7 @@ export class MessageDialog<T = unknown> extends ScopedElementsMixin(LitElement)
           button => html`
             <sl-button
               @click=${() => button.action?.()}
+              ?autofocus=${button.autofocus}
               .fill=${button.fill ?? 'solid'}
               .variant=${button.variant ?? 'default'}
               slot="actions"
@@ -114,10 +142,12 @@ export class MessageDialog<T = unknown> extends ScopedElementsMixin(LitElement)
     `;
   }
 
+  /** Show the message dialog as a modal, in the top layer, with a backdrop. */
   showModal(): void {
     this.renderRoot.querySelector<Dialog>('sl-dialog')?.showModal();
   }
 
+  /** Close the message dialog. */
   close(): void {
     this.renderRoot.querySelector<Dialog>('sl-dialog')?.close();
   }

packages/tokens/src/magister/dark.json

@@ -22,7 +22,7 @@
         "description": "body.surface.100"
       },
       "overlay": {
-        "value": "rgba( {color.palette.primary.700} , {opacity.600})",
+        "value": "rgba({color.palette.primary.700} , {opacity.600})",
         "type": "color",
         "description": "body.surface.200"
       }
@@ -3269,7 +3269,7 @@
       },
       "success": {
         "background": {
-          "value": "rgba( {color.palette.success.base} , {opacity.300})",
+          "value": "rgba({color.palette.success.base} , {opacity.300})",
           "type": "color"
         },
         "foreground": {
@@ -3287,7 +3287,7 @@
       },
       "warning": {
         "background": {
-          "value": "rgba( {color.palette.warning.base} , {opacity.300})",
+          "value": "rgba({color.palette.warning.base} , {opacity.300})",
           "type": "color"
         },
         "foreground": {
@@ -3305,7 +3305,7 @@
       },
       "danger": {
         "background": {
-          "value": "rgba( {color.palette.danger.base} , {opacity.300})",
+          "value": "rgba({color.palette.danger.base} , {opacity.300})",
           "type": "color"
         },
         "foreground": {
@@ -3536,7 +3536,7 @@
         },
         "active": {
           "background": {
-            "value": "rgba( {color.palette.primary.400} , {opacity.600})",
+            "value": "rgba({color.palette.primary.400} , {opacity.600})",
             "type": "color"
           },
           "foreground": {

packages/tokens/src/magister/light.json

@@ -22,7 +22,7 @@
         "description": "body.surface.100"
       },
       "overlay": {
-        "value": "rgba( {color.palette.primary.800} , {opacity.900})",
+        "value": "rgba({color.palette.primary.800} , {opacity.900})",
         "type": "color",
         "description": "body.surface.200"
       }