diff --git a/docs/drag-and-drop.md b/docs/drag-and-drop.md index e2cc31eb0a3..b96d880e0ac 100644 --- a/docs/drag-and-drop.md +++ b/docs/drag-and-drop.md @@ -68,6 +68,8 @@ Three companion fields ride alongside it: These are what `DropHover` and other UI components subscribe to. +Each drop target element (the thought rows, `DropChild`, `DropEnd` and `DropUncle`) renders `data-drop-over`: react-dnd's `isOver({ shallow: true })` for that target, which is true even on a target that rejects the drop. It names the target a drop lands on, and it is set in the same commit as the target's `isHovering`, from which `DropChild`, `DropEnd` and `DropUncle` draw their drop hover. A thought row's drop hover also needs `state.hoveringPath`, which the same hover writes first, but only when the pointer has moved (`throttleByMousePosition`). react-dnd itself moves its hover only on a `dragover`, and a drag driven by Puppeteer dispatches `dragover` only when the pointer moves. So when a hover changes the layout under a stationary pointer, e.g. by expanding a thought, the drop target that ends up under the pointer is not hovered until the pointer moves again. The Puppeteer helper [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts) therefore records `data-drop-over` on the drop target under the pointer as it is rendered, and waits for that record before it holds or drops. + ### Drag sources There are four drag sources in the app, each implemented as a hook: @@ -94,7 +96,7 @@ The drop targets in the UI are: - [`DropChild`](../src/components/DropChild.tsx) — a `SubthoughtsDrop` zone rendered inside a thought when it's empty or collapsed. - [`DropEnd`](../src/components/DropEnd.tsx) — a `SubthoughtsDrop` zone at the end of a subthought list. - [`DropCliff`](../src/components/DropCliff.tsx) — renders one `DropEnd` per cliff level when depth decreases by more than 1, so a thought can be dropped at any of the intermediate levels exposed by the cliff. -- [`DropUncle`](../src/components/DropUncle.tsx) — a `ThoughtDrop` zone for inserting before the next hidden uncle, when the immediate parent is hidden by autofocus. +- [`DropUncle`](../src/components/DropUncle.tsx) — a `SubthoughtsDrop` zone for inserting before the next hidden uncle, when the immediate parent is hidden by autofocus. - [`Favorites`](../src/components/Favorites.tsx) — its own zone, handled by `useDragDropFavorites`. - [`DropGutter`](../src/components/DropGutter.tsx) — the right-edge invisible quick-delete panel; see [Quick drops](#quick-drops). diff --git a/docs/testing.md b/docs/testing.md index d5ed1ab5855..497ebcde7fd 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -640,6 +640,7 @@ Integration tests are blackbox, but named helpers may take shortcuts during arra | Text paint assertion | Assert | [`getThoughtPixels`](../src/e2e/puppeteer/helpers/getThoughtPixels.ts) | Read pixels within a thought's text bounds through the shared stabilized screenshot helper and an offscreen canvas. Does not change the thought or app state. | | Structural assertion | Assert | [`exportThoughts`](../src/e2e/puppeteer/helpers/exportThoughts.ts) | Export the thought tree as plaintext. Do not make additional assertions on Redux state. | | Non-visual synchronization | Wait | [`waitForThoughtspaceIdle`](../src/e2e/puppeteer/helpers/waitForThoughtspaceIdle.ts) | Use only when persistence or another prerequisite has no immediate visual signal. `waitForThoughtspaceIdle` waits for every queued persistence write to commit and rejects if one failed; call it only to synchronize before a reload or before asserting that a write landed. `refresh` waits for persisted state to load after a reload, but shared setup and `resetApp` deliberately do not wait for database initialization. Assert the final user-visible result separately. | +| Drag-and-drop synchronization | Wait | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts) | Waits until react-dnd has hovered the drop target under the pointer, read from its `data-drop-over` attribute ([drag-and-drop.md](drag-and-drop.md#state-machine-statelongpress)). For the length of the move it keeps a `MutationObserver` on `window.__dropTargetHoverWatch`, since a Puppeteer drag dispatches `dragover` only on pointer moves, and a hover can change the layout under the stationary pointer, e.g. by hover expansion, before a poll would see it. Tests must not wait on drop hovers themselves. | | Flake diagnosis | Arrange | [`withDebugLog`](../src/e2e/puppeteer/helpers/withDebugLog.ts) | Wrap only a test under investigation for an intermittent failure. It records the [Debug Log](debug-log.md) and appends it to the error on failure, where the Puppeteer Flaky workflow's failure report carries it. Logging adds a little synchronous work to every action and input event, so it can make a race rarer; remove the wrapper once the flake is fixed. | DOM reads are different from backdoors: inline `page.evaluate`/`browser.execute` may read user-visible DOM when no helper exists, though a repeated read should become a named helper. It may not dispatch actions, mutate app state, set test flags, or write to the DOM. @@ -706,7 +707,7 @@ Puppeteer input is coordinated through the helpers in [`../src/e2e/puppeteer/hel | Press a key or shortcut | [`press`](../src/e2e/puppeteer/helpers/press.ts) | Presses a key with optional `alt`, `ctrl`, `meta`, and `shift` modifiers. | | Swipe/command gesture | [`gesture`](../src/e2e/puppeteer/helpers/gesture.ts) | Emits `touchStart`, stepped `touchMove` events, and `touchEnd` for the supplied direction path or command gesture. | | Long press | [`longPressThought`](../src/e2e/puppeteer/helpers/longPressThought.ts), [`longPressBullet`](../src/e2e/puppeteer/helpers/longPressBullet.ts), [`longPressWord`](../src/e2e/puppeteer/helpers/longPressWord.ts) | Holds a touch on the thought, or the mouse on its bullet, until the bullet reports the long-press highlight, then releases and waits for the body's `data-drag-hold` attribute to return to `false`, since the app ends the long press, and commits the selection, on a timer after the release. Long press is bound to the bullet on every platform but to the thought itself only on touch, so `longPressBullet` is how a thought is long pressed on desktop, without emulating a device. `longPressWord` holds a touch on a word until the highlight, adds Chrome for Android's native long press response (it selects the word, then dispatches a touch `contextmenu`), and leaves the finger down for the test to move or lift. | -| Drag and drop | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts), [`dragAndDropFavorite`](../src/e2e/puppeteer/helpers/dragAndDropFavorite.ts), [`dragAndDrop`](../src/e2e/puppeteer/helpers/dragAndDrop.ts) | Drives real mouse down/move/up input and waits for drag-specific visible conditions. | +| Drag and drop | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts), [`dragAndDropFavorite`](../src/e2e/puppeteer/helpers/dragAndDropFavorite.ts), [`dragAndDrop`](../src/e2e/puppeteer/helpers/dragAndDrop.ts) | Drives real mouse down/move/up input and waits for drag-specific visible conditions. `dragAndDropThought` waits until react-dnd has hovered the drop target under the pointer before it holds or drops, so a test never waits on a drop hover itself. | | Scroll | [`scroll`](../src/e2e/puppeteer/helpers/scroll.ts), [`scrollBy`](../src/e2e/puppeteer/helpers/scrollBy.ts), [`scrollIntoView`](../src/e2e/puppeteer/helpers/scrollIntoView.ts), [`scrollTo`](../src/e2e/puppeteer/helpers/scrollTo.ts) | Scrolls the window or a named container; use the narrowest helper that expresses the intent. | | Emulate a mobile device | [`deviceEmulation.useForSuite`](../src/e2e/puppeteer/helpers/deviceEmulation.ts) | Selects a Puppeteer device profile at suite scope, which `setup` applies before navigation. There is no mid-session equivalent; see the emulation note above. | diff --git a/src/components/DropChild.tsx b/src/components/DropChild.tsx index 0c5cc265c44..20b7b96d9fa 100644 --- a/src/components/DropChild.tsx +++ b/src/components/DropChild.tsx @@ -33,7 +33,7 @@ const DropChild = ({ depth, path, simplePath, cliff, isLastVisible }: DropChildP const dropHoverColorValue = useSelector(state => dropHoverColor(state, depth || 0)) const fontSize = useSelector(state => state.fontSize) - const { isHovering, dropTarget } = useDragAndDropSubThought({ path, simplePath }) + const { isHovering, isOver, dropTarget } = useDragAndDropSubThought({ path, simplePath }) // Calculate the height for the child thought over cliff // If the thought is at a cliff (cliff < 0) or is the last visible thought, calculate the drop target height @@ -62,6 +62,7 @@ const DropChild = ({ depth, path, simplePath, cliff, isLastVisible }: DropChildP }), )} ref={dndRef(dropTarget)} + data-drop-over={isOver} style={{ height: `${0.5 + dropTargetHeight}em`, // Adjust position to compensate for cliff padding on parent thought diff --git a/src/components/DropEnd.tsx b/src/components/DropEnd.tsx index 00102be18b0..decbe08b8e5 100644 --- a/src/components/DropEnd.tsx +++ b/src/components/DropEnd.tsx @@ -58,7 +58,7 @@ const DropEnd = ({ attributeEquals(state, head(rootedParentOf(state, path)), '=view', 'Table'), ) - const { isHovering, dropTarget } = useDragAndDropSubThought({ path }) + const { isHovering, isOver, dropTarget } = useDragAndDropSubThought({ path }) // a boolean indicating if the drop-hover component is shown // true if hovering and the context is not sorted @@ -128,6 +128,7 @@ const DropEnd = ({ transform: `translateX(${DROPEND_FINGERSHIFT}em)`, }} ref={dndRef(dropTarget)} + data-drop-over={isOver} > {testFlags.simulateDrop && ( {testFlags.simulateDrop && ( dragPreview(dropTarget(node)))} aria-label='child' + data-drop-over={isOver} data-divider={isDivider(value)} data-editing={isEditing} onClick={isTouch ? undefined : handleMultiselect} diff --git a/src/e2e/puppeteer/__tests__/drag-and-drop.ts b/src/e2e/puppeteer/__tests__/drag-and-drop.ts index ee824aa10d1..f390eb55531 100644 --- a/src/e2e/puppeteer/__tests__/drag-and-drop.ts +++ b/src/e2e/puppeteer/__tests__/drag-and-drop.ts @@ -21,7 +21,6 @@ import setSelection from '../helpers/setSelection' import simulateDragAndDrop from '../helpers/simulateDragAndDrop' import waitForAlert from '../helpers/waitForAlert' import waitForEditable from '../helpers/waitForEditable' -import waitUntil from '../helpers/waitUntil' import withDebugLog from '../helpers/withDebugLog' import { page } from '../session' @@ -134,10 +133,6 @@ describe('drag', () => { // hover the drop target directly below aaa, i.e. aaa's own position await dragAndDropThought('aaa', 'aaa', { hold: true, position: 'after' }) - // the drop hover is still shown at the thought's own position; only the drop is cancelled - // (.drop-hover is the class that dropHoverRecipe gives every drop hover bar) - await waitUntil(() => !!document.querySelector('.drop-hover'), { timeout: 6000 }) - // release on the same drop target await dragAndDropThought('aaa', 'aaa', { position: 'after', skipMouseDown: true }) diff --git a/src/e2e/puppeteer/helpers/dragAndDropThought.ts b/src/e2e/puppeteer/helpers/dragAndDropThought.ts index fee8118f47b..77ec48d3bfd 100644 --- a/src/e2e/puppeteer/helpers/dragAndDropThought.ts +++ b/src/e2e/puppeteer/helpers/dragAndDropThought.ts @@ -5,7 +5,77 @@ import hide from './hide' import showMousePointer from './showMousePointer' import waitUntil from './waitUntil' -/** Performs Drag and Drop functionality on a thought in Puppeteer browser. */ +/** The page-side record of whether react-dnd has hovered the drop target under the drop point. */ +interface DropTargetHoverWatch { + /** Returns true if react-dnd hovers the drop target under the drop point now. */ + check: () => boolean + observer: MutationObserver + hovered: boolean +} + +declare global { + interface Window { + /** Set by dragAndDropThought for the length of a move. */ + __dropTargetHoverWatch?: DropTargetHoverWatch + } +} + +/** + * Starts recording whether react-dnd hovers the drop target under the given point: the innermost element with + * data-drop-over, which every drop target element renders from react-dnd's isOver({ shallow: true }). That is the + * target a drop there lands on. It is set in the same commit as the target's isHovering, from which DropChild, DropEnd + * and DropUncle draw their drop hover. A thought row's drop hover also needs state.hoveringPath, which the same hover + * writes before it whenever the pointer has moved. It is true even on a target that rejects the drop, so this also + * covers a drop that shows no drop hover. + * + * The hover is recorded as it is rendered rather than polled for. A drag driven by Puppeteer dispatches dragover only + * when the pointer moves, so when a hover changes the layout under the stationary pointer, e.g. by expanding a + * thought, nothing hovers the drop target that is under it afterwards, and a poll would wait for it forever. + */ +const watchDropTargetHover = (point: { x: number; y: number }) => + page.evaluate(({ x, y }) => { + window.__dropTargetHoverWatch?.observer.disconnect() + + /** Returns true if react-dnd hovers the drop target under the point. */ + const check = () => + document.elementFromPoint(x, y)?.closest('[data-drop-over]')?.getAttribute('data-drop-over') === 'true' + + const watch: DropTargetHoverWatch = { + check, + observer: new MutationObserver(() => { + watch.hovered = watch.hovered || check() + }), + hovered: false, + } + watch.observer.observe(document.body, { attributes: true, attributeFilter: ['data-drop-over'], subtree: true }) + window.__dropTargetHoverWatch = watch + }, point) + +/** Waits until react-dnd has hovered the drop target under the point since watchDropTargetHover, or hovers it now, which covers a target that was already hovered before the move. Throws if it never does. */ +const waitForDropTargetHover = async (point: { x: number; y: number }) => { + try { + await page.waitForFunction( + () => !!window.__dropTargetHoverWatch?.hovered || !!window.__dropTargetHoverWatch?.check(), + { timeout: 5000 }, + ) + } catch (e) { + const targetFound = await page.evaluate( + ({ x, y }) => !!document.elementFromPoint(x, y)?.closest('[data-drop-over]'), + point, + ) + throw new Error( + targetFound + ? 'The drop target under the pointer was never hovered.' + : `There is no drop target under the pointer at (${point.x}, ${point.y}).`, + { cause: e }, + ) + } +} + +/** Stops recording the hover. */ +const unwatchDropTargetHover = () => page.evaluate(() => window.__dropTargetHoverWatch?.observer.disconnect()) + +/** Performs Drag and Drop functionality on a thought in Puppeteer browser. Waits for the drop target under the pointer to render its hover before holding or dropping, so callers do not need to synchronize with drag-and-drop themselves. */ const dragAndDropThought = async ( sourceValue: string, /** The value of the thought to drop at. Use the position absolute to control whether to drop before, after, etc. If null, only initiates the drag and move the mouse. */ @@ -92,12 +162,22 @@ const dragAndDropThought = async ( y: dropUncle ? dragEnd.y + dragEnd.height : dragEnd.y + dragEnd.height / 2 + yOffset, } - // A single mousemove is not registered by react-dnd when continuing an existing drag (skipMouseDown), so issue a second move one pixel away first. - // Both events land on the drop target, so no other drop targets are hovered in between. - if (skipMouseDown) { - await page.mouse.move(dropPosition.x, dropPosition.y - 1) + await watchDropTargetHover(dropPosition) + try { + // A single mousemove is not registered by react-dnd when continuing an existing drag (skipMouseDown), so issue a second move one pixel away first. + // Both events land on the drop target, so no other drop targets are hovered in between. + if (skipMouseDown) { + await page.mouse.move(dropPosition.x, dropPosition.y - 1) + } + await page.mouse.move(dropPosition.x, dropPosition.y) + + // The drop targets other than the thought rows (DropChild, DropEnd, DropUncle) mount only once the drag is in + // progress, so the drop target under the pointer is only known after it. + await page.locator('[data-drag-in-progress="true"]').wait() + await waitForDropTargetHover(dropPosition) + } finally { + await unwatchDropTargetHover() } - await page.mouse.move(dropPosition.x, dropPosition.y) } else { // Trigger drag state when no explicit drop target is provided. // Without this move, drag-and-drop alert visibility is flaky in CI. diff --git a/src/hooks/useDragAndDropSubThought.tsx b/src/hooks/useDragAndDropSubThought.tsx index 34e80fe5ceb..726af3e4432 100644 --- a/src/hooks/useDragAndDropSubThought.tsx +++ b/src/hooks/useDragAndDropSubThought.tsx @@ -282,8 +282,8 @@ const drop = (props: DroppableSubthoughts, monitor: DropTargetMonitor) => { /** Creates the props for drop. */ const dropCollect = (monitor: DropTargetMonitor) => ({ isHovering: monitor.isOver({ shallow: true }) && monitor.canDrop(), - // is being hovered over current thought irrespective of whether the given item is droppable - isBeingHoveredOver: monitor.isOver({ shallow: true }), + // hovered irrespective of whether the dragged thought can be dropped here + isOver: monitor.isOver({ shallow: true }), isDeepHovering: monitor.isOver(), canDropThought: monitor.canDrop(), }) @@ -292,7 +292,7 @@ const dropCollect = (monitor: DropTargetMonitor) => ({ const useDragAndDropSubThought = (props: DroppableSubthoughts) => { const dispatch = useDispatch() - const [{ isHovering, isDeepHovering, canDropThought }, dropTarget] = useDrop({ + const [{ isHovering, isOver, isDeepHovering, canDropThought }, dropTarget] = useDrop({ accept: [DragAndDropType.Thought, NativeTypes.FILE], canDrop: (item, monitor) => !dropRejection(props, monitor), drop: (item, monitor) => drop(props, monitor), @@ -338,7 +338,7 @@ const useDragAndDropSubThought = (props: DroppableSubthoughts) => { const isHoveringPinned = usePinDropHover(isHovering) - return { isHovering: isHoveringPinned, isDeepHovering, dropTarget } + return { isHovering: isHoveringPinned, isOver, isDeepHovering, dropTarget } } export default useDragAndDropSubThought diff --git a/src/hooks/useDragAndDropThought.tsx b/src/hooks/useDragAndDropThought.tsx index b03bb85f994..2f5285c24b2 100644 --- a/src/hooks/useDragAndDropThought.tsx +++ b/src/hooks/useDragAndDropThought.tsx @@ -355,6 +355,8 @@ const dragCollect = (monitor: DragSourceMonitor) => ({ /** Collects props from the DropTarget. */ const dropCollect = (monitor: DropTargetMonitor) => ({ isHovering: monitor.isOver({ shallow: true }) && monitor.canDrop(), + // hovered irrespective of whether the dragged thought can be dropped here + isOver: monitor.isOver({ shallow: true }), isDeepHovering: monitor.isOver(), canDropThought: monitor.canDrop(), }) @@ -380,7 +382,7 @@ const useDragAndDropThought = (props: Partial & { hoverZo collect: dragCollect, }) - const [{ isHovering, isDeepHovering, canDropThought }, dropTarget] = useDrop({ + const [{ isHovering, isOver, isDeepHovering, canDropThought }, dropTarget] = useDrop({ accept: [DragAndDropType.Thought, NativeTypes.FILE], canDrop: (item, monitor) => !dropRejection(propsTypes, monitor), drop: (item, monitor) => drop(propsTypes, monitor), @@ -439,6 +441,7 @@ const useDragAndDropThought = (props: Partial & { hoverZo dragSourceEditable, dragPreview, isHovering: isHoveringPinned, + isOver, isDeepHovering, canDropThought, dropTarget,