From eb7019fce4fb8b84321acc325cd61c1f407153dd Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 23 Sep 2026 19:42:46 +0200 Subject: [PATCH] :sparkles: Preserve sidebar scroll positions across tab switches (#11694) * :bug: Preserve layers panel scroll position across tab switches Fixes #7440. Switching between the Layers, Assets and Tokens tabs in the workspace left sidebar unmounts the active panel component, causing its scroll position to reset to the top on re-entry. Add a module-level `scroll-positions` atom keyed by page-id. The layers scroll handler now also saves the current scrollTop value into the atom; a `mf/with-effect` on the page-id dep restores it whenever the `layers-toolbox*` component mounts or the page changes. Co-Authored-By: Paperclip * :bug: Preserve sidebar scroll positions across tab switches Replace the Layers-only global atom with a scroll store held in a use-var in left-sidebar*, shared by the Layers, Assets and Tokens panels through a new sidebar.scroll helper. Positions are keyed per panel and page (or token set) and restore waits for list content to settle, so deep positions in lazily rendered lists survive tab switches. Closes #7440. AI-assisted-by: muse-spark-1.3-contributor * :bug: Add e2e coverage for sidebar scroll preservation Port the regression tests from closed PR #7544 for issue #7440, adapted to the current ref-based implementation and fixtures: async restore needs polled assertions, and setup uses the shared tokens helpers. Also add data-scroll-container hooks to the Assets and Tokens scroll containers so the specs can locate them. AI-assisted-by: muse-spark-1.3-contributor * :paperclip: Fix rebase issue --------- Co-authored-by: Sumit Ridhal Co-authored-by: Paperclip --- .../ui/specs/sidebar-scroll.spec.js | 129 ++++++++++++++++++ .../src/app/main/ui/workspace/sidebar.cljs | 15 +- .../app/main/ui/workspace/sidebar/assets.cljs | 17 ++- .../app/main/ui/workspace/sidebar/layers.cljs | 37 ++++- .../app/main/ui/workspace/sidebar/scroll.cljs | 61 +++++++++ .../app/main/ui/workspace/tokens/sidebar.cljs | 25 +++- frontend/test/frontend_tests/runner.cljs | 2 + .../ui/sidebar_scroll_test.cljs | 66 +++++++++ 8 files changed, 337 insertions(+), 15 deletions(-) create mode 100644 frontend/playwright/ui/specs/sidebar-scroll.spec.js create mode 100644 frontend/src/app/main/ui/workspace/sidebar/scroll.cljs create mode 100644 frontend/test/frontend_tests/ui/sidebar_scroll_test.cljs diff --git a/frontend/playwright/ui/specs/sidebar-scroll.spec.js b/frontend/playwright/ui/specs/sidebar-scroll.spec.js new file mode 100644 index 0000000000..1df484adfe --- /dev/null +++ b/frontend/playwright/ui/specs/sidebar-scroll.spec.js @@ -0,0 +1,129 @@ +import { test, expect } from "@playwright/test"; +import { BaseWebSocketPage } from "../pages/BaseWebSocketPage"; +import { WasmWorkspacePage } from "../pages/WasmWorkspacePage"; +import { setupTokensFileRender, unfoldTokenType } from "./tokens/helpers"; + +// Regression tests for issue #7440: scroll position in the left sidebar +// tabs (Layers / Assets / Tokens) must survive tab switches. +// Ported from closed PR #7544 and adapted to the current implementation +// (scroll store held in the sidebar + async restore on mount) and to the +// current fixtures and page objects. + +// We allow a small tolerance because content can shift slightly on remount. +const POSITION_TOLERANCE = 20; // pixels + +test.beforeEach(async ({ page }) => { + await WasmWorkspacePage.init(page); + await BaseWebSocketPage.mockRPC(page, "get-teams", "get-teams-tokens.json"); +}); + +async function visibleScrollContainers(page) { + const containers = page.locator( + '[data-testid="left-sidebar"] [data-scroll-container="true"]', + ); + const count = await containers.count(); + const visible = []; + for (let i = 0; i < count; i++) { + const el = containers.nth(i); + // eslint-disable-next-line no-await-in-loop + if (await el.evaluate((node) => !!node && node.offsetParent !== null)) { + visible.push(el); + } + } + return visible; +} + +async function scrollableContainers(page) { + const visible = await visibleScrollContainers(page); + const scrollable = []; + for (const el of visible) { + // eslint-disable-next-line no-await-in-loop + if ( + await el.evaluate((node) => node.scrollHeight > node.clientHeight + 100) + ) { + scrollable.push(el); + } + } + return scrollable; +} + +async function scrollToBottom(locator) { + await locator.evaluate((el) => { + el.scrollTop = el.scrollHeight; + }); + // Programmatic scrolls dispatch scroll events asynchronously; wait for + // the save handler before switching tabs. + await locator.page().waitForTimeout(150); +} + +async function expectScrollRestored(locator, expected) { + // Restore runs in a post-mount effect with frame retries, so poll + // instead of asserting immediately after the tab click. + await expect + .poll(async () => locator.evaluate((el) => el.scrollTop), { + timeout: 10000, + }) + .toBeGreaterThanOrEqual(expected - POSITION_TOLERANCE); +} + +test("Sidebar scroll position preserved when switching tabs", async ({ + page, +}) => { + const { tokensSidebar } = await setupTokensFileRender(page); + await unfoldTokenType(tokensSidebar, "color"); + + const containers = await scrollableContainers(page); + expect( + containers.length, + "Tokens tab should have a scrollable container", + ).toBeGreaterThan(0); + const scrollEl = containers[0]; + + await scrollToBottom(scrollEl); + const initialPos = await scrollEl.evaluate((el) => el.scrollTop); + expect(initialPos).toBeGreaterThan(0); + + await page.getByRole("tab", { name: "Assets" }).click(); + await page.getByRole("tab", { name: "Tokens" }).click(); + + const restored = (await scrollableContainers(page))[0]; + await expectScrollRestored(restored, initialPos); +}); + +test("Sidebar maintains independent scroll positions per tab", async ({ + page, +}) => { + const { tokensSidebar } = await setupTokensFileRender(page); + await unfoldTokenType(tokensSidebar, "color"); + + // Scroll Tokens to the bottom. + const tokensContainers = await scrollableContainers(page); + expect( + tokensContainers.length, + "Tokens tab should have a scrollable container", + ).toBeGreaterThan(0); + await scrollToBottom(tokensContainers[0]); + const tokensPos = await tokensContainers[0].evaluate((el) => el.scrollTop); + expect(tokensPos).toBeGreaterThan(0); + + // Switch to Layers: fresh panel, must start at top (not contaminated + // by the Tokens position saved under a different key). + await page.getByRole("tab", { name: "Layers" }).click(); + const layersVisible = await visibleScrollContainers(page); + expect(layersVisible.length).toBeGreaterThan(0); + const layersPos = await layersVisible[0].evaluate((el) => el.scrollTop); + expect(layersPos).toBe(0); + + // Back to Tokens: its own position must be restored, not Layers'. + + // Back to Tokens: its own position must be restored, not Layers'. + await page.getByRole("tab", { name: "Tokens" }).click(); + const tokensRestored = (await scrollableContainers(page))[0]; + await expectScrollRestored(tokensRestored, tokensPos); + + // And back to Layers: still untouched at top. + await page.getByRole("tab", { name: "Layers" }).click(); + const layersAgain = await visibleScrollContainers(page); + const layersAgainPos = await layersAgain[0].evaluate((el) => el.scrollTop); + expect(layersAgainPos).toBe(0); +}); diff --git a/frontend/src/app/main/ui/workspace/sidebar.cljs b/frontend/src/app/main/ui/workspace/sidebar.cljs index 899624002b..c8a0ece18e 100644 --- a/frontend/src/app/main/ui/workspace/sidebar.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar.cljs @@ -79,7 +79,7 @@ (mf/defc layers-content* {::mf/private true ::mf/memo true} - [{:keys [width layout]}] + [{:keys [width layout scroll-store]}] (let [{on-pointer-down :on-pointer-down on-lost-pointer-capture :on-lost-pointer-capture on-pointer-move :on-pointer-move @@ -112,7 +112,8 @@ [:div {:class (stl/css :layers-tab-resize-handle)}]]) - [:> layers-toolbox* {:size-parent width}]])) + [:> layers-toolbox* {:size-parent width + :scroll-store scroll-store}]])) (mf/defc left-sidebar* @@ -121,6 +122,7 @@ (let [options-mode (mf/deref refs/options-mode-global) project (mf/deref refs/project) file-id (get file :id) + scroll-store* (mf/use-var {}) design-tokens? (features/use-feature "design-tokens/v1") mode-inspect? (= options-mode :inspect) @@ -213,18 +215,21 @@ (case section :assets [:> assets-toolbox* {:size (- width 58) - :file-id file-id}] + :file-id file-id + :scroll-store scroll-store*}] :tokens [:> tokens-sidebar-tab* {:tokens-lib tokens-lib :tokens-status tokens-status :active-tokens active-tokens - :resolved-active-tokens resolved-active-tokens}] + :resolved-active-tokens resolved-active-tokens + :scroll-store scroll-store*}] :layers [:> layers-content* {:layout layout - :width width}])]])]])) + :width width + :scroll-store scroll-store*}])]])]])) ;; --- Right Sidebar (Component) diff --git a/frontend/src/app/main/ui/workspace/sidebar/assets.cljs b/frontend/src/app/main/ui/workspace/sidebar/assets.cljs index 3c196b1986..9650b235f3 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/assets.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/assets.cljs @@ -22,6 +22,7 @@ [app.main.ui.icons :as deprecated-icon] [app.main.ui.workspace.sidebar.assets.common :as cmm] [app.main.ui.workspace.sidebar.assets.file-library :refer [file-library*]] + [app.main.ui.workspace.sidebar.scroll :as sc] [app.util.dom :as dom] [app.util.i18n :as i18n :refer [tr]] [cuerdas.core :as str] @@ -89,8 +90,9 @@ (mf/defc assets-toolbox* {::mf/wrap [mf/memo]} - [{:keys [size file-id]}] + [{:keys [size file-id scroll-store]}] (let [read-only? (mf/use-ctx ctx/workspace-read-only?) + assets-ref (mf/use-ref nil) filters* (mf/use-state (fn [] (-> (or (get @session-filters* file-id) @@ -150,6 +152,12 @@ (fn [] (modal/show! :libraries-dialog {:file-id file-id}))) + on-scroll-save + (mf/use-fn + (mf/deps file-id) + (fn [event] + (sc/save-scroll! scroll-store [:assets file-id] event))) + on-open-menu (mf/use-fn #(swap! filters* update :open-menu not)) @@ -184,7 +192,12 @@ (mf/with-effect [file-id term section] (swap! session-filters* assoc file-id {:term term :section section})) - [:article {:class (stl/css :assets-bar)} + (sc/use-restore-scroll scroll-store :assets file-id assets-ref) + + [:article {:class (stl/css :assets-bar) + :data-scroll-container true + :on-scroll on-scroll-save + :ref assets-ref} [:div {:class (stl/css :assets-header)} (when-not ^boolean read-only? (if (and (= num-libs 1) (empty? components) (not shared?)) diff --git a/frontend/src/app/main/ui/workspace/sidebar/layers.cljs b/frontend/src/app/main/ui/workspace/sidebar/layers.cljs index aa316161ea..cdf48df939 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/layers.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/layers.cljs @@ -26,6 +26,7 @@ [app.main.ui.hooks :as hooks] [app.main.ui.notifications.badge :refer [badge-notification]] [app.main.ui.workspace.sidebar.layer-item :refer [layer-item*]] + [app.main.ui.workspace.sidebar.scroll :as sc] [app.util.dom :as dom] [app.util.globals :as globals] [app.util.i18n :as i18n :refer [tr]] @@ -799,7 +800,7 @@ (mf/defc layers-toolbox* {::mf/wrap [mf/memo]} - [{:keys [size-parent]}] + [{:keys [size-parent scroll-store]}] (let [page (mf/deref refs/workspace-page) page-id (get page :id) @@ -811,6 +812,8 @@ observer-var (mf/use-var nil) lazy-load-ref (mf/use-ref nil) + tree-ref (mf/use-ref nil) + search-ref (mf/use-ref nil) [filtered-objects show-more filter-component] (use-search page objects) @@ -834,10 +837,33 @@ (do (.disconnect ^js @observer-var) (reset! observer-var nil))))) + ;; The search-results container reuses the lazy-load observer root + ;; and additionally tracks its node for scroll restore. + on-render-search-container + (fn [element] + (mf/set-ref-val! search-ref element) + (on-render-container element)) + + on-scroll-with-save + (mf/use-fn + (mf/deps page-id) + (fn [event] + (sc/save-scroll! scroll-store [:layers page-id] event) + (on-scroll event))) + + on-search-scroll + (mf/use-fn + (mf/deps page-id) + (fn [event] + (sc/save-scroll! scroll-store [:layers-search page-id] event))) + toogle-focus-mode (mf/use-fn #(st/emit! (dw/toggle-focus-mode)))] + (sc/use-restore-scroll scroll-store :layers page-id tree-ref) + (sc/use-restore-scroll scroll-store :layers-search page-id search-ref) + [:div {:id "layers" :class (stl/css :layers) :data-testid "layer-tree"} @@ -863,13 +889,15 @@ [:* [:div {:class (stl/css :tool-window-content) :data-scroll-container true - :ref on-render-container} + :on-scroll on-search-scroll + :ref on-render-search-container} [:> filters-tree* {:objects filtered-objects :key (dm/str page-id) :parent-size size-parent}] [:div {:ref lazy-load-ref}]] - [:div {:on-scroll on-scroll + [:div {:on-scroll on-scroll-with-save + :ref tree-ref :class (stl/css :tool-window-content) :data-scroll-container true :style {:display (when (some? filtered-objects) "none")}} @@ -878,7 +906,8 @@ :is-filtered true :parent-size size-parent}]]] - [:div {:on-scroll on-scroll + [:div {:on-scroll on-scroll-with-save + :ref tree-ref :class (stl/css :tool-window-content) :data-scroll-container true :style {:display (when (some? filtered-objects) "none")}} diff --git a/frontend/src/app/main/ui/workspace/sidebar/scroll.cljs b/frontend/src/app/main/ui/workspace/sidebar/scroll.cljs new file mode 100644 index 0000000000..7bcaa73bef --- /dev/null +++ b/frontend/src/app/main/ui/workspace/sidebar/scroll.cljs @@ -0,0 +1,61 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns app.main.ui.workspace.sidebar.scroll + "Scroll position save/restore for sidebar panels. + + Panels unmount on every tab switch, so positions that must survive it + cannot live in component state. The store is created once in + `left-sidebar*` (which stays mounted while tabs swap, unlike + `layers-content*` and the toolboxes) with `mf/use-var` and threaded to + the panels as a prop. Shape: {[panel id] scroll-top}." + (:require + [app.util.dom :as dom] + [app.util.timers :as tm] + [rumext.v2 :as mf])) + +(def ^:private max-restore-attempts + 10) + +(defn save-scroll! + "Saves the scrollTop of the scroll event target into the store under k. + Plain swap!, never triggers renders. Missing target is a no-op." + [store* k event] + (when-let [target (dom/get-target event)] + (swap! store* assoc k (.-scrollTop target)))) + +(defn needs-restore-retry? + "Pure decision for the restore loop: keep retrying while the content + height is still changing (lazily rendered lists) and attempts remain." + [prev-height cur-height attempts] + (and (not= prev-height cur-height) + (< attempts max-restore-attempts))) + +(defn restore-scroll! + [node saved] + (set! (.-scrollTop node) saved)) + +(defn- restore-loop! + [node saved prev-height attempts raf*] + (restore-scroll! node saved) + (let [height (.-scrollHeight node)] + (when (needs-restore-retry? prev-height height attempts) + (vreset! raf* (tm/raf #(restore-loop! node saved height (inc attempts) raf*)))))) + +(defn use-restore-scroll + "Restores the scrollTop saved under [panel id] on mount and whenever + panel or id change. Retries while the content height keeps changing so + deep positions in lazily rendered lists are not clamped to the first + chunk. Missing key or node is a silent no-op." + [store* panel id node-ref] + (mf/with-effect [panel id] + (let [raf* (volatile! nil)] + (when-let [node (mf/ref-val node-ref)] + (when-let [saved (get @store* [panel id])] + (restore-loop! node saved -1 0 raf*))) + (fn [] + (when-some [raf @raf*] + (tm/cancel-af! raf)))))) diff --git a/frontend/src/app/main/ui/workspace/tokens/sidebar.cljs b/frontend/src/app/main/ui/workspace/tokens/sidebar.cljs index d14f4913cc..c6e63e605f 100644 --- a/frontend/src/app/main/ui/workspace/tokens/sidebar.cljs +++ b/frontend/src/app/main/ui/workspace/tokens/sidebar.cljs @@ -21,6 +21,7 @@ [app.main.ui.ds.foundations.assets.icon :as i] [app.main.ui.hooks :as h] [app.main.ui.hooks.resize :refer [use-resize-hook]] + [app.main.ui.workspace.sidebar.scroll :as sc] [app.main.ui.workspace.tokens.management :refer [tokens-section*]] [app.main.ui.workspace.tokens.sets :as tsets] [app.main.ui.workspace.tokens.sets.context-menu :refer [token-set-context-menu*]] @@ -150,13 +151,12 @@ :on-click open-settings-modal}])])) (mf/defc tokens-sidebar-tab* - [{:keys [] :as props}] + [{:keys [scroll-store] :as props}] (let [{on-pointer-down-pages :on-pointer-down on-lost-pointer-capture-pages :on-lost-pointer-capture on-pointer-move-pages :on-pointer-move size-pages-opened :size} (use-resize-hook :tokens 200 38 "0.6" :y false nil) - current-file-data (mf/deref refs/workspace-data) @@ -166,7 +166,21 @@ can-edit-tokens? (mf/with-memo [can-edit-file? current-file-data] (and can-edit-file? - (cfo/editable-tokens? current-file-data)))] + (cfo/editable-tokens? current-file-data))) + + set-id + (mf/deref refs/selected-token-set-id) + + tokens-ref + (mf/use-ref nil) + + on-scroll-save + (mf/use-fn + (mf/deps set-id) + (fn [event] + (sc/save-scroll! scroll-store [:tokens set-id] event)))] + + (sc/use-restore-scroll scroll-store :tokens set-id tokens-ref) [:> (mf/provider ctx/can-edit-tokens?) {:value can-edit-tokens?} [:div {:class (stl/css :sidebar-wrapper)} @@ -174,7 +188,10 @@ {:resize-height size-pages-opened :current-file-data current-file-data}] [:article {:class (stl/css :tokens-section-wrapper) - :data-testid "tokens-sidebar"} + :data-testid "tokens-sidebar" + :data-scroll-container true + :on-scroll on-scroll-save + :ref tokens-ref} [:div {:class (stl/css :resize-area-horiz) :on-pointer-down on-pointer-down-pages :on-lost-pointer-capture on-lost-pointer-capture-pages diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 78e7b11f20..d7eea49a9a 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -104,6 +104,7 @@ [frontend-tests.ui.routes-test] [frontend-tests.ui.settings-password-schema-test] [frontend-tests.ui.settings-shortcuts-test] + [frontend-tests.ui.sidebar-scroll-test] [frontend-tests.util-clipboard-test] [frontend-tests.util-object-test] [frontend-tests.util-queue-test] @@ -228,6 +229,7 @@ 'frontend-tests.text-editor-paste-guard-test 'frontend-tests.ui.settings-password-schema-test 'frontend-tests.ui.settings-shortcuts-test + 'frontend-tests.ui.sidebar-scroll-test 'frontend-tests.util-clipboard-test 'frontend-tests.util-object-test 'frontend-tests.util-queue-test diff --git a/frontend/test/frontend_tests/ui/sidebar_scroll_test.cljs b/frontend/test/frontend_tests/ui/sidebar_scroll_test.cljs new file mode 100644 index 0000000000..9cf7bd8bcc --- /dev/null +++ b/frontend/test/frontend_tests/ui/sidebar_scroll_test.cljs @@ -0,0 +1,66 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.ui.sidebar-scroll-test + (:require + [app.main.ui.workspace.sidebar.scroll :as sc] + [app.util.timers :as tm] + [cljs.test :as t :include-macros true])) + +(def ^:private restore-loop! @#'sc/restore-loop!) + +(defn- scroll-event + [scroll-top] + #js {:target #js {:scrollTop scroll-top}}) + +(t/deftest save-scroll-stores-position-per-key + (let [store (atom {})] + (sc/save-scroll! store [:layers :page-1] (scroll-event 250)) + (sc/save-scroll! store [:assets :file-1] (scroll-event 80)) + (t/is (= {[:layers :page-1] 250 + [:assets :file-1] 80} + @store)))) + +(t/deftest save-scroll-ignores-events-without-target + (let [store (atom {})] + (sc/save-scroll! store [:layers :page-1] nil) + (sc/save-scroll! store [:layers :page-1] #js {}) + (t/is (= {} @store)))) + +(t/deftest restore-scroll-writes-saved-position + (let [node #js {:scrollTop 0}] + (sc/restore-scroll! node 320) + (t/is (= 320 (.-scrollTop node))))) + +(t/deftest needs-restore-retry + (t/is (true? (sc/needs-restore-retry? -1 300 0))) + (t/is (true? (sc/needs-restore-retry? 100 300 9))) + (t/is (false? (sc/needs-restore-retry? 300 300 0))) + (t/is (false? (sc/needs-restore-retry? 100 300 10)))) + +(t/deftest restore-loop-writes-once-when-content-settled + (let [node #js {:scrollTop 0 :scrollHeight 300} + scheduled (atom []) + raf* (volatile! nil)] + (with-redefs [tm/raf (fn [f] (swap! scheduled conj f) 1)] + (restore-loop! node 250 300 0 raf*) + (t/is (= 250 (.-scrollTop node))) + (t/is (empty? @scheduled) "no follow-up when height is stable") + (t/is (nil? @raf*))))) + +(t/deftest restore-loop-retries-while-content-grows + (let [node #js {:scrollTop 0 :scrollHeight 300} + scheduled (atom []) + raf* (volatile! nil)] + (with-redefs [tm/raf (fn [f] (swap! scheduled conj f) 1)] + (restore-loop! node 250 -1 0 raf*) + (t/is (= 250 (.-scrollTop node))) + (t/is (= 1 (count @scheduled)) "follow-up scheduled while growing") + (t/is (= 1 @raf*) "frame id tracked for cancellation") + ;; Next frame: content settled at the same height, retry stops. + ((first @scheduled)) + (t/is (= 250 (.-scrollTop node))) + (t/is (= 1 (count @scheduled)) "no further frames once settled"))))