[FIX] figure: wrong focus change on figure unmount

hokolomopo · hokolomopo · commit ca9b4048768e · 2025-12-01T12:51:32.000Z
When a figure is unmounted, the focus is moved to the default focusable element (the grid composer). But this change is done even if the figure wasn't focused. For example, the focus would be removed from the find & replace search input when leaving a sheet with a figure. closes #7333 Task: 5154025 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
diff --git a/src/components/figures/figure/figure.ts b/src/components/figures/figure/figure.ts
@@ -1,4 +1,4 @@
-import { Component, onWillUnmount, useEffect, useRef, useState } from "@odoo/owl";
+import { Component, useEffect, useRef, useState } from "@odoo/owl";
 import {
   ComponentsImportance,
   FIGURE_BORDER_COLOR,
@@ -115,7 +115,6 @@ css/*SCSS*/ `
 interface Props {
   figure: Figure;
   style: string;
-  onFigureDeleted: () => void;
   onMouseDown: (ev: MouseEvent) => void;
   onClickAnchor(dirX: ResizeDirection, dirY: ResizeDirection, ev: MouseEvent): void;
 }
@@ -125,13 +124,11 @@ export class FigureComponent extends Component<Props, SpreadsheetChildEnv> {
   static props = {
     figure: Object,
     style: { type: String, optional: true },
-    onFigureDeleted: { type: Function, optional: true },
     onMouseDown: { type: Function, optional: true },
     onClickAnchor: { type: Function, optional: true },
   };
   static components = { Menu };
   static defaultProps = {
-    onFigureDeleted: () => {},
     onMouseDown: () => {},
     onClickAnchor: () => {},
   };
@@ -214,10 +211,6 @@ export class FigureComponent extends Component<Props, SpreadsheetChildEnv> {
       },
       () => [this.env.model.getters.getSelectedFigureId(), this.props.figure.id, this.figureRef.el]
     );
-
-    onWillUnmount(() => {
-      this.props.onFigureDeleted();
-    });
   }
 
   clickAnchor(dirX: ResizeDirection, dirY: ResizeDirection, ev: MouseEvent) {
@@ -239,7 +232,6 @@ export class FigureComponent extends Component<Props, SpreadsheetChildEnv> {
           sheetId: this.env.model.getters.getActiveSheetId(),
           id: figure.id,
         });
-        this.props.onFigureDeleted();
         ev.preventDefault();
         ev.stopPropagation();
         break;
@@ -304,6 +296,6 @@ export class FigureComponent extends Component<Props, SpreadsheetChildEnv> {
     this.menuState.position = position;
     this.menuState.menuItems = figureRegistry
       .get(this.props.figure.tag)
-      .menuBuilder(this.props.figure.id, this.props.onFigureDeleted, this.env);
+      .menuBuilder(this.props.figure.id, this.env);
   }
 }
diff --git a/src/components/figures/figure/figure.xml b/src/components/figures/figure/figure.xml
@@ -14,7 +14,6 @@
         <t
           t-component="figureRegistry.get(props.figure.tag).Component"
           t-key="props.figure.id"
-          onFigureDeleted="props.onFigureDeleted"
           figure="props.figure"
         />
         <div class="o-figure-menu position-absolute m-2" t-if="!env.isDashboard()">
diff --git a/src/components/figures/figure_chart/figure_chart.ts b/src/components/figures/figure_chart/figure_chart.ts
@@ -18,14 +18,12 @@ interface Props {
   // props figure is currently necessary scorecards, we need the chart dimension at render to avoid having to force the
   // style by hand in the useEffect()
   figure: Figure;
-  onFigureDeleted: () => void;
 }
 
 export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {
   static template = "o-spreadsheet-ChartFigure";
   static props = {
     figure: Object,
-    onFigureDeleted: Function,
   };
   static components = {};
 
diff --git a/src/components/figures/figure_container/figure_container.ts b/src/components/figures/figure_container/figure_container.ts
@@ -22,9 +22,7 @@ import { FigureComponent } from "../figure/figure";
 
 type ContainerType = "topLeft" | "topRight" | "bottomLeft" | "bottomRight" | "dnd";
 
-interface Props {
-  onFigureDeleted: () => void;
-}
+interface Props {}
 
 interface Container {
   type: ContainerType;
@@ -127,9 +125,7 @@ css/*SCSS*/ `
  */
 export class FiguresContainer extends Component<Props, SpreadsheetChildEnv> {
   static template = "o-spreadsheet-FiguresContainer";
-  static props = {
-    onFigureDeleted: Function,
-  };
+  static props = {};
   static components = { FigureComponent };
 
   dnd = useState<DndState>({
diff --git a/src/components/figures/figure_container/figure_container.xml b/src/components/figures/figure_container/figure_container.xml
@@ -11,7 +11,6 @@
             t-att-style="container.inverseViewportStyle">
             <t t-foreach="container.figures" t-as="figure" t-key="figure.id">
               <FigureComponent
-                onFigureDeleted="this.props.onFigureDeleted"
                 figure="figure"
                 style="getFigureStyle(figure)"
                 onMouseDown="(ev) => this.startDraggingFigure(figure, ev)"
diff --git a/src/components/figures/figure_image/figure_image.ts b/src/components/figures/figure_image/figure_image.ts
@@ -3,14 +3,12 @@ import { Figure, SpreadsheetChildEnv, UID } from "../../../types";
 
 interface Props {
   figure: Figure;
-  onFigureDeleted: () => void;
 }
 
 export class ImageFigure extends Component<Props, SpreadsheetChildEnv> {
   static template = "o-spreadsheet-ImageFigure";
   static props = {
     figure: Object,
-    onFigureDeleted: Function,
   };
   static components = {};
 
diff --git a/src/components/grid/grid.xml b/src/components/grid/grid.xml
@@ -16,7 +16,6 @@
         onGridResized.bind="onGridResized"
         onGridMoved.bind="moveCanvas"
         gridOverlayDimensions="gridOverlayDimensions"
-        onFigureDeleted.bind="focusDefaultElement"
       />
       <HeadersOverlay onOpenContextMenu="(type, x, y) => this.toggleContextMenu(type, x, y)"/>
       <GridComposer
diff --git a/src/components/grid_add_rows_footer/grid_add_rows_footer.ts b/src/components/grid_add_rows_footer/grid_add_rows_footer.ts
@@ -1,4 +1,6 @@
 import { Component, useExternalListener, useRef, useState } from "@odoo/owl";
+import { Store, useStore } from "../../store_engine";
+import { DOMFocusableElementStore } from "../../stores/DOM_focus_store";
 import { _t } from "../../translation";
 import { SpreadsheetChildEnv } from "../../types";
 import { css, cssPropertiesToCss } from "../helpers";
@@ -20,23 +22,23 @@ css/* scss */ `
   }
 `;
 
-interface Props {
-  focusGrid: () => void;
-}
+interface Props {}
 
 export class GridAddRowsFooter extends Component<Props, SpreadsheetChildEnv> {
   static template = "o-spreadsheet-GridAddRowsFooter";
-  static props = {
-    focusGrid: Function,
-  };
+  static props = {};
   static components = { ValidationMessages };
+
+  private DOMFocusableElementStore!: Store<DOMFocusableElementStore>;
+
   inputRef = useRef<HTMLInputElement>("inputRef");
   state = useState({
     inputValue: "100",
     errorFlag: false,
   });
 
   setup() {
+    this.DOMFocusableElementStore = useStore(DOMFocusableElementStore);
     useExternalListener(window, "click", this.onExternalClick, { capture: true });
   }
 
@@ -58,7 +60,7 @@ export class GridAddRowsFooter extends Component<Props, SpreadsheetChildEnv> {
 
   onKeydown(ev: KeyboardEvent) {
     if (ev.key.toUpperCase() === "ESCAPE") {
-      this.props.focusGrid();
+      this.focusDefaultElement();
     } else if (ev.key.toUpperCase() === "ENTER") {
       this.onConfirm();
     }
@@ -85,7 +87,7 @@ export class GridAddRowsFooter extends Component<Props, SpreadsheetChildEnv> {
       quantity,
       dimension: "ROW",
     });
-    this.props.focusGrid();
+    this.focusDefaultElement();
 
     // After adding new rows, scroll down to the new last row
     const { scrollX } = this.env.model.getters.getActiveSheetDOMScrollInfo();
@@ -103,6 +105,12 @@ export class GridAddRowsFooter extends Component<Props, SpreadsheetChildEnv> {
     if (this.inputRef.el !== document.activeElement || ev.target === this.inputRef.el) {
       return;
     }
-    this.props.focusGrid();
+    this.focusDefaultElement();
+  }
+
+  private focusDefaultElement() {
+    if (document.activeElement === this.inputRef.el) {
+      this.DOMFocusableElementStore.focus();
+    }
   }
 }
diff --git a/src/components/grid_overlay/grid_overlay.ts b/src/components/grid_overlay/grid_overlay.ts
@@ -129,7 +129,6 @@ interface Props {
   onGridResized: (dimension: Rect) => void;
   onGridMoved: (deltaX: Pixel, deltaY: Pixel) => void;
   gridOverlayDimensions: string;
-  onFigureDeleted: () => void;
 }
 
 export class GridOverlay extends Component<Props, SpreadsheetChildEnv> {
@@ -140,7 +139,6 @@ export class GridOverlay extends Component<Props, SpreadsheetChildEnv> {
     onCellClicked: { type: Function, optional: true },
     onCellRightClicked: { type: Function, optional: true },
     onGridResized: { type: Function, optional: true },
-    onFigureDeleted: { type: Function, optional: true },
     onGridMoved: Function,
     gridOverlayDimensions: String,
   };
@@ -156,7 +154,6 @@ export class GridOverlay extends Component<Props, SpreadsheetChildEnv> {
     onCellClicked: () => {},
     onCellRightClicked: () => {},
     onGridResized: () => {},
-    onFigureDeleted: () => {},
   };
   private gridOverlay: Ref<HTMLElement> = useRef("gridOverlay");
   private gridOverlayRect = useAbsoluteBoundingRect(this.gridOverlay);
diff --git a/src/components/grid_overlay/grid_overlay.xml b/src/components/grid_overlay/grid_overlay.xml
@@ -8,13 +8,12 @@
       t-on-pointerdown="onMouseDown"
       t-on-dblclick.self="onDoubleClick"
       t-on-contextmenu.stop.prevent="onContextMenu">
-      <FiguresContainer onFigureDeleted="props.onFigureDeleted"/>
+      <FiguresContainer/>
       <DataValidationOverlay/>
       <FilterIconsOverlay/>
       <GridAddRowsFooter
         t-if="!env.model.getters.isReadonly()"
         t-key="env.model.getters.getActiveSheetId()"
-        focusGrid="props.onFigureDeleted"
       />
     </div>
   </t>
diff --git a/src/components/spreadsheet/spreadsheet.ts b/src/components/spreadsheet/spreadsheet.ts
@@ -405,23 +405,20 @@ export class Spreadsheet extends Component<SpreadsheetProps, SpreadsheetChildEnv
 
     this.notificationStore.updateNotificationCallbacks({ ...this.props });
 
-    useEffect(
-      () => {
-        /**
-         * Only refocus the grid if the active element is not a child of the spreadsheet
-         * (i.e. activeElement is outside of the spreadsheetRef component)
-         * and spreadsheet is a child of that element. Anything else means that the focus
-         * is on an element that needs to keep it.
-         */
-        if (
-          !this.spreadsheetRef.el!.contains(document.activeElement) &&
-          document.activeElement?.contains(this.spreadsheetRef.el!)
-        ) {
-          this.focusGrid();
-        }
-      },
-      () => [this.env.model.getters.getActiveSheetId()]
-    );
+    useEffect(() => {
+      /**
+       * Only refocus the grid if the active element is not a child of the spreadsheet
+       * (i.e. activeElement is outside of the spreadsheetRef component)
+       * and spreadsheet is a child of that element. Anything else means that the focus
+       * is on an element that needs to keep it.
+       */
+      if (
+        !this.spreadsheetRef.el!.contains(document.activeElement) &&
+        document.activeElement?.contains(this.spreadsheetRef.el!)
+      ) {
+        this.focusGrid();
+      }
+    });
 
     useExternalListener(window as any, "resize", () => this.render(true));
 
diff --git a/src/registries/figure_registry.ts b/src/registries/figure_registry.ts
@@ -19,7 +19,7 @@ import { Registry } from "./registry";
 
 export interface FigureContent {
   Component: any;
-  menuBuilder: (figureId: UID, onFigureDeleted: () => void, env: SpreadsheetChildEnv) => Action[];
+  menuBuilder: (figureId: UID, env: SpreadsheetChildEnv) => Action[];
   SidePanelComponent?: string;
   keepRatio?: boolean;
   minFigSize?: number;
@@ -42,7 +42,7 @@ figureRegistry.add("image", {
 
 function getChartMenu(
   figureId: UID,
-  onFigureDeleted: () => void,
+
   env: SpreadsheetChildEnv
 ): Action[] {
   const menuItemSpecs: ActionSpec[] = [
@@ -58,14 +58,14 @@ function getChartMenu(
     },
     getCopyMenuItem(figureId, env),
     getCutMenuItem(figureId, env),
-    getDeleteMenuItem(figureId, onFigureDeleted, env),
+    getDeleteMenuItem(figureId, env),
   ];
   return createActions(menuItemSpecs);
 }
 
 function getImageMenuRegistry(
   figureId: UID,
-  onFigureDeleted: () => void,
+
   env: SpreadsheetChildEnv
 ): Action[] {
   const menuItemSpecs: ActionSpec[] = [
@@ -94,7 +94,7 @@ function getImageMenuRegistry(
       },
       icon: "o-spreadsheet-Icon.REFRESH",
     },
-    getDeleteMenuItem(figureId, onFigureDeleted, env),
+    getDeleteMenuItem(figureId, env),
   ];
   return createActions(menuItemSpecs);
 }
@@ -131,7 +131,7 @@ function getCutMenuItem(figureId: UID, env: SpreadsheetChildEnv): ActionSpec {
 
 function getDeleteMenuItem(
   figureId: UID,
-  onFigureDeleted: () => void,
+
   env: SpreadsheetChildEnv
 ): ActionSpec {
   return {
@@ -143,7 +143,6 @@ function getDeleteMenuItem(
         sheetId: env.model.getters.getActiveSheetId(),
         id: figureId,
       });
-      onFigureDeleted();
     },
     icon: "o-spreadsheet-Icon.TRASH",
   };
diff --git a/tests/figures/figure_component.test.ts b/tests/figures/figure_component.test.ts
@@ -714,6 +714,23 @@ describe("figures", () => {
     expect(bottomRightContainerStyle.height).toEqual(`${height - 2 * DEFAULT_CELL_HEIGHT}px`);
   });
 
+  test("Deleting a figure does not change the DOM focus if the figure was not focused", async () => {
+    createFigure(model);
+    await nextTick();
+    env.openSidePanel("FindAndReplace");
+    await nextTick();
+
+    const panelInput = fixture.querySelector<HTMLElement>(".o-sidePanel input");
+    panelInput?.focus();
+    expect(document.activeElement).toBe(panelInput);
+
+    const figureId = model.getters.getFigures(sheetId)[0].id;
+    model.dispatch("DELETE_FIGURE", { sheetId, id: figureId });
+    await nextTick();
+
+    expect(document.activeElement).toBe(panelInput);
+  });
+
   describe("Figure drag & drop snap", () => {
     describe("Move figure", () => {
       test.each([
