From 30e1e3758c23307fb602f399b3f508df92d10e3f Mon Sep 17 00:00:00 2001 From: "Adrien Minne (adrm)" Date: Fri, 13 Dec 2024 15:10:14 +0100 Subject: [PATCH] [FIX] spreadsheet: avoid parasitic renders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking on a button on the topbar would trigger 3 renders: - one on model update (expected) - one because we received a collaborative event for our own revision (useless) - one because the onClick of the topbar calls `DOMFocusableElementStore.focus`, which triggers a render because a store method was called (useless) closes odoo/o-spreadsheet#5346 Task: 4373054 Signed-off-by: Rémi Rahir (rar) --- src/collaborative/session.ts | 3 +++ .../bottom_bar_sheet/bottom_bar_sheet.ts | 4 ++-- .../composer/grid_composer/grid_composer.ts | 2 +- src/components/grid/grid.ts | 4 ++-- src/stores/DOM_focus_store.ts | 8 ++----- tests/bottom_bar/bottom_bar_component.test.ts | 10 +++++++-- tests/top_bar_component.test.ts | 21 ++++++++++++++++++- 7 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/collaborative/session.ts b/src/collaborative/session.ts index ffec9be08f..2eb36a8934 100644 --- a/src/collaborative/session.ts +++ b/src/collaborative/session.ts @@ -319,6 +319,9 @@ export class Session extends EventBus { } } this.acknowledge(message); + if (message.type === "REMOTE_REVISION" && message.clientId === this.clientId) { + return; + } this.trigger("collaborative-event-received"); } diff --git a/src/components/bottom_bar_sheet/bottom_bar_sheet.ts b/src/components/bottom_bar_sheet/bottom_bar_sheet.ts index eb2c84131c..8bd5d03e56 100644 --- a/src/components/bottom_bar_sheet/bottom_bar_sheet.ts +++ b/src/components/bottom_bar_sheet/bottom_bar_sheet.ts @@ -157,11 +157,11 @@ export class BottomBarSheet extends Component { if (ev.key === "Enter") { ev.preventDefault(); this.stopEdition(); - this.DOMFocusableElementStore.focus(); + this.DOMFocusableElementStore.focusableElement?.focus(); } if (ev.key === "Escape") { this.cancelEdition(); - this.DOMFocusableElementStore.focus(); + this.DOMFocusableElementStore.focusableElement?.focus(); } } diff --git a/src/components/composer/grid_composer/grid_composer.ts b/src/components/composer/grid_composer/grid_composer.ts index 34ce6e26df..59ec0890d3 100644 --- a/src/components/composer/grid_composer/grid_composer.ts +++ b/src/components/composer/grid_composer/grid_composer.ts @@ -182,7 +182,7 @@ export class GridComposer extends Component { this.isEditing = isEditing; if (!isEditing) { this.rect = this.defaultRect; - this.DOMFocusableElementStore.focus(); + this.DOMFocusableElementStore.focusableElement?.focus(); return; } const position = this.env.model.getters.getActivePosition(); diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts index 2be2735a86..a98b39d615 100644 --- a/src/components/grid/grid.ts +++ b/src/components/grid/grid.ts @@ -176,7 +176,7 @@ export class Grid extends Component { useEffect( () => { if (!this.sidePanel.isOpen) { - this.DOMFocusableElementStore.focus(); + this.DOMFocusableElementStore.focusableElement?.focus(); } }, () => [this.sidePanel.isOpen] @@ -400,7 +400,7 @@ export class Grid extends Component { !this.env.model.getters.getSelectedFigureId() && this.composerStore.editionMode === "inactive" ) { - this.DOMFocusableElementStore.focus(); + this.DOMFocusableElementStore.focusableElement?.focus(); } } diff --git a/src/stores/DOM_focus_store.ts b/src/stores/DOM_focus_store.ts index dba9114d84..0fe2f080ad 100644 --- a/src/stores/DOM_focus_store.ts +++ b/src/stores/DOM_focus_store.ts @@ -1,12 +1,8 @@ export class DOMFocusableElementStore { - mutators = ["setFocusableElement", "focus"] as const; - private focusableElement: HTMLElement | undefined = undefined; + mutators = ["setFocusableElement"] as const; + focusableElement: HTMLElement | undefined = undefined; setFocusableElement(element: HTMLElement) { this.focusableElement = element; } - - focus() { - this.focusableElement?.focus(); - } } diff --git a/tests/bottom_bar/bottom_bar_component.test.ts b/tests/bottom_bar/bottom_bar_component.test.ts index 2926f221b3..56e5ce530e 100644 --- a/tests/bottom_bar/bottom_bar_component.test.ts +++ b/tests/bottom_bar/bottom_bar_component.test.ts @@ -366,14 +366,20 @@ describe("BottomBar component", () => { test.each(["Enter", "Escape"])( "Pressing %s ends the edition and yields back the DOM focus", async (key) => { + const focusElement = jest.fn(); + const focusableElementStore = env.getStore(DOMFocusableElementStore); + const defaultFocusElement = document.createElement("div"); + defaultFocusElement.focus = focusElement; + focusableElementStore.setFocusableElement(defaultFocusElement); + const sheetName = fixture.querySelector(".o-sheet-name")!; // will give focus back to the component main node triggerMouseEvent(sheetName, "dblclick"); await nextTick(); sheetName.textContent = "New name"; + expect(focusElement).not.toHaveBeenCalled(); await keyDown({ key }); - const focusableElementStore = env.getStore(DOMFocusableElementStore); - expect(focusableElementStore.focus).toHaveBeenCalled(); + expect(focusElement).toHaveBeenCalled(); } ); }); diff --git a/tests/top_bar_component.test.ts b/tests/top_bar_component.test.ts index d0c5548f1b..4f3bf83f0a 100644 --- a/tests/top_bar_component.test.ts +++ b/tests/top_bar_component.test.ts @@ -2,11 +2,12 @@ import { Component, onMounted, onWillUnmount, xml } from "@odoo/owl"; import { Model } from "../src"; import { ComposerStore } from "../src/components/composer/composer/composer_store"; import { TopBar } from "../src/components/top_bar/top_bar"; -import { DEFAULT_FONT_SIZE } from "../src/constants"; +import { DEBOUNCE_TIME, DEFAULT_FONT_SIZE } from "../src/constants"; import { toZone, zoneToXc } from "../src/helpers"; import { topbarComponentRegistry, topbarMenuRegistry } from "../src/registries"; import { ConditionalFormat, Currency, Pixel, SpreadsheetChildEnv, Style } from "../src/types"; import { FileStore } from "./__mocks__/mock_file_store"; +import { MockTransportService } from "./__mocks__/transport_service"; import { addCellToSelection, createTable, @@ -910,3 +911,21 @@ describe("Topbar svg icon", () => { expect(icon?.classList.contains(iconClass)).toBeTruthy(); }); }); + +test("Clicking on a topbar button only trigger a single render", async () => { + jest.useFakeTimers(); + const transportService = new MockTransportService(); + + const model = new Model({}, { transportService }); + const { fixture, env } = await mountSpreadsheet({ model }); + jest.advanceTimersByTime(DEBOUNCE_TIME + 10); // wait for the debounce of session.move + jest.useRealTimers(); + + const triggerRender = jest.fn(); + model.on("update", {}, triggerRender); + env["__spreadsheet_stores__"].on("store-updated", null, triggerRender); + + await click(fixture, ".o-spreadsheet-topbar [title='Bold (Ctrl+B)']"); + + expect(triggerRender).toHaveBeenCalledTimes(1); +});