From 5621d94b2ddda131cac41bb279479d07f8d335b0 Mon Sep 17 00:00:00 2001 From: "Adrien Minne (adrm)" Date: Mon, 13 Feb 2023 10:10:37 +0000 Subject: [PATCH] [FIX] figures: menu not displayed for small figures positioned left Before this commit, the menu was not displayed for small figure positioned left of the screen. This was because if the menu button X was smaller than MENU_WIDTH, we gave a negative X as position ot the popover, thus not displaying it. Odoo task 3177176 closes odoo/o-spreadsheet#2073 Signed-off-by: Pierre Rousseau (pro) --- src/components/dashboard/dashboard.ts | 4 +-- .../figures/figure_chart/figure_chart.ts | 15 ++++----- .../figures/figure_image/figure_image.ts | 15 ++++----- src/components/grid/grid.ts | 4 +-- src/components/helpers/position_hook.ts | 26 ++++++++-------- .../link/link_editor/link_editor.ts | 4 +-- src/components/menu/menu.ts | 4 +-- tests/components/figure.test.ts | 31 +++++++++++++++++-- 8 files changed, 66 insertions(+), 37 deletions(-) diff --git a/src/components/dashboard/dashboard.ts b/src/components/dashboard/dashboard.ts index 2d0c30faf9..29a1538f87 100644 --- a/src/components/dashboard/dashboard.ts +++ b/src/components/dashboard/dashboard.ts @@ -15,7 +15,7 @@ import { GridOverlay } from "../grid_overlay/grid_overlay"; import { GridPopover } from "../grid_popover/grid_popover"; import { css } from "../helpers/css"; import { useGridDrawing } from "../helpers/draw_grid_hook"; -import { useAbsolutePosition } from "../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../helpers/position_hook"; import { useWheelHandler } from "../helpers/wheel_hook"; import { Popover } from "../popover/popover"; import { HorizontalScrollBar, VerticalScrollBar } from "../scrollbar/"; @@ -56,7 +56,7 @@ export class SpreadsheetDashboard extends Component setup() { const gridRef = useRef("grid"); - this.canvasPosition = useAbsolutePosition(gridRef); + this.canvasPosition = useAbsoluteBoundingRect(gridRef); this.hoveredCell = useState({ col: undefined, row: undefined }); useChildSubEnv({ getPopoverContainerRect: () => this.getGridRect() }); diff --git a/src/components/figures/figure_chart/figure_chart.ts b/src/components/figures/figure_chart/figure_chart.ts index 3060526003..43c43838bb 100644 --- a/src/components/figures/figure_chart/figure_chart.ts +++ b/src/components/figures/figure_chart/figure_chart.ts @@ -5,7 +5,7 @@ import { MenuItemRegistry } from "../../../registries/menu_items_registry"; import { _lt } from "../../../translation"; import { ChartType, DOMCoordinates, Figure, SpreadsheetChildEnv } from "../../../types"; import { css } from "../../helpers/css"; -import { useAbsolutePosition } from "../../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../../helpers/position_hook"; import { Menu, MenuState } from "../../menu/menu"; // ----------------------------------------------------------------------------- @@ -33,8 +33,8 @@ export class ChartFigure extends Component { private chartContainerRef = useRef("chartContainer"); private menuButtonRef = useRef("menuButton"); - private menuButtonPosition = useAbsolutePosition(this.menuButtonRef); - private position = useAbsolutePosition(this.chartContainerRef); + private menuButtonRect = useAbsoluteBoundingRect(this.menuButtonRef); + private position: DOMCoordinates = useAbsoluteBoundingRect(this.chartContainerRef); private getMenuItemRegistry(): MenuItemRegistry { const registry = new MenuItemRegistry(); @@ -94,11 +94,12 @@ export class ChartFigure extends Component { } showMenu() { - const position = { - x: this.menuButtonPosition.x - MENU_WIDTH, - y: this.menuButtonPosition.y, + const { x, y, width } = this.menuButtonRect; + const menuPosition = { + x: x >= MENU_WIDTH ? x - MENU_WIDTH : x + width, + y: y, }; - this.openContextMenu(position); + this.openContextMenu(menuPosition); } private openContextMenu(position: DOMCoordinates) { diff --git a/src/components/figures/figure_image/figure_image.ts b/src/components/figures/figure_image/figure_image.ts index 9ce8ad01ab..935ad54471 100644 --- a/src/components/figures/figure_image/figure_image.ts +++ b/src/components/figures/figure_image/figure_image.ts @@ -4,7 +4,7 @@ import { getMaxFigureSize } from "../../../helpers/figures/figure/figure"; import { MenuItemRegistry } from "../../../registries/menu_items_registry"; import { _lt } from "../../../translation"; import { DOMCoordinates, Figure, SpreadsheetChildEnv, UID } from "../../../types"; -import { useAbsolutePosition } from "../../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../../helpers/position_hook"; import { Menu, MenuState } from "../../menu/menu"; interface Props { @@ -19,8 +19,8 @@ export class ImageFigure extends Component { private imageContainerRef = useRef("o-image"); private menuButtonRef = useRef("menuButton"); - private menuButtonPosition = useAbsolutePosition(this.menuButtonRef); - private position = useAbsolutePosition(this.imageContainerRef); + private menuButtonRect = useAbsoluteBoundingRect(this.menuButtonRef); + private position: DOMCoordinates = useAbsoluteBoundingRect(this.imageContainerRef); private getMenuItemRegistry(): MenuItemRegistry { const registry = new MenuItemRegistry(); @@ -84,11 +84,12 @@ export class ImageFigure extends Component { } showMenu() { - const position = { - x: this.menuButtonPosition.x - MENU_WIDTH, - y: this.menuButtonPosition.y, + const { x, y, width } = this.menuButtonRect; + const menuPosition = { + x: x >= MENU_WIDTH ? x - MENU_WIDTH : x + width, + y: y, }; - this.openContextMenu(position); + this.openContextMenu(menuPosition); } private openContextMenu(position: DOMCoordinates) { diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts index 1030ba4d01..579f9ee778 100644 --- a/src/components/grid/grid.ts +++ b/src/components/grid/grid.ts @@ -44,7 +44,7 @@ import { GridPopover } from "../grid_popover/grid_popover"; import { HeadersOverlay } from "../headers_overlay/headers_overlay"; import { dragAndDropBeyondTheViewport } from "../helpers/drag_and_drop"; import { useGridDrawing } from "../helpers/draw_grid_hook"; -import { useAbsolutePosition } from "../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../helpers/position_hook"; import { updateSelectionWithArrowKeys } from "../helpers/selection_helpers"; import { useWheelHandler } from "../helpers/wheel_hook"; import { Highlight } from "../highlight/highlight/highlight"; @@ -117,7 +117,7 @@ export class Grid extends Component { }); this.gridRef = useRef("grid"); this.hiddenInput = useRef("hiddenInput"); - this.canvasPosition = useAbsolutePosition(this.gridRef); + this.canvasPosition = useAbsoluteBoundingRect(this.gridRef); this.hoveredCell = useState({ col: undefined, row: undefined }); useChildSubEnv({ getPopoverContainerRect: () => this.getGridRect() }); diff --git a/src/components/helpers/position_hook.ts b/src/components/helpers/position_hook.ts index 134a0d8248..dcbeb92afb 100644 --- a/src/components/helpers/position_hook.ts +++ b/src/components/helpers/position_hook.ts @@ -1,5 +1,5 @@ import { onMounted, onPatched, useComponent, useRef, useState } from "@odoo/owl"; -import { DOMCoordinates, Rect } from "../../types"; +import { Rect } from "../../types"; // type Ref is not exported by owl :( type Ref = ReturnType; @@ -30,28 +30,28 @@ export function useSpreadsheetRect(): Rect { } /** - * Return the component (or ref's component) top left position (in pixels) relative + * Return the component (or ref's component) BoundingRect, relative * to the upper left corner of the screen ( element). * * Note: when used with a component, it will * return the portal position, not the teleported position. */ -export function useAbsolutePosition(ref: Ref): DOMCoordinates { - const position = useState({ x: 0, y: 0 }); - function updateElPosition() { +export function useAbsoluteBoundingRect(ref: Ref): Rect { + const rect = useState({ x: 0, y: 0, width: 0, height: 0 }); + function updateElRect() { const el = ref.el; if (el === null) { return; } - const { top, left } = el.getBoundingClientRect(); - if (left !== position.x || top !== position.y) { - position.x = left; - position.y = top; - } + const { top, left, width, height } = el.getBoundingClientRect(); + rect.x = left; + rect.y = top; + rect.width = width; + rect.height = height; } - onMounted(updateElPosition); - onPatched(updateElPosition); - return position; + onMounted(updateElRect); + onPatched(updateElRect); + return rect; } /** diff --git a/src/components/link/link_editor/link_editor.ts b/src/components/link/link_editor/link_editor.ts index d182f78b43..73d2cb9a29 100644 --- a/src/components/link/link_editor/link_editor.ts +++ b/src/components/link/link_editor/link_editor.ts @@ -5,7 +5,7 @@ import { linkMenuRegistry } from "../../../registries/menus/link_menu_registry"; import { DOMCoordinates, Link, Position, SpreadsheetChildEnv } from "../../../types"; import { CellPopoverComponent, PopoverBuilders } from "../../../types/cell_popovers"; import { css } from "../../helpers/css"; -import { useAbsolutePosition } from "../../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../../helpers/position_hook"; import { Menu } from "../../menu/menu"; const MENU_OFFSET_X = 320; @@ -109,7 +109,7 @@ export class LinkEditor extends Component isOpen: false, }); private linkEditorRef = useRef("linkEditor"); - private position = useAbsolutePosition(this.linkEditorRef); + private position: DOMCoordinates = useAbsoluteBoundingRect(this.linkEditorRef); urlInput = useRef("urlInput"); setup() { diff --git a/src/components/menu/menu.ts b/src/components/menu/menu.ts index 3e3f92644a..e33909a8b4 100644 --- a/src/components/menu/menu.ts +++ b/src/components/menu/menu.ts @@ -11,7 +11,7 @@ import { FullMenuItem, MenuItem } from "../../registries/menu_items_registry"; import { DOMCoordinates, MenuMouseEvent, Pixel, SpreadsheetChildEnv, UID } from "../../types"; import { css } from "../helpers/css"; import { getOpenedMenus, isChildEvent } from "../helpers/dom_helpers"; -import { useAbsolutePosition } from "../helpers/position_hook"; +import { useAbsoluteBoundingRect } from "../helpers/position_hook"; import { Popover, PopoverProps } from "../popover/popover"; //------------------------------------------------------------------------------ @@ -100,7 +100,7 @@ export class Menu extends Component { menuItems: [], }); private menuRef = useRef("menu"); - private position = useAbsolutePosition(this.menuRef); + private position: DOMCoordinates = useAbsoluteBoundingRect(this.menuRef); setup() { useExternalListener(window, "click", this.onExternalClick, { capture: true }); diff --git a/tests/components/figure.test.ts b/tests/components/figure.test.ts index 3172296cf0..b6ec788fce 100644 --- a/tests/components/figure.test.ts +++ b/tests/components/figure.test.ts @@ -98,10 +98,13 @@ class TextFigure extends Component { } mockChart(); + +let mockSpreadsheetRect: Partial; +let mockFigureMenuItemRect: Partial; mockGetBoundingClientRect({ "o-popover": () => ({ height: 0, width: 0 }), - "o-spreadsheet": () => ({ top: 100, left: 200, height: 1000, width: 1000 }), - "o-figure-menu-item": () => ({ top: 500, left: 500 }), + "o-spreadsheet": () => ({ ...mockSpreadsheetRect }), + "o-figure-menu-item": () => ({ ...mockFigureMenuItemRect }), }); beforeAll(() => { @@ -121,6 +124,8 @@ describe("figures", () => { beforeEach(async () => { fixture = makeTestFixture(); ({ app, model, parent } = await mountSpreadsheet(fixture)); + mockSpreadsheetRect = { top: 100, left: 200, height: 1000, width: 1000 }; + mockFigureMenuItemRect = { top: 500, left: 500 }; }); afterEach(() => { @@ -619,6 +624,28 @@ describe("figures", () => { expect(document.querySelectorAll(".o-menu").length).toBe(1); }); + test(`figure menu position is correct when clicking on menu button for ${type}`, async () => { + mockSpreadsheetRect = { top: 25, left: 25, height: 1000, width: 1000 }; + mockFigureMenuItemRect = { top: 500, left: 500 }; + parent.render(true); // force a render to update `useAbsoluteBoundingRect` with new mocked values + await nextTick(); + await simulateClick(".o-figure-menu-item"); + const menuPopover = fixture.querySelector(".o-popover")!; + expect(menuPopover.style.top).toBe(`${500 - 25}px`); // 25 : spreadsheet offset of the mockGetBoundingClientRect + expect(menuPopover.style.left).toBe(`${500 - 25 - MENU_WIDTH}px`); + }); + + test(`figure menu position is correct when menu button position < MENU_WIDTH for ${type}`, async () => { + mockSpreadsheetRect = { top: 25, left: 25, height: 1000, width: 1000 }; + mockFigureMenuItemRect = { top: 500, left: MENU_WIDTH - 50, width: 32 }; + parent.render(true); // force a render to update `useAbsoluteBoundingRect` with new mocked values + await nextTick(); + await simulateClick(".o-figure-menu-item"); + const menuPopover = fixture.querySelector(".o-popover")!; + expect(menuPopover.style.top).toBe(`${500 - 25}px`); // 25 : spreadsheet offset of the mockGetBoundingClientRect + expect(menuPopover.style.left).toBe(`${MENU_WIDTH - 50 - 25 + 32}px`); + }); + test("Cannot open context menu on right click in dashboard mode", async () => { model.updateMode("dashboard"); triggerMouseEvent(".o-figure", "contextmenu");