SPARK-8: Fix Modal Focus Issue when using Keyboard (#1963)

* fix reported focus issue

* Also handle case where modal has no tabbable elements

* fix webpack.config.js for windows

* move if check up

* Create loud-suits-stare.md

* fix whitespace

* revert html changes

* revert harder

* add tests

* lint

Author: mukunku

.changeset/loud-suits-stare.md

@@ -0,0 +1,5 @@
+---
+"@stackoverflow/stacks": patch
+---
+
+SPARK-8: Fix Modal text selection issue when using keyboard

docs/product/components/modals.html

@@ -419,4 +419,4 @@ <h1 class="s-modal--header" id="example-modal-celebratory-title">Congratulations
 </section>
 
 <!-- Example modals javascript -->
-<script src="{{ "/assets/dist/entry.modals.js" | url }}" defer></script>
+<script src="{{ "/assets/dist/entry.modals.js" | url }}" defer></script>

lib/components/modal/modal.test.ts

@@ -8,8 +8,15 @@ const user = userEvent.setup();
 const createModal = ({
     hidden = true,
     initialFocusEl,
-}: { hidden?: boolean; initialFocusEl?: ReturnType<typeof html> } = {}) => html`
-    <div data-controller="s-modal">
+    renderFocusables = true,
+    excludeTabIndex = false,
+}: {
+    hidden?: boolean;
+    initialFocusEl?: ReturnType<typeof html>;
+    renderFocusables?: boolean;
+    excludeTabIndex?: boolean;
+} = {}) => html`
+    <div data-controller="s-modal" data-testid="controller">
         <button
             class="s-btn"
             data-action="s-modal#show"
@@ -20,7 +27,7 @@ const createModal = ({
         <aside
             class="s-modal"
             id="modal-base"
-            tabindex="-1"
+            tabindex="${!excludeTabIndex ? -1 : null}"
             role="dialog"
             aria-labelledby="modal-base-title"
             aria-describedby="modal-base-description"
@@ -31,26 +38,52 @@ const createModal = ({
                 <h1 class="s-modal--header" id="modal-base-title">Title</h1>
 
                 <p class="s-modal--body">
-                    <span id="modal-base-description">Description</span>
+                    <span id="modal-base-description" data-testid="modal-text-body">Description</span>
                     <form>
-                        <input type="text" data-testid="first-focusable-element" />
-                        ${initialFocusEl}
+                        ${
+                            renderFocusables
+                                ? html`
+                                      <input
+                                          type="text"
+                                          data-testid="first-focusable-element"
+                                      />
+                                      ${initialFocusEl}
+                                  `
+                                : null
+                        }
                     </form>
                 </p>
-
-                <div class="d-flex gx8 s-modal--footer">
-                    <button class="flex--item s-btn s-btn__filled" type="button">Save changes</button>
-                    <button class="flex--item s-btn" type="button" data-action="s-modal#hide">Cancel</button>
-                </div>
-
-                <button
-                    class="s-btn s-btn__muted s-modal--close"
-                    type="button"
-                    aria-label="Close"
-                    data-action="s-modal#hide"
-                    data-testid="close-btn">
-                    Close
-                </button>
+                ${
+                    renderFocusables
+                        ? html` <div class="d-flex gx8 s-modal--footer">
+                                  <button
+                                      class="flex--item s-btn s-btn__filled"
+                                      type="button"
+                                      data-testid="save-btn"
+                                  >
+                                      Save changes
+                                  </button>
+                                  <button
+                                      class="flex--item s-btn"
+                                      type="button"
+                                      data-action="s-modal#hide"
+                                      data-testid="cancel-btn"
+                                  >
+                                      Cancel
+                                  </button>
+                              </div>
+
+                              <button
+                                  class="s-btn s-btn__muted s-modal--close"
+                                  type="button"
+                                  aria-label="Close"
+                                  data-action="s-modal#hide"
+                                  data-testid="close-btn"
+                              >
+                                  Close
+                              </button>`
+                        : null
+                }
             </div>
         </aside>
     </div>
@@ -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<void> {
+            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;
+    });
 });

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(<Element>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();
             }
         }
     }

webpack.config.js

@@ -37,7 +37,7 @@ const baseConfig = (isProd, minify) => ({
             {
                 test: /\.less$/,
                 use: [
-                    MiniCssExtractPlugin.loader,
+                    isProd ? MiniCssExtractPlugin.loader : "",
                     {
                         loader: "css-loader",
                         options: {