From ad7b6d8bd2ecf9a4632e0d1f9450833dbdb68bb8 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Fri, 15 Nov 2024 16:28:19 +0200 Subject: [PATCH] refactor: drop threshold from reordering dashboard widgets (#8157) --- .../src/widget-reorder-controller.js | 51 +++++++++-- .../test/dashboard-widget-reordering.test.ts | 87 ++++--------------- packages/dashboard/test/dashboard.test.ts | 5 +- packages/dashboard/test/helpers.ts | 31 ++++--- 4 files changed, 84 insertions(+), 90 deletions(-) diff --git a/packages/dashboard/src/widget-reorder-controller.js b/packages/dashboard/src/widget-reorder-controller.js index 13942ecf14..dd8149cc52 100644 --- a/packages/dashboard/src/widget-reorder-controller.js +++ b/packages/dashboard/src/widget-reorder-controller.js @@ -11,6 +11,7 @@ import { getElementItem, getItemsArrayOfItem, itemsEqual, WRAPPER_LOCAL_NAME } from './vaadin-dashboard-helpers.js'; const REORDER_EVENT_TIMEOUT = 200; +const REORDER_MOVE_THRESHOLD = 10; /** * A controller to widget reordering inside a dashboard. @@ -64,6 +65,29 @@ export class WidgetReorderController { return; } + // The location against which we measure the drag distance + this.__startX ||= e.clientX; + this.__startY ||= e.clientY; + + // Delta values for the current drag event + const currentDx = e.clientX - (this.__previousX || e.clientX); + const currentDy = e.clientY - (this.__previousY || e.clientY); + this.__previousX = e.clientX; + this.__previousY = e.clientY; + + if (currentDx && this.__dx && Math.sign(currentDx) !== Math.sign(this.__dx)) { + // If the direction of the drag changes, reset the start position + this.__startX = e.clientX; + } + if (currentDy && this.__dy && Math.sign(currentDy) !== Math.sign(this.__dy)) { + // If the direction of the drag changes, reset the start position + this.__startY = e.clientY; + } + + // The delta values for the drag event against the start position + this.__dx = e.clientX - this.__startX; + this.__dy = e.clientY - this.__startY; + e.preventDefault(); e.dataTransfer.dropEffect = 'move'; @@ -88,7 +112,15 @@ export class WidgetReorderController { } // Check if the dragged element is dragged enough over the target element - if (!this.__reordering && this.__isDraggedOver(draggedElement, targetElement, e.clientX, e.clientY)) { + if ( + !this.__reordering && + this.__isDraggedOver(draggedElement, targetElement, { + x: e.clientX, + y: e.clientY, + dx: this.__dx, + dy: this.__dy, + }) + ) { // Prevent reordering multiple times in quick succession this.__reordering = true; setTimeout(() => { @@ -109,6 +141,13 @@ export class WidgetReorderController { return; } + this.__previousX = null; + this.__previousY = null; + this.__startX = null; + this.__startY = null; + this.__dx = null; + this.__dy = null; + // If the originally dragged element is restored to the DOM (as a direct child of the host), // to make sure "dragend" event gets dispatched, remove it to avoid duplicates if (this.__draggedElement.parentElement === this.host) { @@ -169,21 +208,21 @@ export class WidgetReorderController { * of the drag event. * @private */ - __isDraggedOver(draggedElement, targetElement, x, y) { + __isDraggedOver(draggedElement, targetElement, { x, y, dx, dy }) { const draggedPos = draggedElement.getBoundingClientRect(); const targetPos = targetElement.getBoundingClientRect(); if (draggedPos.top >= targetPos.bottom) { // target is on a row above the dragged widget - return y < targetPos.top + targetPos.height / 2; + return y < targetPos.bottom && dy < -REORDER_MOVE_THRESHOLD; } else if (draggedPos.bottom <= targetPos.top) { // target is on a row below the dragged widget - return y > targetPos.top + targetPos.height / 2; + return y > targetPos.top && dy > REORDER_MOVE_THRESHOLD; } else if (draggedPos.left >= targetPos.right) { // target is on a column to the left of the dragged widget - return x < targetPos.left + targetPos.width / 2; + return x < targetPos.right && dx < -REORDER_MOVE_THRESHOLD; } else if (draggedPos.right <= targetPos.left) { // target is on a column to the right of the dragged widget - return x > targetPos.left + targetPos.width / 2; + return x > targetPos.left && dx > REORDER_MOVE_THRESHOLD; } } diff --git a/packages/dashboard/test/dashboard-widget-reordering.test.ts b/packages/dashboard/test/dashboard-widget-reordering.test.ts index 10f79bb5a9..23adc00c84 100644 --- a/packages/dashboard/test/dashboard-widget-reordering.test.ts +++ b/packages/dashboard/test/dashboard-widget-reordering.test.ts @@ -79,19 +79,6 @@ describe('dashboard - widget reordering', () => { ]); }); - it('should not reorder if dragged barely over another widget (start -> end)', async () => { - fireDragStart(getElementFromCell(dashboard, 0, 0)!); - await nextFrame(); - - fireDragOver(getElementFromCell(dashboard, 0, 1)!, 'start'); - await nextFrame(); - - // prettier-ignore - expectLayout(dashboard, [ - [0, 1], - ]); - }); - it('should reorder the widgets while dragging (end -> start)', async () => { fireDragStart(getElementFromCell(dashboard, 0, 1)!); await nextFrame(); @@ -105,19 +92,6 @@ describe('dashboard - widget reordering', () => { ]); }); - it('should not reorder if dragged barely over another widget (end -> start)', async () => { - fireDragStart(getElementFromCell(dashboard, 0, 1)!); - await nextFrame(); - - fireDragOver(getElementFromCell(dashboard, 0, 0)!, 'end'); - await nextFrame(); - - // prettier-ignore - expectLayout(dashboard, [ - [0, 1], - ]); - }); - it('should reorder the widgets while dragging (top -> bottom)', async () => { dashboard.style.width = `${columnWidth}px`; await onceResized(dashboard); @@ -141,29 +115,6 @@ describe('dashboard - widget reordering', () => { ]); }); - it('should not reorder if dragged barely over another widget (top -> bottom)', async () => { - dashboard.style.width = `${columnWidth}px`; - await onceResized(dashboard); - - // prettier-ignore - expectLayout(dashboard, [ - [0], - [1], - ]); - - fireDragStart(getElementFromCell(dashboard, 0, 0)!); - await nextFrame(); - - fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'top'); - await nextFrame(); - - // prettier-ignore - expectLayout(dashboard, [ - [0], - [1], - ]); - }); - it('should reorder the widgets while dragging (bottom -> top)', async () => { dashboard.style.width = `${columnWidth}px`; await onceResized(dashboard); @@ -187,29 +138,6 @@ describe('dashboard - widget reordering', () => { ]); }); - it('should not reorder if dragged barely over another widget (bottom -> top)', async () => { - dashboard.style.width = `${columnWidth}px`; - await onceResized(dashboard); - - // prettier-ignore - expectLayout(dashboard, [ - [0], - [1], - ]); - - fireDragStart(getElementFromCell(dashboard, 1, 0)!); - await nextFrame(); - - fireDragOver(getElementFromCell(dashboard, 0, 0)!, 'bottom'); - await nextFrame(); - - // prettier-ignore - expectLayout(dashboard, [ - [0], - [1], - ]); - }); - // The sendMouse helper does not seem to work well with Firefox (isFirefox ? it.skip : it)('should reorder using native DnD', async () => { const spy = sinon.spy(); @@ -221,6 +149,17 @@ describe('dashboard - widget reordering', () => { await sendMouse({ type: 'down', }); + // Start moving the mouse + const draggedWidget = getElementFromCell(dashboard, 0, 0); + const draggedWidgetRect = draggedWidget!.getBoundingClientRect(); + const startY = draggedWidgetRect.top + draggedWidgetRect.height / 2; + const startX = document.dir === 'rtl' ? draggedWidgetRect.right - 1 : draggedWidgetRect.left + 1; + // await sendMouse({ type: 'move', position: [Math.round(startX), Math.round(startY)] }); + // Workaround an issue with sendMouse not supporting multiple subsequent moves + const reorderController = (dashboard as any).__widgetReorderController; + reorderController.__startX = startX; + reorderController.__startY = startY; + // Move the mouse to the second widget const secondWidget = getElementFromCell(dashboard, 0, 1)!; const secondWidgetRect = secondWidget.getBoundingClientRect(); @@ -471,6 +410,10 @@ describe('dashboard - widget reordering', () => { [0], ]); + // Workaround test limitations to have proper delta on the next dragover event + fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'bottom'); + fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'top'); + // Drag over the new widget 2 resetReorderTimeout(dashboard); fireDragOver(getElementFromCell(dashboard, 0, 1)!, 'top'); diff --git a/packages/dashboard/test/dashboard.test.ts b/packages/dashboard/test/dashboard.test.ts index 77b4b5e3ca..6b9493ba4b 100644 --- a/packages/dashboard/test/dashboard.test.ts +++ b/packages/dashboard/test/dashboard.test.ts @@ -63,6 +63,7 @@ describe('dashboard', () => { it('should render a new widget', async () => { dashboard.items = [...dashboard.items, { id: '2' }]; await nextFrame(); + await nextFrame(); const newWidget = getElementFromCell(dashboard, 1, 0); expect(newWidget).to.be.ok; @@ -117,12 +118,14 @@ describe('dashboard', () => { expect(spy.firstCall.args[0].detail.items).to.eql([{ id: '0' }]); }); - it('should not dispatch an item-remove event', () => { + it('should not dispatch an item-remove event', async () => { const spy = sinon.spy(); // @ts-ignore unexpected event type dashboard.addEventListener('item-remove', spy); + await nextFrame(); const widget = getElementFromCell(dashboard, 0, 1); getRemoveButton(widget as DashboardWidget).click(); + await nextFrame(); expect(spy).to.not.be.called; }); diff --git a/packages/dashboard/test/helpers.ts b/packages/dashboard/test/helpers.ts index 1b270b208b..8d751fed8e 100644 --- a/packages/dashboard/test/helpers.ts +++ b/packages/dashboard/test/helpers.ts @@ -224,17 +224,6 @@ export function createDragEvent(type: string, { x, y }: { x: number; y: number } return event; } -export function fireDragStart(dragStartTarget: Element): TestDragEvent { - const draggable = getDraggable(dragStartTarget); - const draggableRect = draggable.getBoundingClientRect(); - const event = createDragEvent('dragstart', { - x: draggableRect.left + draggableRect.width / 2, - y: draggableRect.top + draggableRect.height / 2, - }); - draggable.dispatchEvent(event); - return event; -} - function getEventCoordinates(relativeElement: Element, location: 'top' | 'bottom' | 'start' | 'end') { const { top, bottom, left, right } = relativeElement.getBoundingClientRect(); const y = location === 'top' ? top : bottom; @@ -249,6 +238,26 @@ export function fireDragOver(dragOverTarget: Element, location: 'top' | 'bottom' return event; } +export function fireDragStart(dragStartTarget: Element): TestDragEvent { + const draggable = getDraggable(dragStartTarget); + const draggableRect = draggable.getBoundingClientRect(); + const event = createDragEvent('dragstart', { + x: draggableRect.left + draggableRect.width / 2, + y: draggableRect.top + draggableRect.height / 2, + }); + draggable.dispatchEvent(event); + + // Dispatch initial dragover event to make sure any following dragover has some delta + draggable.dispatchEvent( + createDragEvent('dragover', { + x: draggableRect.left + draggableRect.width / 2, + y: draggableRect.top + draggableRect.height / 2, + }), + ); + + return event; +} + export function fireDragEnd(dashboard: Element): TestDragEvent { const event = createDragEvent('dragend', { x: 0, y: 0 }); dashboard.dispatchEvent(event);