refactor!: replace details with custom collapsible, add tests (#5978)

vaadin · Jun 16, 2023 · 4d236f0 · 4d236f0
1 parent 60210d0
commit 4d236f0
Show file tree

Hide file tree

Showing 7 changed files with 283 additions and 378 deletions.
diff --git a/packages/side-nav/src/vaadin-side-nav-base-styles.js b/packages/side-nav/src/vaadin-side-nav-base-styles.js
@@ -83,28 +83,19 @@ export const sideNavBaseStyles = css`
     display: none !important;
   }
 
-  summary {
+  button {
     display: flex;
     align-items: center;
-    justify-content: space-between;
-  }
-
-  summary ::slotted([slot='label']) {
-    display: block;
-  }
-
-  summary::-webkit-details-marker {
-    display: none;
-  }
-
-  summary::marker {
-    content: '';
-  }
-
-  summary::after {
-    display: inline-flex;
-    align-items: center;
-    justify-content: center;
+    justify-content: inherit;
+    width: 100%;
+    margin: 0;
+    padding: 0;
+    background-color: initial;
+    color: inherit;
+    border: initial;
+    outline: none;
+    font: inherit;
+    text-align: inherit;
   }
 
   [part='children'] {

diff --git a/packages/side-nav/src/vaadin-side-nav.d.ts b/packages/side-nav/src/vaadin-side-nav.d.ts
@@ -4,6 +4,7 @@
  * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
  */
 import { LitElement } from 'lit';
+import { FocusMixin } from '@vaadin/a11y-base/src/focus-mixin.js';
 import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js';
 import { PolylitMixin } from '@vaadin/component-base/src/polylit-mixin.js';
 import { ThemableMixin } from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js';
@@ -50,7 +51,7 @@ export type SideNavEventMap = HTMLElementEventMap & SideNavCustomEventMap;
  *
  * @fires {CustomEvent} collapsed-changed - Fired when the `collapsed` property changes.
  */
-declare class SideNav extends ElementMixin(ThemableMixin(PolylitMixin(LitElement))) {
+declare class SideNav extends FocusMixin(ElementMixin(ThemableMixin(PolylitMixin(LitElement)))) {
   /**
    * Whether the side nav is collapsible. When enabled, the toggle icon is shown.
    */

diff --git a/packages/side-nav/src/vaadin-side-nav.js b/packages/side-nav/src/vaadin-side-nav.js
@@ -4,6 +4,8 @@
  * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
  */
 import { html, LitElement } from 'lit';
+import { ifDefined } from 'lit/directives/if-defined.js';
+import { FocusMixin } from '@vaadin/a11y-base/src/focus-mixin.js';
 import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js';
 import { PolylitMixin } from '@vaadin/component-base/src/polylit-mixin.js';
 import { generateUniqueId } from '@vaadin/component-base/src/unique-id-utils.js';
@@ -50,11 +52,15 @@ function isEnabled() {
  * @mixes ThemableMixin
  * @mixes ElementMixin
  */
-class SideNav extends ElementMixin(ThemableMixin(PolylitMixin(LitElement))) {
+class SideNav extends FocusMixin(ElementMixin(ThemableMixin(PolylitMixin(LitElement)))) {
   static get is() {
     return 'vaadin-side-nav';
   }
 
+  static get shadowRootOptions() {
+    return { ...LitElement.shadowRootOptions, delegatesFocus: true };
+  }
+
   static get properties() {
     return {
       /**
@@ -86,6 +92,17 @@ class SideNav extends ElementMixin(ThemableMixin(PolylitMixin(LitElement))) {
     return sideNavBaseStyles;
   }
 
+  constructor() {
+    super();
+
+    this._labelId = `side-nav-label-${generateUniqueId()}`;
+  }
+
+  /** @protected */
+  get focusElement() {
+    return this.shadowRoot.querySelector('button');
+  }
+
   /** @protected */
   firstUpdated() {
     // By default, if the user hasn't provided a custom role,
@@ -97,36 +114,55 @@ class SideNav extends ElementMixin(ThemableMixin(PolylitMixin(LitElement))) {
 
   /** @protected */
   render() {
-    const label = this.querySelector('[slot="label"]');
-    if (label && this.collapsible) {
-      return html`
-        <details ?open="${!this.collapsed}" @toggle="${this.__toggleCollapsed}">${this.__renderBody(label)}</details>
-      `;
+    return html`
+      <button
+        part="label"
+        @click="${this._onLabelClick}"
+        aria-expanded="${ifDefined(this.collapsible ? !this.collapsed : null)}"
+        aria-controls="children"
+      >
+        <slot name="label" @slotchange="${this._onLabelSlotChange}"></slot>
+        <span part="toggle" aria-hidden="true"></span>
+      </button>
+      <ul id="children" part="children" ?hidden="${this.collapsed}" aria-hidden="${this.collapsed ? 'true' : 'false'}">
+        <slot></slot>
+      </ul>
+    `;
+  }
+
+  /**
+   * @param {Event} event
+   * @return {boolean}
+   * @protected
+   * @override
+   */
+  _shouldSetFocus(event) {
+    return event.composedPath()[0] === this.focusElement;
+  }
+
+  /** @private */
+  _onLabelClick() {
+    if (this.collapsible) {
+      this.__toggleCollapsed();
     }
-    return this.__renderBody(label);
   }
 
   /** @private */
-  __renderBody(label) {
+  _onLabelSlotChange() {
+    const label = this.querySelector('[slot="label"]');
     if (label) {
-      if (!label.id) label.id = `side-nav-label-${generateUniqueId()}`;
+      if (!label.id) {
+        label.id = this._labelId;
+      }
       this.setAttribute('aria-labelledby', label.id);
     } else {
       this.removeAttribute('aria-labelledby');
     }
-    return html`
-      <summary part="label" ?hidden="${label == null}">
-        <slot name="label" @slotchange="${() => this.requestUpdate()}"></slot>
-      </summary>
-      <ul part="children">
-        <slot></slot>
-      </ul>
-    `;
   }
 
   /** @private */
-  __toggleCollapsed(e) {
-    this.collapsed = !e.target.open;
+  __toggleCollapsed() {
+    this.collapsed = !this.collapsed;
   }
 }
 

diff --git a/packages/side-nav/test/accessibility.test.js b/packages/side-nav/test/accessibility.test.js
@@ -1,5 +1,6 @@
 import { expect } from '@esm-bundle/chai';
-import { fixtureSync, nextRender } from '@vaadin/testing-helpers';
+import { fixtureSync, nextFrame, nextRender } from '@vaadin/testing-helpers';
+import { sendKeys } from '@web/test-runner-commands';
 import '../enable.js';
 import '../vaadin-side-nav-item.js';
 import '../vaadin-side-nav.js';
@@ -23,4 +24,105 @@ describe('accessibility', () => {
       expect(sideNavWithCustomRole.getAttribute('role')).to.equal('custom role');
     });
   });
+
+  describe('label', () => {
+    let sideNav;
+
+    beforeEach(async () => {
+      sideNav = fixtureSync('<vaadin-side-nav></vaadin-side-nav>');
+      await nextRender();
+    });
+
+    it('should not have aria-labelledby attribute by default', () => {
+      expect(sideNav.hasAttribute('aria-labelledby')).to.be.false;
+    });
+
+    it('should set id on the lazily added label element', async () => {
+      const label = document.createElement('label');
+      label.setAttribute('slot', 'label');
+
+      sideNav.appendChild(label);
+      await nextFrame();
+
+      const ID_REGEX = /^side-nav-label-\d+$/u;
+      expect(label.getAttribute('id')).to.match(ID_REGEX);
+    });
+
+    it('should not override custom id on the lazily added label', async () => {
+      const label = document.createElement('label');
+      label.setAttribute('slot', 'label');
+      label.id = 'custom-label';
+
+      sideNav.appendChild(label);
+      await nextFrame();
+
+      expect(label.getAttribute('id')).to.equal('custom-label');
+    });
+
+    it('should set aria-labelledby attribute when adding a label', async () => {
+      const label = document.createElement('label');
+      label.setAttribute('slot', 'label');
+
+      sideNav.appendChild(label);
+      await nextFrame();
+
+      const ID_REGEX = /^side-nav-label-\d+$/u;
+      expect(sideNav.getAttribute('aria-labelledby')).to.match(ID_REGEX);
+      expect(sideNav.getAttribute('aria-labelledby')).to.be.equal(label.id);
+    });
+
+    it('should remove aria-labelledby attribute when removing a label', async () => {
+      const label = document.createElement('label');
+      label.setAttribute('slot', 'label');
+
+      sideNav.appendChild(label);
+      await nextFrame();
+
+      sideNav.removeChild(label);
+      await nextFrame();
+
+      expect(sideNav.hasAttribute('aria-labelledby')).to.be.false;
+    });
+  });
+
+  describe('focus', () => {
+    let wrapper, sideNav, input;
+
+    beforeEach(async () => {
+      wrapper = fixtureSync(`
+        <div>
+          <input />
+          <vaadin-side-nav>
+            <vaadin-side-nav-item path="/foo">Foo</vaadin-side-nav-item>
+            <vaadin-side-nav-item path="/bar">Bar</vaadin-side-nav-item>
+          </vaadin-side-nav>
+        </div>
+      `);
+      await nextRender();
+      [input, sideNav] = wrapper.children;
+    });
+
+    it('should delegate focus to the native button element in shadow root', async () => {
+      input.focus();
+      await sendKeys({ press: 'Tab' });
+      const button = sideNav.shadowRoot.querySelector('button');
+      expect(sideNav.shadowRoot.activeElement).to.be.equal(button);
+    });
+
+    it('should set focused and focus-ring attributes when keyboard focused', async () => {
+      input.focus();
+      await sendKeys({ press: 'Tab' });
+      expect(sideNav.hasAttribute('focused')).to.be.true;
+      expect(sideNav.hasAttribute('focus-ring')).to.be.true;
+    });
+
+    it('should remove focused and and focus-ring attributes when losing focus', async () => {
+      input.focus();
+      await sendKeys({ press: 'Tab' });
+      // Move focus to the side nav item
+      await sendKeys({ press: 'Tab' });
+      expect(sideNav.hasAttribute('focused')).to.be.false;
+      expect(sideNav.hasAttribute('focus-ring')).to.be.false;
+    });
+  });
 });