fix(sbb-toggle): fix event timing (#2646)

sbb-design-systems · May 8, 2024 · 6da2d26 · 6da2d26
1 parent 62eb0d5
commit 6da2d26
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 195 deletions.
diff --git a/src/components/tag/tag-group/tag-group.ts b/src/components/tag/tag-group/tag-group.ts
@@ -83,7 +83,7 @@ export class SbbTagGroupElement extends SbbNamedSlotListMixin<SbbTagElement, typ
 
   /** The child instances of sbb-tag as an array. */
   public get tags(): SbbTagElement[] {
-    return Array.from(this.querySelectorAll?.('sbb-tag') ?? []) as SbbTagElement[];
+    return Array.from(this.querySelectorAll?.('sbb-tag') ?? []);
   }
 
   protected override willUpdate(changedProperties: PropertyValues<WithListChildren<this>>): void {

diff --git a/src/components/tag/tag/tag.ts b/src/components/tag/tag/tag.ts
@@ -5,21 +5,11 @@ import { customElement, property } from 'lit/decorators.js';
 import { SbbButtonBaseElement } from '../../core/base-elements.js';
 import { SbbConnectedAbortController, SbbSlotStateController } from '../../core/controllers.js';
 import { EventEmitter } from '../../core/eventing.js';
-import type {
-  SbbCheckedStateChange,
-  SbbStateChange,
-  SbbValueStateChange,
-} from '../../core/interfaces.js';
 import { SbbDisabledTabIndexActionMixin } from '../../core/mixins.js';
 import { SbbIconNameMixin } from '../../icon.js';
 
 import style from './tag.scss?lit&inline';
 
-export type SbbTagStateChange = Extract<
-  SbbStateChange,
-  SbbValueStateChange | SbbCheckedStateChange
->;
-
 /**
  * It displays a selectable element which can be used as a filter.
  *

diff --git a/src/components/toggle/toggle-option/readme.md b/src/components/toggle/toggle-option/readme.md
@@ -35,7 +35,7 @@ The component can be displayed in `checked` or `disabled` states using the self-
 | ---------- | ----------- | ------- | --------------------- | ------- | -------------------------------------------------------------------------------------------------------------------------------- |
 | `checked`  | `checked`   | public  | `boolean`             | `false` | Whether the toggle-option is checked.                                                                                            |
 | `disabled` | `disabled`  | public  | `boolean`             | `false` | Whether the toggle option is disabled.                                                                                           |
-| `value`    | `value`     | public  | `string \| null`      | `null`  | Value of toggle-option.                                                                                                          |
+| `value`    | `value`     | public  | `string`              | `''`    | Value of toggle-option.                                                                                                          |
 | `iconName` | `icon-name` | public  | `string \| undefined` |         | The icon name we want to use, choose from the small icon variants from the ui-icons category from here https://icons.app.sbb.ch. |
 
 ## Slots

diff --git a/src/components/toggle/toggle-option/toggle-option.ts b/src/components/toggle/toggle-option/toggle-option.ts
@@ -5,9 +5,8 @@ import { customElement, property } from 'lit/decorators.js';
 import { SbbConnectedAbortController, SbbSlotStateController } from '../../core/controllers.js';
 import { hostAttributes } from '../../core/decorators.js';
 import { setOrRemoveAttribute } from '../../core/dom.js';
-import { EventEmitter } from '../../core/eventing.js';
 import { SbbIconNameMixin } from '../../icon.js';
-import type { SbbToggleElement, SbbToggleStateChange } from '../toggle.js';
+import type { SbbToggleElement } from '../toggle.js';
 
 import style from './toggle-option.scss?lit&inline';
 
@@ -23,41 +22,26 @@ import style from './toggle-option.scss?lit&inline';
 })
 export class SbbToggleOptionElement extends SbbIconNameMixin(LitElement) {
   public static override styles: CSSResultGroup = style;
-  public static readonly events = {
-    stateChange: 'stateChange',
-  } as const;
 
-  /**
-   * Whether the toggle-option is checked.
-   */
+  /** Whether the toggle-option is checked. */
   @property({ reflect: true, type: Boolean })
   public checked = false;
 
-  /**
-   * Whether the toggle option is disabled.
-   */
+  /** Whether the toggle option is disabled. */
   @property({ reflect: true, type: Boolean })
   public disabled: boolean = false;
 
-  /**
-   * Value of toggle-option.
-   */
+  /** Value of toggle-option. */
   @property()
-  public value: string | null = null;
+  public set value(value: string) {
+    this._value = `${value}`;
+  }
+  public get value(): string {
+    return this._value;
+  }
+  private _value: string = '';
 
   private _toggle?: SbbToggleElement;
-
-  /**
-   * @internal
-   * Internal event that emits whenever the state of the toggle option
-   * in relation to the parent toggle changes.
-   */
-  private _stateChange: EventEmitter<SbbToggleStateChange> = new EventEmitter(
-    this,
-    SbbToggleOptionElement.events.stateChange,
-    { bubbles: true },
-  );
-
   private _abort = new SbbConnectedAbortController(this);
 
   public constructor() {
@@ -68,6 +52,9 @@ export class SbbToggleOptionElement extends SbbIconNameMixin(LitElement) {
   public override connectedCallback(): void {
     super.connectedCallback();
     const signal = this._abort.signal;
+
+    // We need to listen input event on host as with keyboard navigation
+    // the Input Event is triggered from sbb-toggle.
     this.addEventListener('input', () => this._handleInput(), { signal });
     this.addEventListener('click', () => this.shadowRoot!.querySelector('label')?.click(), {
       signal,
@@ -81,28 +68,19 @@ export class SbbToggleOptionElement extends SbbIconNameMixin(LitElement) {
     super.willUpdate(changedProperties);
 
     if (changedProperties.has('checked')) {
-      this._handleCheckedChange(this.checked, changedProperties.get('checked')!);
-    }
-    if (changedProperties.has('value')) {
-      this._handleValueChange(this.value, changedProperties.get('value')!);
+      this.setAttribute('aria-checked', `${this.checked}`);
+      this._verifyTabindex();
+      if (this.checked) {
+        this._uncheckOtherOptions();
+      }
     }
     if (changedProperties.has('disabled')) {
       this._handleDisabledChange();
     }
   }
 
-  private _handleCheckedChange(currentValue: boolean, previousValue: boolean): void {
-    if (currentValue !== previousValue) {
-      this.setAttribute('aria-checked', `${this.checked}`);
-      this._stateChange.emit({ type: 'checked', checked: this.checked });
-      this._verifyTabindex();
-    }
-  }
-
-  private _handleValueChange(currentValue: string | null, previousValue: string | null): void {
-    if (this.checked && currentValue !== previousValue) {
-      this._stateChange.emit({ type: 'value', value: currentValue });
-    }
+  private _uncheckOtherOptions(): void {
+    this._toggle?.options.filter((o) => o !== this).forEach((o) => (o.checked = false));
   }
 
   private _handleDisabledChange(): void {
@@ -120,10 +98,11 @@ export class SbbToggleOptionElement extends SbbIconNameMixin(LitElement) {
   }
 
   private _handleInput(): void {
-    if (this.checked || this.disabled) {
+    if (this.disabled) {
       return;
     }
     this.checked = true;
+    this._uncheckOtherOptions();
   }
 
   private _verifyTabindex(): void {

diff --git a/src/components/toggle/toggle/readme.md b/src/components/toggle/toggle/readme.md
@@ -34,12 +34,13 @@ The component has two different sizes, `s` and `m` (default), which can be set u
 
 ## Properties
 
-| Name       | Attribute  | Privacy | Type                      | Default | Description                                                                                                                                      |
-| ---------- | ---------- | ------- | ------------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------ |
-| `disabled` | `disabled` | public  | `boolean`                 | `false` | Whether the toggle is disabled.                                                                                                                  |
-| `even`     | `even`     | public  | `boolean`                 | `false` | If true, set the width of the component fixed; if false, the width is dynamic based on the label of the sbb-toggle-option.                       |
-| `size`     | `size`     | public  | `'s' \| 'm' \| undefined` | `'m'`   | Size variant, either m or s.                                                                                                                     |
-| `value`    | `value`    | public  | `any \| null`             |         | The value of the toggle. It needs to be mutable since it is updated whenever a new option is selected (see the `onToggleOptionSelect()` method). |
+| Name       | Attribute  | Privacy | Type                       | Default | Description                                                                                                                                      |
+| ---------- | ---------- | ------- | -------------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `disabled` | `disabled` | public  | `boolean`                  | `false` | Whether the toggle is disabled.                                                                                                                  |
+| `even`     | `even`     | public  | `boolean`                  | `false` | If true, set the width of the component fixed; if false, the width is dynamic based on the label of the sbb-toggle-option.                       |
+| `size`     | `size`     | public  | `'s' \| 'm' \| undefined`  | `'m'`   | Size variant, either m or s.                                                                                                                     |
+| `value`    | `value`    | public  | `string`                   | `null`  | The value of the toggle. It needs to be mutable since it is updated whenever a new option is selected (see the `onToggleOptionSelect()` method). |
+| `options`  | -          | public  | `SbbToggleOptionElement[]` |         | The child instances of sbb-toggle-option as an array.                                                                                            |
 
 ## Events
 

diff --git a/src/components/toggle/toggle/toggle.e2e.ts b/src/components/toggle/toggle/toggle.e2e.ts
@@ -1,4 +1,4 @@
-import { assert, expect, nextFrame } from '@open-wc/testing';
+import { assert, expect } from '@open-wc/testing';
 import { sendKeys } from '@web/test-runner-commands';
 import { html } from 'lit/static-html.js';
 
@@ -11,22 +11,26 @@ import { SbbToggleElement } from './toggle.js';
 import '../toggle-option.js';
 
 describe(`sbb-toggle with ${fixture.name}`, () => {
-  let element: SbbToggleElement;
+  let element: SbbToggleElement,
+    firstOption: SbbToggleOptionElement,
+    secondOption: SbbToggleOptionElement;
 
   beforeEach(async () => {
     element = await fixture(
       html`
         <sbb-toggle value="Value one">
-          <sbb-toggle-option id="sbb-toggle-option-1" value="Value one"
-            >Value one</sbb-toggle-option
-          >
-          <sbb-toggle-option id="sbb-toggle-option-2" value="Value two"
-            >Value two</sbb-toggle-option
-          >
+          <sbb-toggle-option id="sbb-toggle-option-1" value="Value one">
+            Value one
+          </sbb-toggle-option>
+          <sbb-toggle-option id="sbb-toggle-option-2" value="Value two">
+            Value two
+          </sbb-toggle-option>
         </sbb-toggle>
       `,
       { modules: ['./toggle.ts', '../toggle-option.ts'] },
     );
+    firstOption = element.querySelector<SbbToggleOptionElement>('#sbb-toggle-option-1')!;
+    secondOption = element.querySelector<SbbToggleOptionElement>('#sbb-toggle-option-2')!;
   });
 
   it('renders', () => {
@@ -35,25 +39,16 @@ describe(`sbb-toggle with ${fixture.name}`, () => {
 
   describe('events', () => {
     it('selects option on click', async () => {
-      const firstOption = element.querySelector(':scope > sbb-toggle-option#sbb-toggle-option-1');
-      const secondOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-2',
-      ) as SbbToggleOptionElement;
-
       expect(firstOption).to.have.attribute('checked');
 
       secondOption.click();
       await waitForLitRender(secondOption);
-      await nextFrame();
 
       expect(secondOption).to.have.attribute('checked');
       expect(firstOption).not.to.have.attribute('checked');
     });
 
     it('selects option on checked attribute change', async () => {
-      const firstOption = element.querySelector(':scope > sbb-toggle-option#sbb-toggle-option-1');
-      const secondOption = element.querySelector(':scope > sbb-toggle-option#sbb-toggle-option-2')!;
-
       expect(firstOption).to.have.attribute('checked');
 
       secondOption.toggleAttribute('checked', true);
@@ -63,35 +58,54 @@ describe(`sbb-toggle with ${fixture.name}`, () => {
       expect(firstOption).not.to.have.attribute('checked');
     });
 
+    it('selects option on checked property change', async () => {
+      expect(firstOption.checked).to.equal(true);
+
+      secondOption.checked = true;
+      await waitForLitRender(element);
+
+      expect(firstOption.checked).to.equal(false);
+      expect(secondOption.checked).to.equal(true);
+    });
+
     it('dispatches event on option change', async () => {
-      const firstOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-1',
-      ) as SbbToggleOptionElement;
-      const secondOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-2',
-      ) as SbbToggleOptionElement;
       const changeSpy = new EventSpy('change');
       const inputSpy = new EventSpy('input');
 
+      let valueInEvent;
+
+      // Checking value in events of EventSpy is too late to check the real use case,
+      // therefore we create a once-EventListener manually here.
+      element.addEventListener(
+        'change',
+        (event) => (valueInEvent = (event.target as SbbToggleElement).value),
+        { once: true },
+      );
+
       secondOption.click();
+      await waitForLitRender(firstOption);
       await waitForCondition(() => changeSpy.events.length === 1);
-      expect(changeSpy.count).to.be.equal(1);
       await waitForCondition(() => inputSpy.events.length === 1);
-      expect(inputSpy.count).to.be.equal(1);
+      expect(valueInEvent).to.equal('Value two');
+
+      // Checking value in events of EventSpy is too late to check the real use case,
+      // therefore we create a once-EventListener manually here.
+      element.addEventListener(
+        'change',
+        (event) => (valueInEvent = (event.target as SbbToggleElement).value),
+        { once: true },
+      );
 
       firstOption.click();
       await waitForLitRender(firstOption);
+      await waitForCondition(() => changeSpy.events.length === 2);
+      await waitForCondition(() => inputSpy.events.length === 2);
+
       expect(firstOption).to.have.attribute('checked');
+      expect(valueInEvent).to.equal('Value one');
     });
 
     it('prevents selection with disabled state', async () => {
-      const firstOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-1',
-      ) as SbbToggleOptionElement;
-      const secondOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-2',
-      ) as SbbToggleOptionElement;
-
       element.disabled = true;
       await waitForLitRender(element);
 
@@ -112,22 +126,14 @@ describe(`sbb-toggle with ${fixture.name}`, () => {
     it('selects option on left arrow key pressed', async () => {
       const changeSpy = new EventSpy('change');
       const inputSpy = new EventSpy('input');
-      const firstOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-1',
-      ) as SbbToggleOptionElement;
-      const secondOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-2',
-      ) as SbbToggleOptionElement;
 
       firstOption.focus();
       await sendKeys({ down: 'ArrowLeft' });
       await waitForLitRender(element);
 
       expect(secondOption).to.have.attribute('checked');
       await waitForCondition(() => changeSpy.events.length === 1);
-      expect(changeSpy.count).to.be.equal(1);
       await waitForCondition(() => inputSpy.events.length === 1);
-      expect(inputSpy.count).to.be.equal(1);
 
       firstOption.click();
       await waitForLitRender(firstOption);
@@ -138,12 +144,6 @@ describe(`sbb-toggle with ${fixture.name}`, () => {
     it('selects option on right arrow key pressed', async () => {
       const changeSpy = new EventSpy('change');
       const inputSpy = new EventSpy('input');
-      const firstOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-1',
-      ) as SbbToggleOptionElement;
-      const secondOption = element.querySelector(
-        ':scope > sbb-toggle-option#sbb-toggle-option-2',
-      ) as SbbToggleOptionElement;
 
       firstOption.focus();
       await waitForLitRender(firstOption);
@@ -152,9 +152,7 @@ describe(`sbb-toggle with ${fixture.name}`, () => {
 
       expect(secondOption).to.have.attribute('checked');
       await waitForCondition(() => changeSpy.events.length === 1);
-      expect(changeSpy.count).to.be.equal(1);
       await waitForCondition(() => inputSpy.events.length === 1);
-      expect(inputSpy.count).to.be.equal(1);
 
       firstOption.click();
       await waitForLitRender(firstOption);