From 213e6a83eab915bb03af9eb249335e5bbcfba821 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= <rar@odoo.com>
Date: Wed, 3 Jun 2026 12:44:27 +0200
Subject: [PATCH] [FIX] Figure: only focus when the selected figure changes

The condition of the useEffect to re-focus a figure were removed by
mistake. This meant that every render, if there was a selected figure,
its DOM element would be automatically focus.

There was a race condition with the component SelectionInput that
sometimes let the latter have the focus, sometimes not, which lead to
the weird behaviour described in the task:

- Insert a scorecard
- Add a key value
- Edit the key value and select it
- Hit backspace twice

=> The first press deletes the value, the second deletes the chart

Task: 6267606
---
 src/components/figures/figure/figure.ts      | 35 +++++++++++---------
 tests/figures/chart/charts_component.test.ts | 35 ++++++++++++++++++++
 tests/figures/figure_component.test.ts       | 13 ++++++++
 3 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/src/components/figures/figure/figure.ts b/src/components/figures/figure/figure.ts
index 390846078d..e65200e4a9 100644
--- a/src/components/figures/figure/figure.ts
+++ b/src/components/figures/figure/figure.ts
@@ -114,22 +114,25 @@ export class FigureComponent extends Component<SpreadsheetChildEnv> {
   setup() {
     const borderWidth = figureRegistry.get(this.props.figureUI.tag).borderWidth;
     this.borderWidth = borderWidth !== undefined ? borderWidth : BORDER_WIDTH;
-    useLayoutEffect(() => {
-      const selectedFigureIds = this.env.model.getters.getSelectedFigureIds();
-      const thisFigureId = this.props.figureUI.id;
-      const el = this.figureRef();
-      if (selectedFigureIds.includes(thisFigureId)) {
-        /** Scrolling on a newly inserted figure that overflows outside the viewport
-         * will break the whole layout.
-         * NOTE: `preventScroll`does not work on mobile but then again,
-         * mobile is not really supported ATM.
-         *
-         * TODO: When implementing proper mobile, we will need to scroll the viewport
-         * correctly (and render?) before focusing the element.
-         */
-        el?.focus({ preventScroll: true });
-      }
-    });
+    useLayoutEffect(
+      () => {
+        const selectedFigureIds = this.env.model.getters.getSelectedFigureIds();
+        const thisFigureId = this.props.figureUI.id;
+        const el = this.figureRef();
+        if (selectedFigureIds.includes(thisFigureId)) {
+          /** Scrolling on a newly inserted figure that overflows outside the viewport
+           * will break the whole layout.
+           * NOTE: `preventScroll`does not work on mobile but then again,
+           * mobile is not really supported ATM.
+           *
+           * TODO: When implementing proper mobile, we will need to scroll the viewport
+           * correctly (and render?) before focusing the element.
+           */
+          el?.focus({ preventScroll: true });
+        }
+      },
+      () => [this.env.model.getters.getSelectedFigureIds(), this.props.figureUI.id]
+    );
   }
 
   clickAnchor(dirX: ResizeDirection, dirY: ResizeDirection, ev: MouseEvent) {
diff --git a/tests/figures/chart/charts_component.test.ts b/tests/figures/chart/charts_component.test.ts
index e4088e82f5..8a0536ceb0 100644
--- a/tests/figures/chart/charts_component.test.ts
+++ b/tests/figures/chart/charts_component.test.ts
@@ -61,6 +61,7 @@ import { TEST_CHART_DATA } from "../../test_helpers/constants";
 import {
   click,
   clickAndDrag,
+  clickCell,
   doubleClick,
   editSelectComponent,
   focusAndKeyDown,
@@ -3404,3 +3405,37 @@ test("Can update the chart data source from the side panel", async () => {
     })
   );
 });
+
+test("Can edit a chart range", async () => {
+  model = new Model();
+  createChart(
+    model,
+    {
+      type: "bar",
+      ...toChartDataSource({
+        dataSets: [{ dataRange: "A1:A3" }],
+        dataSetsHaveTitle: false,
+      }),
+    },
+    chartId
+  );
+  await mountSpreadsheet();
+  selectFigure(model, model.getters.getFigureIdFromChartId(chartId)!);
+  await simulateClick(".o-figure");
+  env.openSidePanel("ChartPanel");
+  await nextTick();
+  expect(document.activeElement).toBe(fixture.querySelector(".o-figure")!);
+  await simulateClick(".o-selection-input input.o-input");
+  await clickCell(model, "B1");
+  await keyDown({ key: "ArrowDown" });
+
+  await simulateClick(".o-data-series .o-selection-ok");
+  const definition = model.getters.getChartDefinition(chartId) as BarChartDefinition<string>;
+
+  expect(definition).toMatchObject(
+    toChartDataSource({
+      dataSets: [{ dataRange: "B2", dataSetId: expect.any(String) }],
+      dataSetsHaveTitle: false,
+    })
+  );
+});
diff --git a/tests/figures/figure_component.test.ts b/tests/figures/figure_component.test.ts
index ff96750ee7..18e38dcaac 100644
--- a/tests/figures/figure_component.test.ts
+++ b/tests/figures/figure_component.test.ts
@@ -1222,6 +1222,19 @@ describe("figures", () => {
     expect(document.activeElement).toBe(panelInput);
   });
 
+  test("Can navigate the figure menu with the keyboard", async () => {
+    createChart(model, { type: "bar" });
+    await nextTick();
+    await simulateClick(".o-figure-menu-item");
+    // NOTE: need to "force" a rendering such that the menu takes the focus back
+    // See task https://www.odoo.com/odoo/2328/tasks/6272762
+    await keyDown({ key: "ArrowDown" });
+    expect(document.activeElement).toHaveClass("o-menu-wrapper");
+    await keyDown({ key: "ArrowDown" });
+    expect(document.activeElement).toHaveClass("o-menu-item");
+    expect(document.activeElement).toHaveAttribute("data-name", "edit");
+  });
+
   describe("Figure drag & drop snap", () => {
     describe("Move figure", () => {
       test.each([