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"))))