sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4282746654
########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,528 @@ +/** + * 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; + } + `} +`; + +/** Vertical step between a container's handles and its children's. */ +const HANDLE_STEP = 8; +/** Beyond this, nesting stops pushing handles further down the widget. */ +const MAX_HANDLE_STEPS = 3; + +const handleTop = (theme: { sizeUnit: number }, depth: number) => + theme.sizeUnit * (1 + Math.min(depth, MAX_HANDLE_STEPS) * HANDLE_STEP); Review Comment: The depth offset isn't bounded by the widget's height. At depth 2 `handleTop` is `sizeUnit * 17` = 68px, but a one-row widget is 40px tall (with a 16px gap), so its grip lands inside the next row's widget. The grip is only transparent at rest (`opacity: 0`), so it still takes the click: with two full-width one-row widgets stacked in a nested group, grabbing what looks like the lower widget's body area near the right edge starts a move of the upper one. Should the step be clamped to the widget's rendered height, or the nested grips laid out some other way so they stay inside their own widget? ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,528 @@ +/** + * 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; + } + `} +`; + +/** Vertical step between a container's handles and its children's. */ +const HANDLE_STEP = 8; +/** Beyond this, nesting stops pushing handles further down the widget. */ +const MAX_HANDLE_STEPS = 3; + +const handleTop = (theme: { sizeUnit: number }, depth: number) => + theme.sizeUnit * (1 + Math.min(depth, MAX_HANDLE_STEPS) * HANDLE_STEP); + +const Grip = styled.button<{ depth: number }>` + ${({ theme, depth }) => css` + position: absolute; + top: ${handleTop(theme, depth)}px; + /* + * Clear of a widget's own controls, which sit at the top right corner and + * are 8 units wide (see WithQuery's .canvas-widget-controls): the grip is + * above them in the stack, so overlapping would swallow clicks meant for + * the menu. + */ + right: ${theme.sizeUnit * 9}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; Review Comment: `ResizeCorner` is always `right: 0; bottom: 0` with the same `z-index: 3` and, unlike `Grip`, gets no nesting offset. In an editable group with no title whose only full-width child spans the group's full height (e.g. both 4 rows), the group's corner and the child's corner sit on the same pixels, and the group's is rendered later so it wins. Dragging the child's corner then resizes the group instead of the child. Is there a way to keep a container's corner distinct from its last child's (an inset by depth like the grips get)? ########## superset-frontend/src/features/canvas/PlanModeButton.tsx: ########## @@ -0,0 +1,94 @@ +/** + * 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 { useEffect, useRef, useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { Button } from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; + +/** + * What entering a draft is called in the UI. A working name, still being + * settled; changing it here changes it everywhere it is shown. + */ +export const PLAN_MODE_LABEL = () => t('Plan mode'); Review Comment: This PR adds nine new `t()` strings ('Plan mode', 'Move widget', 'Resize widget', 'Saving your changes…', 'That change could not be saved.', 'That plan could not be started.', 'This canvas changed while you were editing, so your change was undone.', 'This overlaps another widget', 'You do not have permission to change this layout.') without regenerating `superset/translations/messages.pot`, so the `babel-extract` check (`check_pot_drift.py`) fails on this head and translators never get the strings. Can you run `./scripts/translations/babel_update.sh` and commit the regenerated template? ########## superset-frontend/src/features/canvas/CanvasGrid.test.tsx: ########## @@ -190,3 +213,81 @@ test('setting a cross-filter does nothing while cross-filters are off', () => { expect(onValueChange).not.toHaveBeenCalled(); }); + +test('a canvas that is not editable gets no drag or resize handles', () => { + renderGrid(); + + expect( + screen.queryByRole('button', { name: 'Move widget' }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'Resize widget' }), + ).not.toBeInTheDocument(); +}); + +test('an editable canvas gets a handle per placed widget', () => { + renderGrid(emptyScopeValues(), jest.fn(), result, jest.fn(), true); + + // Every node with a resolved placement, on the root grid and inside the + // nested group, is draggable and resizable. + expect(screen.getAllByRole('button', { name: 'Move widget' })).toHaveLength( + Object.keys(result.placements).length, + ); + expect(screen.getAllByRole('button', { name: 'Resize widget' })).toHaveLength( + Object.keys(result.placements).length, + ); +}); + +/** + * jsdom carries none of the properties a gesture needs on its own + * `PointerEvent`, so these are built on `MouseEvent` with the pointer id added + * -- the same shape `CanvasGridSurface.test.tsx` uses. + */ +class TestPointerEvent extends MouseEvent { + pointerId: number; + + constructor(type: string, init: MouseEventInit = {}) { + super(type, { bubbles: true, cancelable: true, ...init }); + this.pointerId = 1; + } +} + +const dragRight = (handle: HTMLElement, dx: number) => + [ + new TestPointerEvent('pointerdown', { button: 0, clientX: 0, clientY: 0 }), + new TestPointerEvent('pointermove', { clientX: dx, clientY: 0 }), + new TestPointerEvent('pointerup', { clientX: dx, clientY: 0 }), + ].forEach(event => fireEvent(handle, event)); + +beforeAll(() => { + // jsdom lays nothing out, so every grid is told it is 1000px wide. + Object.defineProperty(HTMLElement.prototype, 'clientWidth', { + configurable: true, + get: () => 1000, + }); + // jsdom has no pointer capture; the gesture only needs it not to throw. + HTMLElement.prototype.setPointerCapture = jest.fn(); + HTMLElement.prototype.releasePointerCapture = jest.fn(); +}); + +test('a nested grid resizes against its own column count, not the root', () => { Review Comment: The fixture gives `c1` `layoutConstraints: { c1: { minColSpan: 2 } }`, but the only resize here grows `c1` to the nested grid's edge, so nothing checks that `CanvasGrid` forwards `layoutConstraints` to the nested surface. If that prop were replaced with `{}`, this file stays green. Could a case shrink `c1` past its minimum at this boundary (drag the resize corner left more than one column) and assert `place` is called with `colSpan: 2` rather than `1`? ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,528 @@ +/** + * 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; + } + `} +`; + +/** Vertical step between a container's handles and its children's. */ +const HANDLE_STEP = 8; +/** Beyond this, nesting stops pushing handles further down the widget. */ +const MAX_HANDLE_STEPS = 3; + +const handleTop = (theme: { sizeUnit: number }, depth: number) => + theme.sizeUnit * (1 + Math.min(depth, MAX_HANDLE_STEPS) * HANDLE_STEP); + +const Grip = styled.button<{ depth: number }>` + ${({ theme, depth }) => css` + position: absolute; + top: ${handleTop(theme, depth)}px; + /* + * Clear of a widget's own controls, which sit at the top right corner and + * are 8 units wide (see WithQuery's .canvas-widget-controls): the grip is + * above them in the stack, so overlapping would swallow clicks meant for + * the menu. + */ + right: ${theme.sizeUnit * 9}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; + /** + * How deeply this grid is nested. A container's box begins where its first + * child's does and its handles paint last, so without an offset per level a + * container's grip would sit over its child's and swallow the click. + */ + depth: number; + onPlace: (nodeId: string, placement: GridPlacement) => void; + renderNode: (nodeId: string) => ReactNode; +} + +export default function CanvasGridSurface({ + childIds, + columns, + gap, + rowUnit, + placements, + constraints, + editable, + depth, + 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], + ); + + /** + * Persists `next` for `nodeId`, where `from` is the placement the gesture + * resolved against. + * + * Both no-op checks matter. A gesture that moved nothing resolves back to + * `from`, and sending that would write the geometry the widget had when it + * was grabbed over anything that landed while the button was held -- a plain + * click on a handle would revert a draft poll or another editor's resize. + * Comparing with the current placement then drops a gesture that happens to + * land exactly where the widget already sits. + */ + const commit = useCallback( + (nodeId: string, next: GridPlacement | undefined, from: GridPlacement) => { + const placement = placements[nodeId]; + if (!next || !placement) return; + if (samePlacement(next, from) || samePlacement(next, placement)) return; + onPlace(nodeId, next); + }, + [onPlace, placements], + ); + + const begin = + (nodeId: string, kind: GestureKind) => + (event: ReactPointerEvent<HTMLButtonElement>) => { + if (event.button !== 0) return; + // One gesture at a time. A second finger on another handle also reports + // button 0, and letting it replace the live gesture would strand the + // first pointer: its move and up events fail the pointerId guards, so + // its drag would be dropped without ever being committed. + if (live.current) return; + const origin = placements[nodeId]; + if (!origin) return; + event.preventDefault(); + event.stopPropagation(); + event.currentTarget.setPointerCapture(event.pointerId); + update({ + nodeId, + kind, + pointerId: event.pointerId, + startX: event.clientX, + startY: event.clientY, + dx: 0, + dy: 0, + origin, + }); + }; + + const onPointerMove = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update({ + ...current, + dx: event.clientX - current.startX, + dy: event.clientY - current.startY, + }); + }; + + const onPointerUp = (event: ReactPointerEvent<HTMLButtonElement>) => { + const { current } = live; + if (!current || current.pointerId !== event.pointerId) return; + update(undefined); + // From the release event's own position: the pointer can travel between + // the last move event and the release, and the drop belongs where the + // user let go. + commit( + current.nodeId, + targetOf( + current.nodeId, + current.kind, + current.origin, + event.clientX - current.startX, + event.clientY - current.startY, Review Comment: The delta is computed in viewport coordinates (`clientX/clientY - startX/startY`) and nothing accounts for the scroll container moving during the gesture. Grab a widget near the top of a canvas taller than the viewport, scroll down about two row pitches (about 112px) with the wheel while holding, and release at the same screen position: the pointer is now two rows further down the content, but the delta is 0, so the widget stays put and `commit` writes nothing. There's no edge auto-scroll either, so moving a widget further than one screen needs this. Should the drag add the scroll offset of the grid's scroll ancestors since pointer-down (or track scroll events while a gesture is live)? ########## tests/unit_tests/canvas/api_test.py: ########## @@ -154,6 +192,42 @@ def test_get_definition(client: Any, full_api_access: None, canvas: Any) -> None assert response.json["result"]["placements"] == {} assert response.json["result"]["widgetTypes"] == {} assert response.json["result"]["gridColumns"] == {} + assert response.json["result"]["layoutConstraints"] == {} + assert response.json["result"]["canEdit"] is True + + +def test_definition_reports_a_view_only_user_cannot_edit( + client: Any, full_api_access: None, canvas: Any, mocker: MockerFixture +) -> None: + """ + ``canEdit`` answers the same question the write path enforces, so a user + who may only read is told not to offer drag and resize. + """ + mocker.patch( + "superset.canvas.api.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(MagicMock()), + ) + + response = client.get(URL) + + assert response.status_code == 200 + assert response.json["result"]["canEdit"] is False + + +def test_definition_reports_no_edit_without_route_write_permission( + client: Any, full_api_access: None, canvas: Any, mocker: MockerFixture +) -> None: + """ + The write route needs ``can_write`` on Canvas as well as editorship, so an + object editor whose role lacks the route permission is told not to offer + editing -- otherwise its saves would 403. + """ + mocker.patch("superset.canvas.api.security_manager.can_access", return_value=False) Review Comment: Patching `security_manager.can_access` to return `False` for every call means this test passes whether production checks `can_write` or `can_read` on `Canvas`, so a regression that swaps the permission would go unnoticed. Could the mock return `True` for `can_read` and `False` for `can_write`, and assert the GET still returns 200 with `canEdit: false`? The draft-payload test in `draft_test.py` around line 250 has the same all-False mock. -- 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]
