diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 1623301cfc..bdef8f7f13 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 a8d9ff82c2..0cabd06302 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 b483922bff..efa5672d2c 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. */ + } +