diff --git a/.changeset/loud-suits-stare.md b/.changeset/loud-suits-stare.md new file mode 100644 index 0000000000..bc3eaac5e2 --- /dev/null +++ b/.changeset/loud-suits-stare.md @@ -0,0 +1,5 @@ +--- +"@stackoverflow/stacks": patch +--- + +SPARK-8: Fix Modal text selection issue when using keyboard diff --git a/docs/product/components/modals.html b/docs/product/components/modals.html index 0bad757b6d..f92c07d666 100644 --- a/docs/product/components/modals.html +++ b/docs/product/components/modals.html @@ -419,4 +419,4 @@

Congratulations - + \ No newline at end of file diff --git a/lib/components/modal/modal.test.ts b/lib/components/modal/modal.test.ts index 7564850442..df0ab84a8e 100644 --- a/lib/components/modal/modal.test.ts +++ b/lib/components/modal/modal.test.ts @@ -8,8 +8,15 @@ const user = userEvent.setup(); const createModal = ({ hidden = true, initialFocusEl, -}: { hidden?: boolean; initialFocusEl?: ReturnType } = {}) => html` -
+ renderFocusables = true, + excludeTabIndex = false, +}: { + hidden?: boolean; + initialFocusEl?: ReturnType; + renderFocusables?: boolean; + excludeTabIndex?: boolean; +} = {}) => html` +

- Description + Description

- - ${initialFocusEl} + ${ + renderFocusables + ? html` + + ${initialFocusEl} + ` + : null + }

- -
- - -
- - + ${ + renderFocusables + ? html`
+ + +
+ + ` + : null + } @@ -156,4 +189,114 @@ describe("modal", () => { // has not changed to the first focusable element expect(closeButton).to.have.focus; }); + + it("should not shift focus outside the modal when tab is pressed if there's no tabbable element in the modal", async () => { + // test with `tabindex="-1"` + await testFocusChange(true); + + // reset the test + const controller = await screen.findByTestId("controller"); + controller.remove(); + + // test without `tabindex="-1"` + await testFocusChange(false); + + async function testFocusChange(withTabIndex: boolean): Promise { + await fixture( + createModal({ + renderFocusables: false, + excludeTabIndex: !withTabIndex, + }) + ); + + const modal = await screen.findByTestId("modal"); + const trigger = await screen.findByTestId("trigger"); + + expect(modal).not.to.be.visible; + await user.click(trigger); + expect(modal).to.be.visible; + + // wait for s-modal:shown handler to complete + await new Promise((resolve) => + modal.addEventListener("s-modal:shown", resolve) + ); + + // since there's nothing else to focus, the modal itself or the triggering element + // should be focused. depends on whether the modal has a `tabindex` attribute or not. + const expectedFocusElement = withTabIndex ? modal : trigger; + expect(expectedFocusElement).to.have.focus; + + await user.tab(); + + // since there's nothing else to focus, the same element should still be focused + expect(expectedFocusElement).to.have.focus; + } + }); + + it("should not deselect highlighted text when a keypress is detected", async () => { + await fixture(createModal({ excludeTabIndex: true })); + + const modal = await screen.findByTestId("modal"); + const trigger = await screen.findByTestId("trigger"); + + expect(modal).not.to.be.visible; + await user.click(trigger); + expect(modal).to.be.visible; + + // highlight some text with the cursor + const description = await screen.findByTestId("modal-text-body"); + await user.pointer([ + // left click and hold at char 0 + { target: description, offset: 0, keys: "[MouseLeft>]" }, + // drag the mouse to the right 5 characters + { offset: 5 }, + // release the left mouse button + { keys: "[/MouseLeft]" }, + ]); + + // confirm highlight + const selection = document.getSelection()?.toString(); + expect(selection).to.be.equal("Descr"); + + // simulate a few non-Tab keypresses + await user.keyboard( + "{s}{t}{a}{c}{k}{Shift}{Control}{ArrowUp}{ArrowRight}" + ); + + // highlight should remain intact + expect(selection).to.be.equal("Descr"); + }); + + it("should cycle through focusable modal elements when tab is pressed", async () => { + await fixture(createModal()); + + const modal = await screen.findByTestId("modal"); + const trigger = await screen.findByTestId("trigger"); + + expect(modal).not.to.be.visible; + await user.click(trigger); + expect(modal).to.be.visible; + + // wait for s-modal:shown handler to complete + await new Promise((resolve) => + modal.addEventListener("s-modal:shown", resolve) + ); + + const firstFocusableElement = await screen.findByTestId( + "first-focusable-element" + ); + const secondFocusableElement = await screen.findByTestId("save-btn"); + const thirdFocusableElement = await screen.findByTestId("cancel-btn"); + const lastFocusableElement = await screen.findByTestId("close-btn"); + + expect(firstFocusableElement).to.have.focus; + await user.tab(); + expect(secondFocusableElement).to.have.focus; + await user.tab(); + expect(thirdFocusableElement).to.have.focus; + await user.tab(); + expect(lastFocusableElement).to.have.focus; + await user.tab(); + expect(firstFocusableElement).to.have.focus; + }); }); diff --git a/lib/components/modal/modal.ts b/lib/components/modal/modal.ts index ab35f53f18..670ffb2870 100644 --- a/lib/components/modal/modal.ts +++ b/lib/components/modal/modal.ts @@ -225,13 +225,14 @@ export class ModalController extends Stacks.StacksController { this.modalTarget.addEventListener( "s-modal:shown", () => { - const initialFocus = - this.firstVisible(this.initialFocusTargets) ?? - this.firstVisible(this.getAllTabbables()); - // Only set focus if focus is not already set on an element within the modal if (!this.modalTarget.contains(document.activeElement)) { - initialFocus?.focus(); + const initialFocus = + this.firstVisible(this.initialFocusTargets) ?? + this.firstVisible(this.getAllTabbables()) ?? + this.modalTarget; //If there's nothing else, focus the modal itself + + initialFocus.focus(); } }, { once: true } @@ -242,9 +243,21 @@ export class ModalController extends Stacks.StacksController { * Returns keyboard focus to the modal if it has left or is about to leave. */ private keepFocusWithinModal(e: KeyboardEvent) { - // If somehow the user has tabbed out of the modal or if focus started outside the modal, push them to the first item. + if (e.key !== "Tab") { + return; + } + + //Tab key was pressed, figure out what to focus next + const tabbables = this.getAllTabbables(); + if (tabbables.length === 0) { + // Nothing focusable found in the modal. Stop the user from tabbing to something outside the modal + e.preventDefault(); + return; + } + + // If somehow the user has tabbed out of the modal, push them to the first item. if (!this.modalTarget.contains(e.target)) { - const focusTarget = this.firstVisible(this.getAllTabbables()); + const focusTarget = this.firstVisible(tabbables); if (focusTarget) { e.preventDefault(); focusTarget.focus(); @@ -253,24 +266,20 @@ export class ModalController extends Stacks.StacksController { return; } - // If we observe a tab keydown and we're on an edge, cycle the focus to the other side. - if (e.key === "Tab") { - const tabbables = this.getAllTabbables(); - - const firstTabbable = this.firstVisible(tabbables); - const lastTabbable = this.lastVisible(tabbables); - - if (firstTabbable && lastTabbable) { - if (firstTabbable === lastTabbable) { - e.preventDefault(); - firstTabbable.focus(); - } else if (e.shiftKey && e.target === firstTabbable) { - e.preventDefault(); - lastTabbable.focus(); - } else if (!e.shiftKey && e.target === lastTabbable) { - e.preventDefault(); - firstTabbable.focus(); - } + // Keep focusable cycling within the modal by looping through elements within the modal + const firstTabbable = this.firstVisible(tabbables); + const lastTabbable = this.lastVisible(tabbables); + + if (firstTabbable && lastTabbable) { + if (firstTabbable === lastTabbable) { + e.preventDefault(); + firstTabbable.focus(); + } else if (e.shiftKey && e.target === firstTabbable) { + e.preventDefault(); + lastTabbable.focus(); + } else if (!e.shiftKey && e.target === lastTabbable) { + e.preventDefault(); + firstTabbable.focus(); } } } diff --git a/webpack.config.js b/webpack.config.js index 3566a9fbc1..2c9ca781a7 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -37,7 +37,7 @@ const baseConfig = (isProd, minify) => ({ { test: /\.less$/, use: [ - MiniCssExtractPlugin.loader, + isProd ? MiniCssExtractPlugin.loader : "", { loader: "css-loader", options: {