Merge branch 'next' into lit-a11y-update

docs/pages/resources/changelog.md

@@ -15,16 +15,19 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 ## Next
 
 - Added tests for `<sl-qr-code>` [#1416]
+- Added support for pressing [[Space]] to select/toggle selected `<sl-menu-item>` elements [#1429]
+- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot [#1403]
+- Fixed a bug in `valueAsDate` on `<sl-input>` where it would always set `type="date"` for the underlying `<input>` element. It now falls back to the native browser implementation for the in-memory input. This may cause unexpected behavior if you're using `valueAsDate` on any input elements that aren't `type="date"`. [#1399]
 - Fixed a bug in `<sl-qr-code>` where the `background` attribute was never passed to the QR code [#1416]
 - Fixed a bug in `<sl-dropdown>` where aria attributes were incorrectly applied to the default `<slot>` causing Lighthouse errors [#1417]
 - Fixed a bug in `<sl-carousel>` that caused navigation to work incorrectly in some case [#1420]
 - Fixed a number of slots that incorrectly had aria- and/or role attributes directly on them [#1422]
+- Fixed a bug in `<sl-tree>` that caused focus to be stolen when removing focused tree items [#1430]
 - Updated ESLint and related plugins to the latest versions
 
 ## 2.5.2
 
 - Fixed broken source buttons in the docs [#1401]
-- Fixed broken links in the docs [#1407]
 
 ## 2.5.1
 

src/components/dialog/dialog.test.ts

@@ -1,6 +1,7 @@
 import '../../../dist/shoelace.js';
 // cspell:dictionaries lorem-ipsum
-import { expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { aTimeout, elementUpdated, expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import type SlDialog from './dialog';
@@ -146,4 +147,124 @@ describe('<sl-dialog>', () => {
 
     expect(el.open).to.be.false;
   });
+
+  // https://github.com/shoelace-style/shoelace/issues/1382
+  it('should properly cycle through tabbable elements when sl-dialog is used in a shadowRoot', async () => {
+    class AContainer extends LitElement {
+      get dialog() {
+        return this.shadowRoot?.querySelector('sl-dialog');
+      }
+
+      openDialog() {
+        this.dialog?.show();
+      }
+
+      render() {
+        return html`
+          <h1>Dialog Example</h1>
+          <sl-dialog label="Dialog" class="dialog-overview">
+            Lorem ipsum dolor sit amet, consectetur adipiscing elit.
+            <br />
+            <label><input type="checkbox" />A</label>
+            <label><input type="checkbox" />B</label>
+            <button>Button</button>
+          </sl-dialog>
+
+          <sl-button @click=${this.openDialog}>Open Dialog</sl-button>
+        `;
+      }
+    }
+
+    if (!window.customElements.get('a-container')) {
+      window.customElements.define('a-container', AContainer);
+    }
+
+    const testCase = await fixture(html`
+      <div>
+        <a-container></a-container>
+
+        <p>
+          Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches
+          things outside the dialog.
+        </p>
+      </div>
+    `);
+
+    const container = testCase.querySelector('a-container');
+
+    if (!container) {
+      throw Error('Could not find <a-container> element.');
+    }
+
+    await elementUpdated(container);
+    const dialog = container.shadowRoot?.querySelector('sl-dialog');
+
+    if (!dialog) {
+      throw Error('Could not find <sl-dialog> element.');
+    }
+
+    const closeButton = dialog.shadowRoot?.querySelector('sl-icon-button');
+    const checkbox1 = dialog.querySelector("input[type='checkbox']");
+    const checkbox2 = dialog.querySelectorAll("input[type='checkbox']")[1];
+    const button = dialog.querySelector('button');
+
+    // Opens modal.
+    const openModalButton = container.shadowRoot?.querySelector('sl-button');
+
+    if (openModalButton) openModalButton.click();
+
+    // Test tab cycling
+    await pressTab();
+
+    expect(container.shadowRoot?.activeElement).to.equal(dialog);
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    // Test Shift+Tab cycling
+
+    // I found these timeouts were needed for WebKit locally.
+    await aTimeout(10);
+    await sendKeys({ down: 'Shift' });
+    await aTimeout(10);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    // End shift+tab cycling
+    await sendKeys({ up: 'Shift' });
+  });
 });
+
+// We wait 50ms just to give the browser some time to figure out the current focus.
+// 50 was the magic number I found locally :shrug:
+async function pressTab() {
+  await aTimeout(50);
+  await sendKeys({ press: 'Tab' });
+  await aTimeout(50);
+}

src/components/dialog/dialog.ts

@@ -104,6 +104,7 @@ export default class SlDialog extends ShoelaceElement {
 
   disconnectedCallback() {
     super.disconnectedCallback();
+    this.modal.deactivate();
     unlockBodyScrolling(this);
   }
 
@@ -269,7 +270,7 @@ export default class SlDialog extends ShoelaceElement {
           aria-hidden=${this.open ? 'false' : 'true'}
           aria-label=${ifDefined(this.noHeader ? this.label : undefined)}
           aria-labelledby=${ifDefined(!this.noHeader ? 'title' : undefined)}
-          tabindex="0"
+          tabindex="-1"
         >
           ${!this.noHeader
             ? html`
@@ -292,8 +293,10 @@ export default class SlDialog extends ShoelaceElement {
                 </header>
               `
             : ''}
-
-          <slot part="body" class="dialog__body"></slot>
+          ${
+            '' /* The tabindex="-1" is here because the body is technically scrollable if overflowing. However, if there's no focusable elements inside, you won't actually be able to scroll it via keyboard. */
+          }
+          <slot part="body" class="dialog__body" tabindex="-1"></slot>
 
           <footer part="footer" class="dialog__footer">
             <slot name="footer"></slot>

src/components/input/input.ts

@@ -198,13 +198,17 @@ export default class SlInput extends ShoelaceElement implements ShoelaceFormCont
   // can be set before the component is rendered.
   //
 
-  /** Gets or sets the current value as a `Date` object. Returns `null` if the value can't be converted. */
+  /**
+   * Gets or sets the current value as a `Date` object. Returns `null` if the value can't be converted. This will use the native `<input type="{{type}}">` implementation and may result in an error.
+   */
   get valueAsDate() {
+    this.__dateInput.type = this.type;
     this.__dateInput.value = this.value;
     return this.input?.valueAsDate || this.__dateInput.valueAsDate;
   }
 
   set valueAsDate(newValue: Date | null) {
+    this.__dateInput.type = this.type;
     this.__dateInput.valueAsDate = newValue;
     this.value = this.__dateInput.value;
   }

src/components/menu/menu.ts

@@ -45,8 +45,8 @@ export default class SlMenu extends ShoelaceElement {
   }
 
   private handleKeyDown(event: KeyboardEvent) {
-    // Make a selection when pressing enter
-    if (event.key === 'Enter') {
+    // Make a selection when pressing enter or space
+    if (event.key === 'Enter' || event.key === ' ') {
       const item = this.getCurrentItem();
       event.preventDefault();
 
@@ -54,11 +54,6 @@ export default class SlMenu extends ShoelaceElement {
       item?.click();
     }
 
-    // Prevent scrolling when space is pressed
-    if (event.key === ' ') {
-      event.preventDefault();
-    }
-
     // Move the selection when pressing down or up
     if (['ArrowDown', 'ArrowUp', 'Home', 'End'].includes(event.key)) {
       const items = this.getAllItems();

src/components/tree/tree.ts

@@ -159,14 +159,8 @@ export default class SlTree extends ShoelaceElement {
   private handleTreeChanged = (mutations: MutationRecord[]) => {
     for (const mutation of mutations) {
       const addedNodes: SlTreeItem[] = [...mutation.addedNodes].filter(SlTreeItem.isTreeItem) as SlTreeItem[];
-      const removedNodes = [...mutation.removedNodes].filter(SlTreeItem.isTreeItem) as SlTreeItem[];
 
       addedNodes.forEach(this.initTreeItem);
-
-      // If the focused item has been removed form the DOM, move the focus to the first focusable item
-      if (removedNodes.includes(this.lastFocusedItem)) {
-        this.focusItem(this.getFocusableItems()[0]);
-      }
     }
   };
 

src/internal/modal.ts

@@ -1,10 +1,11 @@
-import { getTabbableBoundary } from './tabbable.js';
+import { getTabbableElements } from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
 
 export default class Modal {
   element: HTMLElement;
   tabDirection: 'forward' | 'backward' = 'forward';
+  currentFocus: HTMLElement | null;
 
   constructor(element: HTMLElement) {
     this.element = element;
@@ -22,6 +23,7 @@ export default class Modal {
 
   deactivate() {
     activeModals = activeModals.filter(modal => modal !== this.element);
+    this.currentFocus = null;
     document.removeEventListener('focusin', this.handleFocusIn);
     document.removeEventListener('keydown', this.handleKeyDown);
     document.removeEventListener('keyup', this.handleKeyUp);
@@ -34,11 +36,14 @@ export default class Modal {
 
   checkFocus() {
     if (this.isActive()) {
+      const tabbableElements = getTabbableElements(this.element);
       if (!this.element.matches(':focus-within')) {
-        const { start, end } = getTabbableBoundary(this.element);
+        const start = tabbableElements[0];
+        const end = tabbableElements[tabbableElements.length - 1];
         const target = this.tabDirection === 'forward' ? start : end;
 
         if (typeof target?.focus === 'function') {
+          this.currentFocus = target;
           target.focus({ preventScroll: true });
         }
       }
@@ -49,13 +54,45 @@ export default class Modal {
     this.checkFocus();
   }
 
-  handleKeyDown(event: KeyboardEvent) {
-    if (event.key === 'Tab' && event.shiftKey) {
-      this.tabDirection = 'backward';
+  get currentFocusIndex() {
+    return getTabbableElements(this.element).findIndex(el => el === this.currentFocus);
+  }
 
-      // Ensure focus remains trapped after the key is pressed
-      requestAnimationFrame(() => this.checkFocus());
+  handleKeyDown(event: KeyboardEvent) {
+    if (event.key !== 'Tab') return;
+
+    if (event.shiftKey) {
+      this.tabDirection = 'backward';
+    } else {
+      this.tabDirection = 'forward';
     }
+
+    event.preventDefault();
+
+    const tabbableElements = getTabbableElements(this.element);
+    const start = tabbableElements[0];
+    let focusIndex = this.currentFocusIndex;
+
+    if (focusIndex === -1) {
+      this.currentFocus = start;
+      this.currentFocus.focus({ preventScroll: true });
+      return;
+    }
+
+    const addition = this.tabDirection === 'forward' ? 1 : -1;
+
+    if (focusIndex + addition >= tabbableElements.length) {
+      focusIndex = 0;
+    } else if (this.currentFocusIndex + addition < 0) {
+      focusIndex = tabbableElements.length - 1;
+    } else {
+      focusIndex += addition;
+    }
+
+    this.currentFocus = tabbableElements[focusIndex];
+    this.currentFocus?.focus({ preventScroll: true });
+
+    setTimeout(() => this.checkFocus());
   }
 
   handleKeyUp() {

src/internal/tabbable.ts

@@ -1,3 +1,5 @@
+import { offsetParent } from 'composed-offset-position';
+
 /** Determines if the specified element is tabbable using heuristics inspired by https://github.com/focus-trap/tabbable */
 function isTabbable(el: HTMLElement) {
   const tag = el.tagName.toLowerCase();
@@ -23,7 +25,8 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent === null) {
+  // offsetParent() is added because otherwise it misses elements in Safari
+  if (el.offsetParent === null && offsetParent(el) === null) {
     return false;
   }
 
@@ -56,10 +59,20 @@ function isTabbable(el: HTMLElement) {
  * element because it short-circuits after finding the first and last ones.
  */
 export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
+  const tabbableElements = getTabbableElements(root);
+
+  // Find the first and last tabbable elements
+  const start = tabbableElements[0] ?? null;
+  const end = tabbableElements[tabbableElements.length - 1] ?? null;
+
+  return { start, end };
+}
+
+export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
-    if (el instanceof HTMLElement) {
+    if (el instanceof Element) {
       allElements.push(el);
 
       if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') {
@@ -73,9 +86,10 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  // Find the first and last tabbable elements
-  const start = allElements.find(el => isTabbable(el)) ?? null;
-  const end = allElements.reverse().find(el => isTabbable(el)) ?? null;
-
-  return { start, end };
+  return allElements.filter(isTabbable).sort((a, b) => {
+    // Make sure we sort by tabindex.
+    const aTabindex = Number(a.getAttribute('tabindex')) || 0;
+    const bTabindex = Number(b.getAttribute('tabindex')) || 0;
+    return bTabindex - aTabindex;
+  });
 }