Browse Source

Fix stale drop target on slow drags in sorter (#6798)

The stale-target cache in DropLocationDeterminer#getValidParent compared
the current drop-bounds check (container-relative coords) against one
computed from the last mouse event's clientX/clientY (viewport coords),
making the comparison meaningless. On slow drags, where the hovered node
and index stay the same between move samples, the cached target was
returned even after the pointer entered an edge band that should
escalate the target to the parent.

Store the drop-bounds result of each move in lastMoveData and compare
against it, keeping the whole check in a single coordinate space.

Introduced in #6542 (0.22.10).
pull/6803/head
Garvish Panchal 2 months ago
committed by GitHub
parent
commit
c27b787192
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
  1. 10
      packages/core/src/utils/sorter/DropLocationDeterminer.ts
  2. 67
      packages/core/test/specs/utils/sorter/sorterLayers.ts

10
packages/core/src/utils/sorter/DropLocationDeterminer.ts

@ -42,6 +42,8 @@ type lastMoveData<NodeType> = {
placement?: Placement;
/** The mouse event, used if we want to move placeholder with scrolling. */
mouseEvent?: MouseEvent;
/** Whether the mouse was within the hovered node's drop bounds during the last move. */
hoveredWithinBounds?: boolean;
placeholderDimensions?: Dimension;
};
@ -161,6 +163,7 @@ export class DropLocationDeterminer<T, NodeType extends SortableTreeNode<T>> ext
mouseEvent,
index,
hoveredIndex,
hoveredWithinBounds: hoveredNode.isWithinDropBounds(mouseX, mouseY),
placement,
placeholderDimensions,
};
@ -372,7 +375,7 @@ export class DropLocationDeterminer<T, NodeType extends SortableTreeNode<T>> ext
targetNode: lastTargetNode,
hoveredNode: lastHoveredNode,
hoveredIndex: lastHoveredIndex,
mouseEvent: lastMouseEvent,
hoveredWithinBounds: lastHoveredWithinBounds,
} = this.lastMoveData;
const sameHoveredNode = targetNode.equals(lastHoveredNode);
@ -380,10 +383,7 @@ export class DropLocationDeterminer<T, NodeType extends SortableTreeNode<T>> ext
const hoverIndex = this.getIndexInParent(targetNode, targetNode.nodeDimensions!, mouseX, mouseY);
const sameHoveredIndex = hoverIndex === lastHoveredIndex;
const isWithinDropArea = targetNode.isWithinDropBounds(mouseX, mouseY);
const sameHoverPosition =
sameHoveredNode &&
sameHoveredIndex &&
isWithinDropArea === targetNode.isWithinDropBounds(lastMouseEvent?.clientX ?? 0, lastMouseEvent?.clientY ?? 0);
const sameHoverPosition = sameHoveredNode && sameHoveredIndex && isWithinDropArea === lastHoveredWithinBounds;
if (sameHoverPosition && lastTargetNode) return lastTargetNode;

67
packages/core/test/specs/utils/sorter/sorterLayers.ts

@ -1,5 +1,6 @@
import Component from '../../../../src/dom_components/model/Component';
import Editor from '../../../../src/editor';
import Dimension from '../../../../src/utils/sorter/Dimension';
import LayersComponentNode from '../../../../src/utils/sorter/LayersComponentNode';
import { setupTestEditor } from '../../../common';
@ -69,4 +70,70 @@ describe('Layers sorter', () => {
expect(getChildIds(wrapper)).toEqual(['visible-a', 'hidden-a', 'visible-b', 'hidden-b']);
});
test('escalates the drop target to the parent when a slow drag leaves the drop bounds without changing node or index', () => {
editor.setComponents(`
<div id="block-a">Block A</div>
<div id="block-b">Block B</div>
<div id="section-c"><div id="child-c1">Child C1</div></div>
`);
const wrapper = editor.getWrapper()!;
const source = wrapper.find('#block-a')[0];
const blockB = wrapper.find('#block-b')[0];
const sectionC = wrapper.find('#section-c')[0];
editor.select(source);
editor.Layers.setRoot(wrapper);
fixtures.appendChild(editor.Layers.render());
const sorter = source.viewLayer!.sorter;
const determiner: any = sorter.dropLocationDeterminer;
const sectionEl = sectionC.viewLayer!.el;
// Container-relative boxes for a vertical layer list. The section's drop
// area (ratio 0.4, min 3px, max 20px) shrinks 76-124 to 90.4-109.6.
const box = (top: number, height: number) =>
new Dimension({ top, left: 0, height, width: 200, offsets: {} as any, dir: true });
const boxes = new Map<HTMLElement | undefined, Dimension>([
[source.viewLayer!.el, box(28, 24)],
[blockB.viewLayer!.el, box(52, 24)],
[sectionEl, box(76, 48)],
]);
const wrapperBox = () => box(0, 300);
// The container offset makes viewport and container coordinates diverge,
// as they do in a real page where the layer panel is not at the origin.
const offsetTop = 200;
jest.spyOn(determiner, 'cacheContainerPosition').mockImplementation(() => {
determiner.containerOffset = { top: offsetTop, left: 0 };
});
jest.spyOn(determiner, 'getDim').mockImplementation((el: any) => boxes.get(el)?.clone() ?? wrapperBox());
jest.spyOn(determiner, 'getDirection').mockReturnValue(true);
jest.spyOn(determiner, 'getMouseTargetElement').mockReturnValue(sectionEl);
jest.spyOn(determiner, 'getFirstElementWithAModel').mockReturnValue(sectionEl);
// Placeholder placement is downstream of target selection and needs
// rendered child layers, which jsdom cannot lay out.
jest.spyOn(determiner, 'getDropPosition').mockImplementation(() => ({
index: 0,
placement: 'before',
placeholderDimensions: box(0, 0),
}));
sorter.startSort([{ element: source.viewLayer!.el }]);
const move = (containerY: number) =>
determiner.handleMove(new MouseEvent('mousemove', { clientX: 100, clientY: containerY + offsetTop }));
// Inside the section's drop area: the section itself is the target.
move(95);
expect(determiner.lastMoveData.targetNode?.model).toBe(sectionC);
// Slow drag into the edge band above the drop area: same hovered node,
// same index ('before' in both samples), but the target must escalate
// to the wrapper instead of sticking to the cached section.
move(85);
expect(determiner.lastMoveData.targetNode?.model).toBe(wrapper);
determiner.cancelDrag();
});
});

Loading…
Cancel
Save