Merge pull request #1176 from Patternslib/scrum-1455--scroll

Fix for "unwanted scrolling when inside collapsible"

Author: thet

src/core/dom.js

@@ -529,6 +529,34 @@ const escape_css_id = (id) => {
     return `#${CSS.escape(id.split("#")[1])}`;
 };
 
+/**
+ * Get a universally unique id (uuid) for a DOM element.
+ *
+ * This method returns a uuid for the given element. On the first call it will
+ * generate a uuid and store it on the element.
+ *
+ * @param {Node} el - The DOM node to get the uuid for.
+ * @returns {String} - The uuid.
+ */
+const element_uuid = (el) => {
+    if (!get_data(el, "uuid", false)) {
+        let uuid;
+        if (window.crypto.randomUUID) {
+            // Create a real UUID
+            // window.crypto.randomUUID does only exist in browsers with secure
+            // context.
+            // See: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
+            uuid = window.crypto.randomUUID();
+        } else {
+            // Create a sufficiently unique ID
+            const array = new Uint32Array(4);
+            uuid = window.crypto.getRandomValues(array).join("");
+        }
+        set_data(el, "uuid", uuid);
+    }
+    return get_data(el, "uuid");
+};
+
 const dom = {
     toNodeArray: toNodeArray,
     querySelectorAllAndMe: querySelectorAllAndMe,
@@ -556,6 +584,7 @@ const dom = {
     template: template,
     get_visible_ratio: get_visible_ratio,
     escape_css_id: escape_css_id,
+    element_uuid: element_uuid,
     add_event_listener: events.add_event_listener, // BBB export. TODO: Remove in an upcoming version.
     remove_event_listener: events.remove_event_listener, // BBB export. TODO: Remove in an upcoming version.
 };

src/core/dom.test.js

@@ -874,3 +874,32 @@ describe("escape_css_id", function () {
         expect(dom.escape_css_id("#-1-2-3")).toBe("#-\\31 -2-3");
     });
 });
+
+describe("element_uuid", function () {
+    it("returns a UUIDv4 for an element", function () {
+        const el = document.createElement("div");
+        const uuid = dom.element_uuid(el);
+        expect(uuid).toMatch(
+            /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/
+        );
+
+        // The UUID isn't created anew when called again.
+        expect(dom.element_uuid(el)).toBe(uuid);
+    });
+
+    it("returns a sufficiently unique id for an element", function () {
+        // Mock window.crypto.randomUUID not existing, like in browser with
+        // non-secure context.
+        const orig_randomUUID = window.crypto.randomUUID;
+        window.crypto.randomUUID = undefined;
+
+        const el = document.createElement("div");
+        const uuid = dom.element_uuid(el);
+        expect(uuid).toMatch(/^[0-9]*$/);
+
+        // The UUID isn't created anew when called again.
+        expect(dom.element_uuid(el)).toBe(uuid);
+
+        window.crypto.randomUUID = orig_randomUUID;
+    });
+});

src/pat/collapsible/documentation.md

@@ -170,4 +170,4 @@ attribute. The available options are:
 | `effect-duration` | `fast`    | Duration of transition. This is ignored if the transition is `none` or `css`.                                                                                        |
 | `effect-easing`   | `swing`   | Easing to use for the open/close animation. This must be a known jQuery easing method. jQuery includes `swing` and `linear`, but more can be included via jQuery UI. |
 | `scroll-selector` |           | CSS selector, `self` or `none`. Defines which element will be scrolled into view. `self` if it is the collapsible element itself. `none` to disable scrolling if a scrolling selector is inherited from a parent pat-collapsible element. |
-| `scroll-offset`   |           | `offset` in pixels to stop scrolling before the target position defines by `scroll-selector`. Can also be a negative number.                                         |
+| `scroll-offset`   |           | `offset` in pixels to stop scrolling before the target position defined by `scroll-selector`. Can also be a negative number.                                         |

src/pat/scroll/scroll.js

@@ -1,4 +1,3 @@
-import $ from "jquery";
 import { BasePattern } from "../../core/basepattern";
 import dom from "../../core/dom";
 import events from "../../core/events";
@@ -36,15 +35,6 @@ class Pattern extends BasePattern {
         if (this.options.trigger === "auto" || this.options.trigger === "click") {
             this.el.addEventListener("click", this.scrollTo.bind(this));
         }
-        $(this.el).on("pat-update", this.onPatternsUpdate.bind(this));
-    }
-
-    onPatternsUpdate(ev, data) {
-        if (data?.pattern === "stacks") {
-            if (data.originalEvent && data.originalEvent.type === "click") {
-                this.scrollTo();
-            }
-        }
     }
 
     get_target() {

src/pat/scroll/scroll.test.js

@@ -1,4 +1,3 @@
-import $ from "jquery";
 import Pattern from "./scroll";
 import events from "../../core/events";
 import utils from "../../core/utils";
@@ -89,25 +88,7 @@ describe("pat-scroll", function () {
         expect(this.spy_scrollTo).toHaveBeenCalled();
     });
 
-    it("5 - will scroll to an anchor on pat-update with originalEvent of click", async function () {
-        document.body.innerHTML = `
-            <a href="#p1" class="pat-scroll" data-pat-scroll="trigger: click">p1</a>
-            <p id="p1"></p>
-        `;
-        const $el = $(".pat-scroll");
-
-        const instance = new Pattern($el[0]);
-        await events.await_pattern_init(instance);
-        $el.trigger("pat-update", {
-            pattern: "stacks",
-            originalEvent: {
-                type: "click",
-            },
-        });
-        expect(this.spy_scrollTo).toHaveBeenCalled();
-    });
-
-    it("6 - will allow for programmatic scrolling with trigger set to 'manual'", async function () {
+    it("5 - will allow for programmatic scrolling with trigger set to 'manual'", async function () {
         document.body.innerHTML = `
             <a href="#p1" class="pat-scroll" data-pat-scroll="trigger: manual">p1</a>
             <p id="p1"></p>
@@ -124,7 +105,7 @@ describe("pat-scroll", function () {
         expect(this.spy_scrollTo).toHaveBeenCalled();
     });
 
-    it("7 - will scroll to bottom with selector:bottom", async function () {
+    it("6 - will scroll to bottom with selector:bottom", async function () {
         document.body.innerHTML = `
             <div id="scroll-container" style="overflow-y: scroll">
               <button class="pat-scroll" data-pat-scroll="selector: bottom; trigger: manual">to bottom</button>
@@ -156,7 +137,7 @@ describe("pat-scroll", function () {
         expect(container.scrollTop).toBe(1000);
     });
 
-    it("8 - will add an offset to the scroll position", async function () {
+    it("7 - will add an offset to the scroll position", async function () {
         // Testing with `selector: top`, as this just sets scrollTop to 0
 
         document.body.innerHTML = `
@@ -186,7 +167,7 @@ describe("pat-scroll", function () {
         expect(container.scrollTop).toBe(-40);
     });
 
-    it("9 - will adds a negative offset to scroll position", async function () {
+    it("8 - will adds a negative offset to scroll position", async function () {
         // Testing with `selector: top`, as this just sets scrollTop to 0
 
         document.body.innerHTML = `
@@ -215,7 +196,7 @@ describe("pat-scroll", function () {
         expect(container.scrollTop).toBe(40);
     });
 
-    it("10 - handles different selector options.", async function () {
+    it("9 - handles different selector options.", async function () {
         document.body.innerHTML = `
             <a href="#el3" class="pat-scroll">scroll</a>
             <div id="el1"></div>

src/pat/stacks/documentation.md

@@ -45,3 +45,5 @@ The Stacks pattern may be configured through a `data-pat-stacks` attribute. The
 | `transition`      | `none`  | Transition effect to use. Must be one of `none`, `css`, `fade` or `slide`.                                                                                 |
 | `effect-duration` | `fast`  | Duration of transition. This is ignored if the transition is `none` or `css`.                                                                              |
 | `effect-easing`   | `swing` | Easing to use for the transition. This must be a known jQuery easing method. jQuery includes `swing` and `linear`, but more can be included via jQuery UI. |
+| `scroll-selector` |         | CSS selector, `self` or `none`. Defines which element will be scrolled into view. `self` if it is the stacks content element itself. `none` to disable scrolling if a scrolling selector is inherited from a parent pat-stacks element. |
+| `scroll-offset`   |         | `offset` in pixels to stop scrolling before the target position defined by `scroll-selector`. Can also be a negative number.                               |

src/pat/stacks/stacks.js

@@ -1,7 +1,8 @@
 import $ from "jquery";
 import { BasePattern } from "../../core/basepattern";
-import Parser from "../../core/parser";
+import events from "../../core/events";
 import logging from "../../core/logging";
+import Parser from "../../core/parser";
 import registry from "../../core/registry";
 import utils from "../../core/utils";
 
@@ -12,19 +13,47 @@ parser.addArgument("selector", "> *[id]");
 parser.addArgument("transition", "none", ["none", "css", "fade", "slide"]);
 parser.addArgument("effect-duration", "fast");
 parser.addArgument("effect-easing", "swing");
+// pat-scroll support
+parser.addArgument("scroll-selector");
+parser.addArgument("scroll-offset", 0);
+
+const debounce_scroll_timer = { timer: null };
 
 class Pattern extends BasePattern {
     static name = "stacks";
     static trigger = ".pat-stacks";
     static parser = parser;
     document = document;
 
-    init() {
+    async init() {
         this.$el = $(this.el);
+
+        // pat-scroll support
+        if (this.options.scroll?.selector && this.options.scroll.selector !== "none") {
+            const Scroll = (await import("../scroll/scroll")).default;
+            this.scroll = new Scroll(this.el, {
+                trigger: "manual",
+                selector: this.options.scroll.selector,
+                offset: this.options.scroll?.offset,
+            });
+            await events.await_pattern_init(this.scroll);
+
+            // scroll debouncer for later use.
+            this.debounce_scroll = utils.debounce(
+                this.scroll.scrollTo.bind(this.scroll),
+                10,
+                debounce_scroll_timer
+            );
+        }
+
         this._setupStack();
         $(this.document).on("click", "a", this._onClick.bind(this));
     }
 
+    destroy() {
+        $(this.document).off("click", "a", this._onClick.bind(this));
+    }
+
     _setupStack() {
         let selected = this._currentFragment();
         const $sheets = this.$el.find(this.options.selector);
@@ -72,12 +101,16 @@ class Pattern extends BasePattern {
         if (base_url !== href_parts[0] || !href_parts[1]) {
             return;
         }
-        if (!this.$el.has("#" + href_parts[1]).length) {
+        if (!this.$el.has(`#${href_parts[1]}`).length) {
             return;
         }
         e.preventDefault();
         this._updateAnchors(href_parts[1]);
         this._switch(href_parts[1]);
+
+        this.debounce_scroll?.(); // debounce scroll, if available.
+
+        // Notify other patterns
         $(e.target).trigger("pat-update", {
             pattern: "stacks",
             action: "attribute-changed",
@@ -89,20 +122,20 @@ class Pattern extends BasePattern {
     _updateAnchors(selected) {
         const $sheets = this.$el.find(this.options.selector);
         const base_url = this._base_URL();
-        $sheets.each(function (idx, sheet) {
+        for (const sheet of $sheets) {
             // This may appear odd, but: when querying a browser uses the
             // original href of an anchor as it appeared in the document
             // source, but when you access the href property you always get
             // the fully qualified version.
-            var $anchors = $(
-                'a[href="' + base_url + "#" + sheet.id + '"],a[href="#' + sheet.id + '"]'
+            const $anchors = $(
+                `a[href="${base_url}#${sheet.id}"], a[href="#${sheet.id}"]`
             );
             if (sheet.id === selected) {
                 $anchors.addClass("current");
             } else {
                 $anchors.removeClass("current");
             }
-        });
+        }
     }
 
     _switch(sheet_id) {

src/pat/stacks/stacks.test.js

@@ -1,6 +1,7 @@
 import $ from "jquery";
 import events from "../../core/events";
 import Stacks from "./stacks";
+import utils from "../../core/utils";
 import { jest } from "@jest/globals";
 
 describe("pat-stacks", function () {
@@ -164,4 +165,65 @@ describe("pat-stacks", function () {
             expect($("#l2").hasClass("current")).toBe(true);
         });
     });
+
+    describe("5 - Scrolling support.", function () {
+        beforeEach(function () {
+            document.body.innerHTML = "";
+            this.spy_scrollTo = jest
+                .spyOn(window, "scrollTo")
+                .mockImplementation(() => null);
+        });
+
+        afterEach(function () {
+            this.spy_scrollTo.mockRestore();
+        });
+
+        it("5.1 - Scrolls to self.", async function () {
+            document.body.innerHTML = `
+                <a href='#s51'>1</a>
+                <div class="pat-stacks" data-pat-stacks="scroll-selector: self">
+                    <section id="s51">
+                    </section>
+                </div>
+            `;
+            const el = document.querySelector(".pat-stacks");
+
+            const instance = new Stacks(el);
+            await events.await_pattern_init(instance);
+
+            const s51 = document.querySelector("[href='#s51']");
+            $(s51).click();
+            await utils.timeout(10);
+
+            expect(this.spy_scrollTo).toHaveBeenCalled();
+        });
+
+        it("5.2 - Does clear scroll setting from parent config.", async function () {
+            // NOTE: We give the stack section a different id.
+            // The event handler which is registered on the document in the
+            // previous test is still attached. Two event handlers are run when
+            // clicking here and if the anchor-targets would have the same id
+            // the scrolling would happen as it was set up in the previous
+            // test.
+            document.body.innerHTML = `
+                <div data-pat-stacks="scroll-selector: self">
+                    <a href='#s52'>1</a>
+                    <div class="pat-stacks" data-pat-stacks="scroll-selector: none">
+                        <section id="s52">
+                        </section>
+                    </div>
+                </div>
+            `;
+            const el = document.querySelector(".pat-stacks");
+
+            const instance = new Stacks(el);
+            await events.await_pattern_init(instance);
+
+            const s52 = document.querySelector("[href='#s52']");
+            $(s52).click();
+            await utils.timeout(10);
+
+            expect(this.spy_scrollTo).not.toHaveBeenCalled();
+        });
+    });
 });

src/setup-tests.js

@@ -22,3 +22,7 @@ dom.is_visible = (el) => {
 
 // polyfill css.escape for jsdom
 import("css.escape");
+
+// NodeJS polyfill for window.crypto.randomUUID
+import crypto from "crypto";
+window.crypto.randomUUID = () => crypto.randomUUID();