From c9e644f3fc57d0b65c37fe06283b383641e56d3e Mon Sep 17 00:00:00 2001 From: Cory LaViska Date: Thu, 6 Jul 2023 10:36:29 -0400 Subject: [PATCH 1/4] Allow selecting menu items with space (#1429) * allow selecting menu items with space; #1423 * update PR --- docs/pages/resources/changelog.md | 1 + src/components/menu/menu.ts | 9 ++------- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index df0c4b84..bb92ac15 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 tests for `` [#1416] +- Added support for pressing [[Space]] to select/toggle selected `` elements [#1429] - Fixed a bug in `` where the `background` attribute was never passed to the QR code [#1416] - Fixed a bug in `` where aria attributes were incorrectly applied to the default `` causing Lighthouse errors [#1417] - Fixed a bug in `` that caused navigation to work incorrectly in some case [#1420] diff --git a/src/components/menu/menu.ts b/src/components/menu/menu.ts index 8fecda05..3091f834 100644 --- a/src/components/menu/menu.ts +++ b/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(); From fe3906f7665a6125dc8ed0771f43a3bd1d61283c Mon Sep 17 00:00:00 2001 From: Cory LaViska Date: Thu, 6 Jul 2023 10:36:41 -0400 Subject: [PATCH 2/4] Don't steal focus when removing focused tree items (#1430) * don't steal focus when removing focused tree items; #1428 * update PR link --- docs/pages/resources/changelog.md | 1 + src/components/tree/tree.ts | 6 ------ 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index bb92ac15..6c415285 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -19,6 +19,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Fixed a bug in `` where the `background` attribute was never passed to the QR code [#1416] - Fixed a bug in `` where aria attributes were incorrectly applied to the default `` causing Lighthouse errors [#1417] - Fixed a bug in `` that caused navigation to work incorrectly in some case [#1420] +- Fixed a bug in `` that caused focus to be stolen when removing focused tree items [#1430] ## 2.5.2 diff --git a/src/components/tree/tree.ts b/src/components/tree/tree.ts index 3c615611..9889f530 100644 --- a/src/components/tree/tree.ts +++ b/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]); - } } }; From a4f0ae9088af6332e25baedb99cf86e49979dea9 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Fri, 7 Jul 2023 13:51:22 -0400 Subject: [PATCH 3/4] fix: valueAsDate now falls back to native implementation (#1399) * fix: valueAsDate now falls back to native implementation * changelog * prettier * prettier --- docs/pages/resources/changelog.md | 2 +- src/components/input/input.ts | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 6c415285..1623301c 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -16,6 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Added tests for `` [#1416] - Added support for pressing [[Space]] to select/toggle selected `` elements [#1429] +- Fixed a bug in `valueAsDate` on `` where it would always set `type="date"` for the underlying `` 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 `` where the `background` attribute was never passed to the QR code [#1416] - Fixed a bug in `` where aria attributes were incorrectly applied to the default `` causing Lighthouse errors [#1417] - Fixed a bug in `` that caused navigation to work incorrectly in some case [#1420] @@ -24,7 +25,6 @@ New versions of Shoelace are released as-needed and generally occur when a criti ## 2.5.2 - Fixed broken source buttons in the docs [#1401] -- Fixed broken links in the docs [#1407] ## 2.5.1 diff --git a/src/components/input/input.ts b/src/components/input/input.ts index 4d80890e..3f4b1757 100644 --- a/src/components/input/input.ts +++ b/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 `` 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; } From 82446e211451bfbd7364eee4033b2490330b54a5 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Fri, 7 Jul 2023 15:32:23 -0400 Subject: [PATCH 4/4] Add modal tab tracking (#1403) * add modal tab tracking * prettier * sort by tabindex * sort by tabindex * add a dialog test case for shadow roots * add a changelog note * add a changelog note * prettier + test fixes * prettier + test fixes --- docs/pages/resources/changelog.md | 1 + src/components/dialog/dialog.test.ts | 123 ++++++++++++++++++++++++++- src/components/dialog/dialog.ts | 9 +- src/internal/modal.ts | 51 +++++++++-- src/internal/tabbable.ts | 28 ++++-- 5 files changed, 194 insertions(+), 18 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 1623301c..bdef8f7f 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -16,6 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Added tests for `` [#1416] - Added support for pressing [[Space]] to select/toggle selected `` elements [#1429] +- Fixed a bug in focus trapping of modal elements like ``. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `` inside a shadowRoot [#1403] - Fixed a bug in `valueAsDate` on `` where it would always set `type="date"` for the underlying `` 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 `` where the `background` attribute was never passed to the QR code [#1416] - Fixed a bug in `` where aria attributes were incorrectly applied to the default `` causing Lighthouse errors [#1417] diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts index a8d9ff82..0cabd063 100644 --- a/src/components/dialog/dialog.test.ts +++ b/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('', () => { 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` +

Dialog Example

+ + Lorem ipsum dolor sit amet, consectetur adipiscing elit. +
+ + + + + + Open Dialog + `; + } + } + + if (!window.customElements.get('a-container')) { + window.customElements.define('a-container', AContainer); + } + + const testCase = await fixture(html` +
+ + +

+ Open the dialog, then use Tab to cycle through the inputs. Focus should be trapped, but it reaches + things outside the dialog. +

+
+ `); + + const container = testCase.querySelector('a-container'); + + if (!container) { + throw Error('Could not find element.'); + } + + await elementUpdated(container); + const dialog = container.shadowRoot?.querySelector('sl-dialog'); + + if (!dialog) { + throw Error('Could not find 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); +} diff --git a/src/components/dialog/dialog.ts b/src/components/dialog/dialog.ts index b483922b..efa5672d 100644 --- a/src/components/dialog/dialog.ts +++ b/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 { ` : ''} - - + ${ + '' /* 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. */ + } +