Skip to content

Commit

Permalink
[FIX] spreadsheet: avoid parasitic renders
Browse files Browse the repository at this point in the history
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 #5346

Task: 4373054
Signed-off-by: Rémi Rahir (rar) <[email protected]>
  • Loading branch information
hokolomopo committed Jan 6, 2025
1 parent d8f81f1 commit 30e1e37
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 14 deletions.
3 changes: 3 additions & 0 deletions src/collaborative/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,9 @@ export class Session extends EventBus<CollaborativeEvent> {
}
}
this.acknowledge(message);
if (message.type === "REMOTE_REVISION" && message.clientId === this.clientId) {
return;
}
this.trigger("collaborative-event-received");
}

Expand Down
4 changes: 2 additions & 2 deletions src/components/bottom_bar_sheet/bottom_bar_sheet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,11 @@ export class BottomBarSheet extends Component<Props, SpreadsheetChildEnv> {
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();
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/components/composer/grid_composer/grid_composer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export class GridComposer extends Component<Props, SpreadsheetChildEnv> {
this.isEditing = isEditing;
if (!isEditing) {
this.rect = this.defaultRect;
this.DOMFocusableElementStore.focus();
this.DOMFocusableElementStore.focusableElement?.focus();
return;
}
const position = this.env.model.getters.getActivePosition();
Expand Down
4 changes: 2 additions & 2 deletions src/components/grid/grid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
useEffect(
() => {
if (!this.sidePanel.isOpen) {
this.DOMFocusableElementStore.focus();
this.DOMFocusableElementStore.focusableElement?.focus();
}
},
() => [this.sidePanel.isOpen]
Expand Down Expand Up @@ -400,7 +400,7 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
!this.env.model.getters.getSelectedFigureId() &&
this.composerStore.editionMode === "inactive"
) {
this.DOMFocusableElementStore.focus();
this.DOMFocusableElementStore.focusableElement?.focus();
}
}

Expand Down
8 changes: 2 additions & 6 deletions src/stores/DOM_focus_store.ts
Original file line number Diff line number Diff line change
@@ -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();
}
}
10 changes: 8 additions & 2 deletions tests/bottom_bar/bottom_bar_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLElement>(".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();
}
);
});
Expand Down
21 changes: 20 additions & 1 deletion tests/top_bar_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
});

0 comments on commit 30e1e37

Please sign in to comment.