sadpandajoe commented on code in PR #45090:
URL: https://github.com/apache/superset/pull/45090#discussion_r4249894232
##########
superset/canvas/api.py:
##########
@@ -548,6 +567,16 @@ def get_definition(self, id_or_uuid: str) -> Response:
gridColumns:
type: object
description: The column count of each grid
container
+ layoutConstraints:
+ type: object
+ description: >-
+ The span limits each node's widget declares, for
+ a client that lets the user resize
+ canEdit:
Review Comment:
`canEdit` and `layoutConstraints` are documented here and typed in the
frontend, but `docs/static/resources/openapi.json` isn't regenerated, so the
spec still lacks both fields and `check-openapi-spec-drift` fails on exactly
those lines. Clients generated from the published spec can't see either field.
Can the spec be regenerated in this PR?
##########
superset-frontend/src/pages/Canvas/index.tsx:
##########
@@ -101,10 +117,97 @@ const notFound = (
/>
);
+interface CanvasBodyProps {
+ canvasId: number;
+ result: CanvasDefinitionResult;
+ reload: () => void;
+ values: ScopeValues;
+ onValueChange: (
+ kind: canvasApi.ScopeKind,
+ nodeId: string,
+ value: unknown,
+ ) => void;
+ refreshKeys: Record<string, number>;
+ /** Set while a draft is open; layout writes then go to the draft. */
+ draftToken?: string;
+ /** Leaves the draft, returning to the published canvas. */
+ onDraftClosed: () => void;
+ /** True when the canvas holds no widgets yet. */
+ isEmpty: boolean;
+}
+
+/**
+ * The canvas itself, once its definition has loaded. Drag and resize persist
+ * from here; a write that doesn't land says so and leaves the layout where the
+ * server still has it.
+ */
+function CanvasBody({
+ canvasId,
+ result,
+ reload,
+ values,
+ onValueChange,
+ refreshKeys,
+ draftToken,
+ onDraftClosed,
+ isEmpty,
+}: CanvasBodyProps) {
+ const layout = useCanvasLayout(canvasId, result, reload, draftToken);
+ // Changes are made in a plan; the published canvas is read-only even for
+ // someone who may edit it. The single place this policy is decided.
+ const editable = result.canEdit && Boolean(draftToken);
Review Comment:
`editable` stays true while Publish is processing; the commit's busy state
lives only inside `DraftBanner`, so the grid and `place()` don't know it. If
the user resizes a widget after the commit has read revision N but before it
deletes the draft, that PATCH is acknowledged at N+1, then the commit replays
only N's operations and deletes the draft, so the accepted resize is lost while
Publish reports success. Should gestures be blocked while a publish or discard
is running (e.g. lift the banner's busy state up and fold it into `editable`)?
##########
superset-frontend/src/features/canvas/CanvasGridSurface.tsx:
##########
@@ -0,0 +1,457 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * @fileoverview One grid of a canvas, with drag and resize built in.
+ *
+ * There is no edit mode: a user who may edit gets a drag grip and a resize
+ * corner on each widget, on the canvas as they view it. A user who may not
+ * gets the same grid with no handles. Dragging never covers the widget's own
+ * surface, so a filter or chart underneath stays clickable.
+ *
+ * While a gesture is live the widget follows the pointer and a dashed outline
+ * marks the cells it would take; releasing persists exactly those cells.
+ */
+
+import {
+ PointerEvent as ReactPointerEvent,
+ KeyboardEvent as ReactKeyboardEvent,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useRef,
+ useState,
+} from 'react';
+import { css, styled } from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import { Icons } from '@superset-ui/core/components';
+import {
+ GridMetrics,
+ gridDelta,
+ movedPlacement,
+ NO_DELTA,
+ overlappedSiblings,
+ resizedPlacement,
+ samePlacement,
+} from './gridGeometry';
+import type { GridPlacement, SpanConstraints } from './types';
+
+type GestureKind = 'move' | 'resize';
+
+interface Gesture {
+ nodeId: string;
+ kind: GestureKind;
+ pointerId: number;
+ startX: number;
+ startY: number;
+ dx: number;
+ dy: number;
+ /**
+ * Where the widget sat when it was grabbed. The gesture resolves against
+ * this, not against the current placement: a write landing mid-gesture (a
+ * sibling's drop pushing this one down, or a draft poll) would otherwise
+ * move the ground under the pointer and persist the drop relative to a row
+ * the user never aimed at.
+ */
+ origin: GridPlacement;
+}
+
+const Grid = styled.div<GridMetrics>`
+ display: grid;
+ grid-template-columns: repeat(${({ columns }) => columns}, minmax(0, 1fr));
+ grid-auto-rows: ${({ rowUnit }) => rowUnit}px;
+ gap: ${({ gap }) => gap}px;
+`;
+
+const GridItem = styled.div<{
+ placement?: GridPlacement;
+ dragging?: boolean;
+ offset?: { x: number; y: number };
+}>`
+ ${({ placement, dragging, offset }) => css`
+ min-width: 0;
+ min-height: 0;
+ position: relative;
+ ${
+ placement &&
+ css`
+ grid-column: ${placement.col} / span ${placement.colSpan};
+ grid-row: ${placement.row} / span ${placement.rowSpan};
+ `
+ }
+ ${
+ dragging &&
+ offset &&
+ css`
+ /* Follow the pointer without reflowing the grid under it. */
+ transform: translate(${offset.x}px, ${offset.y}px);
+ z-index: 2;
+ opacity: 0.85;
+ `
+ }
+ `}
+`;
+
+/**
+ * Where the widget being dragged would land. Turns red once it covers a
+ * sibling, so the user sees an overlap before releasing rather than being
+ * surprised when the server pushes a widget down to resolve it.
+ */
+const DropPreview = styled.div<{
+ placement: GridPlacement;
+ colliding?: boolean;
+}>`
+ ${({ theme, placement, colliding }) => css`
+ grid-column: ${placement.col} / span ${placement.colSpan};
+ grid-row: ${placement.row} / span ${placement.rowSpan};
+ border: 2px dashed ${colliding ? theme.colorError : theme.colorPrimary};
+ border-radius: ${theme.borderRadius}px;
+ background: ${colliding ? theme.colorErrorBg : theme.colorPrimaryBg};
+ pointer-events: none;
+ z-index: 1;
+ `}
+`;
+
+/**
+ * Wraps a widget so its handles can sit on top of it. The handles stay hidden
+ * until the pointer is over the widget or a handle has focus, so a canvas at
+ * rest looks the same whether or not the user may edit it.
+ */
+const Interactive = styled.div`
+ ${({ theme }) => css`
+ height: 100%;
+ position: relative;
+
+ /* Direct children only: hovering a container must not reveal the
+ handles of every widget nested inside it. */
+ & > .canvas-handle {
+ opacity: 0;
+ transition: opacity ${theme.motionDurationMid};
+ }
+
+ &:hover > .canvas-handle,
+ & > .canvas-handle:focus-visible {
+ opacity: 1;
+ }
+ `}
+`;
+
+const Grip = styled.button`
+ ${({ theme }) => css`
+ position: absolute;
+ top: ${theme.sizeUnit}px;
+ right: ${theme.sizeUnit}px;
+ z-index: 3;
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ padding: ${theme.sizeUnit / 2}px;
+ border: 1px solid ${theme.colorBorder};
+ border-radius: ${theme.borderRadiusSM}px;
+ background: ${theme.colorBgElevated};
+ color: ${theme.colorTextSecondary};
+ cursor: grab;
+ touch-action: none;
+
+ &:active {
+ cursor: grabbing;
+ }
+ `}
+`;
+
+const ResizeCorner = styled.button`
+ ${({ theme }) => css`
+ position: absolute;
+ right: 0;
+ bottom: 0;
+ z-index: 3;
+ width: ${theme.sizeUnit * 4}px;
+ height: ${theme.sizeUnit * 4}px;
+ padding: 0;
+ border: none;
+ background: transparent;
+ cursor: nwse-resize;
+ touch-action: none;
+
+ /* A two-line corner, the conventional resize affordance. */
+ &::after {
+ content: '';
+ position: absolute;
+ right: ${theme.sizeUnit / 2}px;
+ bottom: ${theme.sizeUnit / 2}px;
+ width: ${theme.sizeUnit * 2}px;
+ height: ${theme.sizeUnit * 2}px;
+ border-right: 2px solid ${theme.colorTextTertiary};
+ border-bottom: 2px solid ${theme.colorTextTertiary};
+ }
+ `}
+`;
+
+/** The grid's content width, kept current as the viewport changes. */
+function useElementWidth(): [(element: HTMLDivElement | null) => void, number]
{
+ const [element, setElement] = useState<HTMLDivElement | null>(null);
+ const [width, setWidth] = useState(0);
+
+ useEffect(() => {
+ if (!element) return undefined;
+ const measure = () => setWidth(element.clientWidth);
+ measure();
+ if (typeof ResizeObserver === 'undefined') return undefined;
+ const observer = new ResizeObserver(measure);
+ observer.observe(element);
+ return () => observer.disconnect();
+ }, [element]);
+
+ return [setElement, width];
+}
+
+export interface CanvasGridSurfaceProps {
+ childIds: string[];
+ columns: number;
+ gap: number;
+ rowUnit: number;
+ placements: Record<string, GridPlacement>;
+ constraints: Record<string, SpanConstraints>;
+ /** Whether the user may drag and resize these widgets. */
+ editable: boolean;
+ onPlace: (nodeId: string, placement: GridPlacement) => void;
+ renderNode: (nodeId: string) => ReactNode;
+}
+
+export default function CanvasGridSurface({
+ childIds,
+ columns,
+ gap,
+ rowUnit,
+ placements,
+ constraints,
+ editable,
+ onPlace,
+ renderNode,
+}: CanvasGridSurfaceProps) {
+ const [gridRef, width] = useElementWidth();
+ const [gesture, setGesture] = useState<Gesture>();
+ // The handlers read the gesture directly, so committing never runs as a
+ // side effect inside a state updater.
+ const live = useRef<Gesture>();
+
+ const update = useCallback((next: Gesture | undefined) => {
+ live.current = next;
+ setGesture(next);
+ }, []);
+
+ /** Where `from` would land given a pointer delta, in whole grid units. */
+ const targetOf = useCallback(
+ (
+ nodeId: string,
+ kind: GestureKind,
+ from: GridPlacement,
+ dx: number,
+ dy: number,
+ ) => {
+ const metrics: GridMetrics = { columns, gap, rowUnit };
+ const delta = width > 0 ? gridDelta(dx, dy, width, metrics) : NO_DELTA;
+ return kind === 'move'
+ ? movedPlacement(from, delta, columns)
+ : resizedPlacement(from, delta, columns, constraints[nodeId]);
+ },
+ [width, columns, gap, rowUnit, constraints],
+ );
+
+ const commit = useCallback(
+ (nodeId: string, next: GridPlacement | undefined) => {
+ const placement = placements[nodeId];
+ if (next && placement && !samePlacement(next, placement)) {
+ onPlace(nodeId, next);
+ }
+ },
+ [onPlace, placements],
+ );
+
+ const begin =
+ (nodeId: string, kind: GestureKind) =>
+ (event: ReactPointerEvent<HTMLButtonElement>) => {
+ if (event.button !== 0) return;
Review Comment:
`begin` only checks `button`, not whether `live.current` already holds a
gesture. A second finger touching another widget's handle also reports `button
=== 0`, so it overwrites the first gesture; the first finger's remaining
move/up events then fail the `pointerId` guards and its drag is silently
dropped. Should `begin` return early when a gesture is already active?
##########
superset-frontend/src/features/canvas/CanvasGridSurface.test.tsx:
##########
@@ -0,0 +1,359 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { fireEvent, render, screen } from 'spec/helpers/testing-library';
+import CanvasGridSurface from './CanvasGridSurface';
+import type { GridPlacement, SpanConstraints } from './types';
+
+// jsdom lays nothing out, so the grid is told how wide it is: 1000px over 24
+// columns with 16px gaps gives a 42.33px column pitch and a 56px row pitch.
+const WIDTH = 1000;
+const COL_PITCH = 632 / 24 + 16;
+const ROW_PITCH = 56;
+
+const placements: Record<string, GridPlacement> = {
+ kpi: { col: 1, row: 1, colSpan: 6, rowSpan: 4 },
+ trend: { col: 1, row: 5, colSpan: 24, rowSpan: 8 },
+};
+
+const surface = (
+ live: Record<string, GridPlacement>,
+ editable: boolean,
+ onPlace: jest.Mock,
+ constraints: Record<string, SpanConstraints>,
+) => (
+ <CanvasGridSurface
+ childIds={Object.keys(live)}
+ columns={24}
+ gap={16}
+ rowUnit={40}
+ placements={live}
+ constraints={constraints}
+ editable={editable}
+ onPlace={onPlace}
+ renderNode={nodeId => <div>{`widget ${nodeId}`}</div>}
+ />
+);
+
+const renderSurface = ({
+ editable = true,
+ onPlace = jest.fn(),
+ constraints = {} as Record<string, SpanConstraints>,
+} = {}) => {
+ const { rerender } = render(
+ surface(placements, editable, onPlace, constraints),
+ );
+ return Object.assign(onPlace, {
+ /** Simulate a write landing while a gesture is still live. */
+ replace: (next: Record<string, GridPlacement>) =>
+ rerender(surface(next, editable, onPlace, constraints)),
+ });
+};
+
+/**
+ * jsdom's `PointerEvent` carries none of the properties a gesture needs, so
+ * the pointer events are built on `MouseEvent`, which does support
+ * coordinates and buttons, with the pointer id added on top.
+ */
+class TestPointerEvent extends MouseEvent {
+ pointerId: number;
+
+ constructor(
+ type: string,
+ init: MouseEventInit & { pointerId?: number } = {},
+ ) {
+ super(type, { bubbles: true, cancelable: true, ...init });
+ this.pointerId = init.pointerId ?? 1;
+ }
+}
+
+const pointer = (
+ handle: HTMLElement,
+ type: 'pointerdown' | 'pointermove' | 'pointerup' | 'pointercancel',
+ init: MouseEventInit & { pointerId?: number } = {},
+) => fireEvent(handle, new TestPointerEvent(type, { pointerId: 1, ...init }));
+
+/** A whole gesture on `handle`, from grab to release. */
+const drag = (handle: HTMLElement, dx: number, dy: number) => {
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: dx, clientY: dy });
+ pointer(handle, 'pointerup', { clientX: dx, clientY: dy });
+};
+
+const handles = (name: 'Move widget' | 'Resize widget') =>
+ screen.getAllByRole('button', { name });
+
+beforeAll(() => {
+ Object.defineProperty(HTMLElement.prototype, 'clientWidth', {
+ configurable: true,
+ get: () => WIDTH,
+ });
+ // jsdom has no pointer capture; the gesture only needs it not to throw.
+ HTMLElement.prototype.setPointerCapture = jest.fn();
+ HTMLElement.prototype.releasePointerCapture = jest.fn();
+});
+
+test('dragging a widget persists the cells it was dropped on', () => {
+ const onPlace = renderSurface();
+
+ drag(handles('Move widget')[0], COL_PITCH * 4, ROW_PITCH * 2);
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 5,
+ row: 3,
+ colSpan: 6,
+ rowSpan: 4,
+ });
+});
+
+test('resizing a widget persists the new spans and leaves its origin alone',
() => {
+ const onPlace = renderSurface();
+
+ drag(handles('Resize widget')[0], COL_PITCH * 3, ROW_PITCH);
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 1,
+ row: 1,
+ colSpan: 9,
+ rowSpan: 5,
+ });
+});
+
+test('a resize stops at the span limits the widget declares', () => {
+ const onPlace = renderSurface({ constraints: { kpi: { maxColSpan: 8 } } });
+
+ drag(handles('Resize widget')[0], COL_PITCH * 10, 0);
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 1,
+ row: 1,
+ colSpan: 8,
+ rowSpan: 4,
+ });
+});
+
+test('a gesture too small to reach the next cell changes nothing', () => {
+ const onPlace = renderSurface();
+
+ drag(handles('Move widget')[0], 8, 10);
+
+ expect(onPlace).not.toHaveBeenCalled();
+});
+
+test('a cancelled gesture is not persisted', () => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: COL_PITCH * 4, clientY: 0 });
+ pointer(handle, 'pointercancel');
+
+ expect(onPlace).not.toHaveBeenCalled();
+});
+
+test('a drag shows where the widget would land', () => {
+ renderSurface();
+ const handle = handles('Move widget')[0];
+
+ expect(screen.queryByTestId('canvas-drop-preview')).not.toBeInTheDocument();
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: COL_PITCH * 4, clientY: 0 });
+
+ expect(screen.getByTestId('canvas-drop-preview')).toBeInTheDocument();
+
+ pointer(handle, 'pointerup', { clientX: COL_PITCH * 4, clientY: 0 });
+
+ expect(screen.queryByTestId('canvas-drop-preview')).not.toBeInTheDocument();
+});
+
+test('arrow keys move a widget a cell at a time', () => {
+ const onPlace = renderSurface();
+
+ fireEvent.keyDown(handles('Move widget')[0], { key: 'ArrowRight' });
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 2,
+ row: 1,
+ colSpan: 6,
+ rowSpan: 4,
+ });
+});
+
+test('arrow keys on the resize corner resize a cell at a time', () => {
+ const onPlace = renderSurface();
+
+ fireEvent.keyDown(handles('Resize widget')[0], { key: 'ArrowDown' });
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 1,
+ row: 1,
+ colSpan: 6,
+ rowSpan: 5,
+ });
+});
+
+test('a non-primary button does not start a gesture', () => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 2, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: COL_PITCH * 4, clientY: 0 });
+ pointer(handle, 'pointerup', { clientX: COL_PITCH * 4, clientY: 0 });
+
+ expect(onPlace).not.toHaveBeenCalled();
+});
+
+test('a view-only surface renders the widgets with no handles', () => {
+ renderSurface({ editable: false });
+
+ expect(screen.getByText('widget kpi')).toBeInTheDocument();
+ expect(
+ screen.queryByRole('button', { name: 'Move widget' }),
+ ).not.toBeInTheDocument();
+ expect(
+ screen.queryByRole('button', { name: 'Resize widget' }),
+ ).not.toBeInTheDocument();
+});
+
+test('the drop preview warns while it covers another widget', () => {
+ renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ // 'kpi' is 6x4 at row 1; four rows down puts it on 'trend' at row 5.
+ pointer(handle, 'pointermove', { clientX: 0, clientY: ROW_PITCH * 4 });
+
+ const preview = screen.getByTestId('canvas-drop-preview');
+ expect(preview).toHaveAttribute('data-colliding', 'true');
+ expect(preview).toHaveAttribute('title', 'This overlaps another widget');
+});
+
+test('the drop preview stays neutral over free space', () => {
+ renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ // Straight right, staying on row 1 where nothing else sits.
+ pointer(handle, 'pointermove', { clientX: COL_PITCH * 8, clientY: 0 });
+
+ const preview = screen.getByTestId('canvas-drop-preview');
+ expect(preview).toHaveAttribute('data-colliding', 'false');
+ expect(preview).not.toHaveAttribute('title');
+});
+
+test('a resize that grows onto a neighbour warns too', () => {
+ renderSurface();
+ const handle = handles('Resize widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ // Grow 'kpi' four rows taller, reaching into 'trend'.
+ pointer(handle, 'pointermove', { clientX: 0, clientY: ROW_PITCH * 4 });
+
+ expect(screen.getByTestId('canvas-drop-preview')).toHaveAttribute(
+ 'data-colliding',
+ 'true',
+ );
+});
+
+test('the drop lands where the pointer was released, not where it last moved',
() => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: COL_PITCH, clientY: 0 });
+ // The pointer travels further before the release reports its own position.
+ pointer(handle, 'pointerup', { clientX: COL_PITCH * 5, clientY: 0 });
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 6,
+ row: 1,
+ colSpan: 6,
+ rowSpan: 4,
+ });
+});
+
+test('a cancel from a different pointer leaves the gesture alone', () => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ pointer(handle, 'pointerdown', {
+ button: 0,
+ pointerId: 1,
+ clientX: 0,
+ clientY: 0,
+ });
+ pointer(handle, 'pointermove', {
+ pointerId: 1,
+ clientX: COL_PITCH * 4,
+ clientY: 0,
+ });
+ // A second pointer going away must not discard the first one's drag.
+ pointer(handle, 'pointercancel', { pointerId: 2 });
+ pointer(handle, 'pointerup', {
+ pointerId: 1,
+ clientX: COL_PITCH * 4,
+ clientY: 0,
+ });
+
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 5,
+ row: 1,
+ colSpan: 6,
+ rowSpan: 4,
+ });
+});
+
+test('a write landing mid-gesture does not move the ground under the pointer',
() => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ // Grab 'kpi' at row 1 and drag two rows down: it is heading for row 3.
+ pointer(handle, 'pointerdown', { button: 0, clientX: 0, clientY: 0 });
+ pointer(handle, 'pointermove', { clientX: 0, clientY: ROW_PITCH * 2 });
+
+ // Mid-gesture, the server resolves a sibling's drop and pushes 'kpi' to
+ // row 9 — the kind of thing a draft poll or another drag's response does.
+ onPlace.replace({
+ ...placements,
+ kpi: { col: 1, row: 9, colSpan: 6, rowSpan: 4 },
+ });
+
+ pointer(handle, 'pointerup', { clientX: 0, clientY: ROW_PITCH * 2 });
+
+ // The drop is two rows from where the user grabbed it, not from row 9.
+ expect(onPlace).toHaveBeenCalledWith('kpi', {
+ col: 1,
+ row: 3,
+ colSpan: 6,
+ rowSpan: 4,
+ });
+});
+
+test('a gesture cannot start on a widget with no placement', () => {
+ const onPlace = renderSurface();
+ const handle = handles('Move widget')[0];
+
+ onPlace.replace({ trend: placements.trend });
Review Comment:
`childIds` is derived from `Object.keys(live)`, so replacing the placements
with `{ trend }` unmounts `kpi` and its handle; the events on lines 354-356
then hit a detached node and never reach the React handlers, so `onPlace` is
uncalled whether or not `begin` guards on a missing placement. This would still
pass if the `if (!origin) return` check were removed. Could the test keep `kpi`
in `childIds` while omitting only its placement, and assert its content still
renders without handles?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
