From d599a2b1251d1f7ef2a446327918dfc892aad2e3 Mon Sep 17 00:00:00 2001 From: oscarkilhed Date: Tue, 13 Jan 2026 13:03:33 +0100 Subject: [PATCH] Dashboards: Fix drag-to-tabs edge cases and improve drop behavior - Use capture phase for pointerup listeners to handle tab header stopPropagation - Drop items into current tab when released on tab header after cross-tab drag - Handle RowsLayoutManager in TabItem.draggedGridItemInside (add to first row) - Clear tab activation timer on row drag pointerup to prevent late tab switches - Add tests for cross-tab drag scenarios --- .../scene/DashboardLayoutOrchestrator.test.ts | 187 ++++++++++++++++++ .../scene/DashboardLayoutOrchestrator.tsx | 111 ++++++----- .../scene/layout-tabs/TabItem.tsx | 13 ++ 3 files changed, 265 insertions(+), 46 deletions(-) diff --git a/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.test.ts b/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.test.ts index 967ac472d34..7b1c0979194 100644 --- a/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.test.ts +++ b/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.test.ts @@ -10,6 +10,130 @@ import { TabItem } from './layout-tabs/TabItem'; import { TabsLayoutManager } from './layout-tabs/TabsLayoutManager'; describe('DashboardLayoutOrchestrator', () => { + describe('cross-tab drag cancel', () => { + it('should drop item into current tab when dropped on tab header after detach', () => { + const { orchestrator, tab1Manager, tab2Manager, gridItem, tabsManager, tab1 } = setupWithTwoTabs(); + + // Simulate state after cross-tab drag started: + // - Item was detached from source + // - We're on Tab 2 now + // - User releases mouse over tab header (no valid drop target under mouse) + // Expected: Item drops into Tab 2's layout + + orchestrator.setState({ + draggingGridItem: gridItem.getRef(), + sourceTabKey: tab1.state.key, + }); + + const tab2 = tabsManager.state.tabs[1]; + + // @ts-expect-error - accessing private property for testing + orchestrator._sourceDropTarget = tab1Manager; + // @ts-expect-error - accessing private property for testing + // lastDropTarget is the TabItem (set when tab switches) + orchestrator._lastDropTarget = tab2; + // @ts-expect-error - accessing private property for testing + orchestrator._itemDetachedFromSource = true; + + // Simulate the item being removed from source (as happens during tab switch) + tab1Manager.draggedGridItemOutside(gridItem); + + // Switch to tab 2 (simulating what happens after 600ms hover) + tabsManager.switchToTab(tab2); + + // Verify item was removed from tab1 + expect(tab1Manager.state.layout.state.children).toHaveLength(0); + // Verify tab2 is empty before drop + expect(tab2Manager.state.layout.state.children).toHaveLength(0); + + // Mock _getDropTargetUnderMouse to return null (simulating cursor over tab header) + // @ts-expect-error - accessing private method for testing + const originalGetDropTargetUnderMouse = orchestrator._getDropTargetUnderMouse; + // @ts-expect-error - accessing private method for testing + orchestrator._getDropTargetUnderMouse = jest.fn().mockReturnValue(null); + + // Create a mock pointer event + const mockEvent = { + clientX: 100, + clientY: 100, + } as PointerEvent; + + // Call _stopDraggingSync (this is what happens on mouse release) + // @ts-expect-error - accessing private method for testing + orchestrator._stopDraggingSync(mockEvent); + + // Restore original methods + // @ts-expect-error - accessing private method for testing + orchestrator._getDropTargetUnderMouse = originalGetDropTargetUnderMouse; + + // Wait for setTimeout to execute + return new Promise((resolve) => { + setTimeout(() => { + // Verify item was dropped into tab2 + expect(tab2Manager.state.layout.state.children).toHaveLength(1); + expect(tab2Manager.state.layout.state.children[0]).toBe(gridItem); + + // Tab1 should still be empty + expect(tab1Manager.state.layout.state.children).toHaveLength(0); + + // We should still be on tab2 + expect(tabsManager.getCurrentTab()).toBe(tab2); + + resolve(); + }, 0); + }); + }); + + it('should complete normal drop when valid drop target exists', () => { + const { orchestrator, tab1Manager, tab2Manager, gridItem, tab1 } = setupWithTwoTabs(); + + // Simulate state after cross-tab drag started + orchestrator.setState({ + draggingGridItem: gridItem.getRef(), + sourceTabKey: tab1.state.key, + }); + + // @ts-expect-error - accessing private property for testing + orchestrator._sourceDropTarget = tab1Manager; + // @ts-expect-error - accessing private property for testing + orchestrator._lastDropTarget = tab2Manager; + // @ts-expect-error - accessing private property for testing + orchestrator._itemDetachedFromSource = true; + + // Simulate the item being removed from source + tab1Manager.draggedGridItemOutside(gridItem); + expect(tab1Manager.state.layout.state.children).toHaveLength(0); + + // Mock _getDropTargetUnderMouse to return the tab2Manager (valid drop target) + // @ts-expect-error - accessing private method for testing + const originalGetDropTargetUnderMouse = orchestrator._getDropTargetUnderMouse; + // @ts-expect-error - accessing private method for testing + orchestrator._getDropTargetUnderMouse = jest.fn().mockReturnValue(tab2Manager); + + const mockEvent = { + clientX: 100, + clientY: 100, + } as PointerEvent; + + // @ts-expect-error - accessing private method for testing + orchestrator._stopDraggingSync(mockEvent); + + // @ts-expect-error - accessing private method for testing + orchestrator._getDropTargetUnderMouse = originalGetDropTargetUnderMouse; + + return new Promise((resolve) => { + setTimeout(() => { + // Verify item was NOT returned to source (it should go to tab2) + expect(tab1Manager.state.layout.state.children).toHaveLength(0); + expect(tab2Manager.state.layout.state.children).toHaveLength(1); + expect(tab2Manager.state.layout.state.children[0]).toBe(gridItem); + + resolve(); + }, 0); + }); + }); + }); + describe('isDragging', () => { it('should return false when nothing is being dragged', () => { const { orchestrator } = setup(); @@ -319,3 +443,66 @@ function setupAutoGrid() { return { manager, gridItem1, gridItem2, panel1, panel2 }; } + +function setupWithTwoTabs() { + // Create panel for Tab 1 + const panel1 = new VizPanel({ + title: 'Panel in Tab 1', + key: 'panel-tab1', + pluginId: 'table', + }); + + const gridItem = new AutoGridItem({ + key: 'grid-item-tab1', + body: panel1, + }); + + const tab1Manager = new AutoGridLayoutManager({ + key: 'tab1-manager', + layout: new AutoGridLayout({ children: [gridItem] }), + }); + + const tab1 = new TabItem({ + key: 'tab-1', + title: 'Tab 1', + layout: tab1Manager, + }); + + // Create empty Tab 2 + const tab2Manager = new AutoGridLayoutManager({ + key: 'tab2-manager', + layout: new AutoGridLayout({ children: [] }), + }); + + const tab2 = new TabItem({ + key: 'tab-2', + title: 'Tab 2', + layout: tab2Manager, + }); + + const tabsManager = new TabsLayoutManager({ + tabs: [tab1, tab2], + }); + + const orchestrator = new DashboardLayoutOrchestrator(); + + const dashboard = new DashboardScene({ + body: tabsManager, + layoutOrchestrator: orchestrator, + }); + + // Activate the scene hierarchy to set up parent relationships + dashboard.activate(); + + return { + orchestrator, + tabsManager, + tab1, + tab2, + tab1Manager, + tab2Manager, + gridItem, + panel1, + dashboard, + }; +} diff --git a/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.tsx b/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.tsx index a8369bcbb4f..ba99413e2a2 100644 --- a/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.tsx +++ b/public/app/features/dashboard-scene/scene/DashboardLayoutOrchestrator.tsx @@ -97,8 +97,8 @@ export class DashboardLayoutOrchestrator extends SceneObjectBase { document.body.removeEventListener('pointermove', this._onPointerMove); document.body.removeEventListener('pointermove', this._onRowDragPointerMove); - document.body.removeEventListener('pointerup', this._stopDraggingSync); - document.body.removeEventListener('pointerup', this._onRowDragPointerUp); + document.body.removeEventListener('pointerup', this._stopDraggingSync, true); + document.body.removeEventListener('pointerup', this._onRowDragPointerUp, true); this._clearTabActivationTimer(); this._clearDragPreview(); }; @@ -133,65 +133,83 @@ export class DashboardLayoutOrchestrator extends SceneObjectBase { - if (gridItem) { - // Only remove from source if not already detached during tab switch - if (!wasDetached) { - sourceDropTarget?.draggedGridItemOutside?.(gridItem); - } - // Pass drop position for precise placement (AutoGrid uses this) - // Note: draggedGridItemInside also clears isDropTarget and dropPosition - lastDropTarget?.draggedGridItemInside?.(gridItem, dropPosition ?? undefined); - - // Clean up source grid's drag state (CSS variables and draggingKey) after item is moved. - // This is done here (after movement) to prevent flickering where the item - // would momentarily appear at wrong position (CSS vars cleared but draggingKey set - // = absolute positioning with no valid position values). - if (sourceDropTarget instanceof AutoGridLayoutManager) { - sourceDropTarget.state.layout.endExternalDrag(); - } - } else { - const warningMessage = 'No grid item to drag'; - console.warn(warningMessage); - logWarning(warningMessage); + lastDropTarget.draggedGridItemInside?.(gridItem); + // Clean up source grid state + if (sourceDropTarget instanceof AutoGridLayoutManager) { + sourceDropTarget.state.layout.endExternalDrag(); } }); + } else { + const isCrossLayoutDrop = sourceDropTarget !== lastDropTarget || wasDetached; + + // Handle cross-layout or cross-tab drop + if (isCrossLayoutDrop) { + // Wrapped in setTimeout to ensure that any event handlers are called + // Useful for allowing react-grid-layout to remove placeholders, etc. + setTimeout(() => { + if (gridItem) { + // Only remove from source if not already detached during tab switch + if (!wasDetached) { + sourceDropTarget?.draggedGridItemOutside?.(gridItem); + } + // Pass drop position for precise placement (AutoGrid uses this) + // Note: draggedGridItemInside also clears isDropTarget and dropPosition + lastDropTarget?.draggedGridItemInside?.(gridItem, dropPosition ?? undefined); + + // Clean up source grid's drag state (CSS variables and draggingKey) after item is moved. + // This is done here (after movement) to prevent flickering where the item + // would momentarily appear at wrong position (CSS vars cleared but draggingKey set + // = absolute positioning with no valid position values). + if (sourceDropTarget instanceof AutoGridLayoutManager) { + sourceDropTarget.state.layout.endExternalDrag(); + } + } else { + const warningMessage = 'No grid item to drag'; + console.warn(warningMessage); + logWarning(warningMessage); + } + }); + } else { + // For same-layout drops, clear drop position state synchronously + this._clearDropPosition(); + this._lastDropTarget?.setIsDropTarget?.(false); + } } document.body.removeEventListener('pointermove', this._onPointerMove); - document.body.removeEventListener('pointerup', this._stopDraggingSync); + document.body.removeEventListener('pointerup', this._stopDraggingSync, true); this._clearTabActivationTimer(); this._clearDragPreview(); - // For cross-layout drops, don't clear drop position/target state synchronously. - // The placeholder should remain visible until the item is added by draggedGridItemInside, - // which also clears isDropTarget and dropPosition. This prevents flickering where the - // grid would momentarily shrink (placeholder removed) before expanding again (item added). - if (!isCrossLayoutDrop) { - this._clearDropPosition(); - this._lastDropTarget?.setIsDropTarget?.(false); - } - // Clear internal tracking state (but not the visual state on the target for cross-layout drops) this._currentDropPosition = null; this._lastHoveredAutoGridItemKey = null; @@ -227,7 +245,8 @@ export class DashboardLayoutOrchestrator extends SceneObjectBase { @@ -249,6 +268,10 @@ export class DashboardLayoutOrchestrator extends SceneObjectBase { + // Always clear the tab activation timer on pointerup to prevent + // the tab from switching after the user has released the mouse + this._clearTabActivationTimer(); + // Handle drop after cross-tab row drag if (this._itemDetachedFromSource) { const row = this.state.draggingRow?.resolve(); @@ -259,13 +282,9 @@ export class DashboardLayoutOrchestrator extends SceneObjectBase