Fix selection issues (#6128)

**Problem:**

1. In a selected scene, dragging an element should drag the scene not
the element
2. In a selected scene, clicking on an element should select the element
3. In a selected scene, dragging an element under the threshold not move
anything, but select the element

4. In a selected scene, cmd-dragging an element should drag the element
not the scene
5. In a selected scene, cmd-clicking an element should select the
element
6. In a selected scene, cmd-dragging an element under the drag threshold
should not move anything, but select the element

**Fix:**
To fix 1-2-3 we need to make sure that dragging below the threshold is
handled as a "click", so the selection happens on mouse up.

To fix 4-5-6 we need to make sure the cmd-mousedown already selects the
element, which guarantees that the following dragging operates on the
element (and not the scene)

Meanwhile, the failing tests uncovered that we have an extra bug: you
can deselect a selected item if you click on a non-visible (e.g.
cropped) part of it. To fix it I modified the behavior of
`findValidTarget` in `dont-prefer-selected` mode: it means we should
change our selection to something different if possible, but we should
still not deselect it. To make this clearer I renamed
`don't-prefer-selected` to `prefer-more-specific-selection`

As I fixed the tests I had to realize that our dragging helper functions
are not good enough: we can specifiy modifier keys, but we can not
specify separate modifier keys for the mouse down and for the mouse move
events. Which is necessary because e.g. cmd changes the behavior
differently in the two cases: cmd on mouse down changes how selection
work, so it changes what will be dragged, while cmd on mouse move
changes which strategy will be applied (cmd allows reparenting).

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #6012

editor/src/components/canvas/canvas-strategies/strategies/absolute-flex-reparent-strategy-helpers.spec.browser2.tsx

@@ -37,6 +37,7 @@ async function dragElement(
   modifiers: Modifiers,
   includeMouseUp: boolean,
   midDragCallback?: () => Promise<void>,
+  mouseDownModifiers?: Modifiers,
 ): Promise<void> {
   const targetElement = renderResult.renderedDOM.getByTestId(targetTestId)
   const targetElementBounds = targetElement.getBoundingClientRect()
@@ -51,10 +52,11 @@ async function dragElement(
   if (includeMouseUp) {
     await mouseDragFromPointToPoint(canvasControlsLayer, startPoint, endPoint, {
       modifiers: modifiers,
+      mouseDownModifiers: mouseDownModifiers,
       midDragCallback: midDragCallback,
     })
   } else {
-    await mouseDownAtPoint(canvasControlsLayer, startPoint, { modifiers: modifiers })
+    await mouseDownAtPoint(canvasControlsLayer, startPoint)
     await mouseMoveToPoint(canvasControlsLayer, endPoint, {
       modifiers: modifiers,
       eventOptions: { buttons: 1 },

editor/src/components/canvas/canvas-strategies/strategies/absolute-move-strategy.spec.browser2.tsx

@@ -52,6 +52,7 @@ async function dragByPixels(
   delta: WindowPoint,
   testid: string,
   modifiers: Modifiers = emptyModifiers,
+  mouseDownModifiers?: Modifiers,
 ) {
   const targetElement = editor.renderedDOM.getByTestId(testid)
   const targetElementBounds = targetElement.getBoundingClientRect()
@@ -60,6 +61,7 @@ async function dragByPixels(
 
   await mouseDragFromPointWithDelta(canvasControlsLayer, targetElementCenter, delta, {
     modifiers,
+    mouseDownModifiers,
     midDragCallback: async () => {
       NO_OP()
     },
@@ -72,8 +74,9 @@ async function dragElement(
   dragDelta: WindowPoint,
   modifiers: Modifiers,
   midDragCallback?: () => Promise<void>,
+  mouseDownModifiers?: Modifiers,
 ): Promise<void> {
-  await mouseDownAtPoint(canvasControlsLayer, startPoint, { modifiers: cmdModifier })
+  await mouseDownAtPoint(canvasControlsLayer, startPoint, { modifiers: mouseDownModifiers })
   await mouseDragFromPointWithDelta(canvasControlsLayer, startPoint, dragDelta, {
     modifiers,
     midDragCallback,
@@ -316,7 +319,7 @@ describe('Absolute Move Strategy', () => {
 
       await selectComponentsForTest(editor, [targetElement])
 
-      await dragByPixels(editor, windowPoint({ x: 15, y: 15 }), 'bbb', cmdModifier)
+      await dragByPixels(editor, windowPoint({ x: 15, y: 15 }), 'bbb')
 
       expect(getPrintedUiJsCode(editor.getEditorState())).toEqual(
         makeTestProjectCodeWithSnippet(
@@ -947,8 +950,6 @@ describe('Absolute Move Strategy', () => {
       EP.fromString(`${BakedInStoryboardUID}/${TestSceneUID}/${TestAppUID}:aaa/bbb/ccc`),
     ])
 
-    // await wait(10000)
-
     await dragByPixels(editor, windowPoint({ x: 15, y: 15 }), 'bbb')
 
     expect(getPrintedUiJsCode(editor.getEditorState())).toEqual(
@@ -1215,7 +1216,14 @@ describe('Absolute Move Strategy', () => {
     const startPoint = windowPoint({ x: targetElementBounds.x + 5, y: targetElementBounds.y + 5 })
     const dragDelta = windowPoint({ x: 40, y: 25 })
 
-    await dragElement(canvasControlsLayer, startPoint, dragDelta, emptyModifiers)
+    await dragElement(
+      canvasControlsLayer,
+      startPoint,
+      dragDelta,
+      cmdModifier,
+      undefined,
+      cmdModifier,
+    )
 
     await renderResult.getDispatchFollowUpActionsFinished()
     expect(getPrintedUiJsCode(renderResult.getEditorState())).toEqual(

editor/src/components/canvas/canvas-strategies/strategies/absolute-reparent-strategy.spec.browser2.tsx

@@ -72,6 +72,7 @@ async function dragElement(
   modifiers: Modifiers,
   checkCursor: CheckCursor | null,
   midDragCallback: (() => Promise<void>) | null,
+  mouseDownModifiers?: Modifiers,
 ) {
   const targetElement = renderResult.renderedDOM.getByTestId(targetTestId)
   const targetElementBounds = targetElement.getBoundingClientRect()
@@ -90,6 +91,7 @@ async function dragElement(
   await mouseClickAtPoint(canvasControlsLayer, startPoint, { modifiers: cmdModifier })
   await mouseDragFromPointWithDelta(canvasControlsLayer, startPoint, dragDelta, {
     modifiers: modifiers,
+    mouseDownModifiers: mouseDownModifiers,
     midDragCallback: combinedMidDragCallback,
   })
 }
@@ -1325,7 +1327,15 @@ export var ${BakedInStoryboardVariableName} = (props) => {
           )
 
           const dragDelta = windowPoint({ x: 50, y: 0 })
-          await dragElement(renderResult, 'child-1', dragDelta, cmdModifier, null, null)
+          await dragElement(
+            renderResult,
+            'child-1',
+            dragDelta,
+            cmdModifier,
+            null,
+            null,
+            cmdModifier,
+          )
 
           await renderResult.getDispatchFollowUpActionsFinished()
 

editor/src/components/canvas/canvas-strategies/strategies/convert-to-absolute-and-move-strategy.spec.browser2.tsx

@@ -55,6 +55,7 @@ import {
 import { selectComponentsForTest } from '../../../../utils/utils.test-utils'
 import { ConvertToAbsoluteAndMoveStrategyID } from './convert-to-absolute-and-move-strategy'
 import CanvasActions from '../../canvas-actions'
+import { ctrlModifier } from '../../../../utils/modifiers'
 
 const complexProject = () => {
   const code = `
@@ -1140,12 +1141,7 @@ describe('Convert to absolute/escape hatch', () => {
             y: elementBounds.y + 10,
           },
           {
-            modifiers: {
-              alt: false,
-              cmd: true,
-              ctrl: true,
-              shift: false,
-            },
+            modifiers: ctrlModifier,
           },
         )
 
@@ -1805,12 +1801,7 @@ describe('Escape hatch strategy on awkward project', () => {
         y: 15,
       },
       {
-        modifiers: {
-          alt: false,
-          cmd: true,
-          ctrl: true,
-          shift: false,
-        },
+        modifiers: ctrlModifier,
       },
     )
 

editor/src/components/canvas/controls/select-mode/select-mode-hooks.tsx

@@ -303,7 +303,7 @@ function getCandidateSelectableViews(
 export function useFindValidTarget(): (
   selectableViews: Array<ElementPath>,
   mousePoint: WindowPoint | null,
-  preferAlreadySelected: 'prefer-selected' | 'dont-prefer-selected',
+  preferAlreadySelected: 'prefer-selected' | 'prefer-more-specific-selection',
 ) => {
   elementPath: ElementPath
   isSelected: boolean
@@ -325,7 +325,7 @@ export function useFindValidTarget(): (
     (
       selectableViews: Array<ElementPath>,
       mousePoint: WindowPoint | null,
-      preferAlreadySelected: 'prefer-selected' | 'dont-prefer-selected',
+      preferAlreadySelected: 'prefer-selected' | 'prefer-more-specific-selection',
     ) => {
       const {
         selectedViews,
@@ -336,26 +336,43 @@ export function useFindValidTarget(): (
         elementPathTree,
         allElementProps,
       } = storeRef.current
-      const validElementMouseOver: ElementPath | null =
-        preferAlreadySelected === 'prefer-selected'
-          ? getSelectionOrValidTargetAtPoint(
-              componentMetadata,
-              selectedViews,
-              hiddenInstances,
-              selectableViews,
-              mousePoint,
-              canvasScale,
-              canvasOffset,
-              elementPathTree,
-              allElementProps,
-            )
-          : getValidTargetAtPoint(
-              selectableViews,
-              mousePoint,
-              canvasScale,
-              canvasOffset,
-              componentMetadata,
-            )
+      const validElementMouseOver: ElementPath | null = (() => {
+        if (preferAlreadySelected === 'prefer-selected') {
+          return getSelectionOrValidTargetAtPoint(
+            componentMetadata,
+            selectedViews,
+            hiddenInstances,
+            selectableViews,
+            mousePoint,
+            canvasScale,
+            canvasOffset,
+            elementPathTree,
+            allElementProps,
+          )
+        }
+        const newSelection = getValidTargetAtPoint(
+          selectableViews,
+          mousePoint,
+          canvasScale,
+          canvasOffset,
+          componentMetadata,
+        )
+        if (newSelection != null) {
+          return newSelection
+        }
+        return getSelectionOrValidTargetAtPoint(
+          componentMetadata,
+          selectedViews,
+          hiddenInstances,
+          selectableViews,
+          mousePoint,
+          canvasScale,
+          canvasOffset,
+          elementPathTree,
+          allElementProps,
+        )
+      })()
+
       const validElementPath: ElementPath | null =
         validElementMouseOver != null ? validElementMouseOver : null
       if (validElementPath != null) {
@@ -452,7 +469,11 @@ export function useCalculateHighlightedViews(
   return React.useCallback(
     (targetPoint: WindowPoint, eventCmdPressed: boolean) => {
       const selectableViews: Array<ElementPath> = getHighlightableViews(eventCmdPressed, false)
-      const validElementPath = findValidTarget(selectableViews, targetPoint, 'dont-prefer-selected')
+      const validElementPath = findValidTarget(
+        selectableViews,
+        targetPoint,
+        'prefer-more-specific-selection',
+      )
       const validElementPathForHover = findValidTarget(
         selectableViews,
         targetPoint,
@@ -528,14 +549,15 @@ export function useHighlightCallbacks(
 function getPreferredSelectionForEvent(
   eventType: 'mousedown' | 'mouseup' | string,
   isDoubleClick: boolean,
-): 'prefer-selected' | 'dont-prefer-selected' {
+  cmdModifier: boolean,
+): 'prefer-selected' | 'prefer-more-specific-selection' {
   // mousedown keeps selection on a single click to allow dragging overlapping elements and selection happens on mouseup
   // with continuous clicking mousedown should select
   switch (eventType) {
     case 'mousedown':
-      return isDoubleClick ? 'dont-prefer-selected' : 'prefer-selected'
+      return isDoubleClick || cmdModifier ? 'prefer-more-specific-selection' : 'prefer-selected'
     case 'mouseup':
-      return isDoubleClick ? 'prefer-selected' : 'dont-prefer-selected'
+      return isDoubleClick ? 'prefer-selected' : 'prefer-more-specific-selection'
     default:
       return 'prefer-selected'
   }
@@ -561,6 +583,7 @@ function useSelectOrLiveModeSelectAndHover(
   )
   const windowToCanvasCoordinates = useWindowToCanvasCoordinates()
   const interactionSessionHappened = React.useRef(false)
+  const draggedOverThreshold = React.useRef(false)
   const didWeHandleMouseDown = React.useRef(false) //  this is here to avoid selecting when closing text editing
 
   const { onMouseMove: innerOnMouseMove } = useHighlightCallbacks(
@@ -582,6 +605,14 @@ function useSelectOrLiveModeSelectAndHover(
           editorStoreRef.current.editor.canvas.interactionSession,
           editorStoreRef.current.editor.keysPressed['space'],
         ) || event.buttons === 4
+
+      const draggingOverThreshold =
+        editorStoreRef.current.editor.canvas.interactionSession?.interactionData?.type === 'DRAG'
+          ? editorStoreRef.current.editor.canvas.interactionSession?.interactionData?.drag != null
+          : false
+
+      draggedOverThreshold.current = draggedOverThreshold.current || draggingOverThreshold
+
       if (isDragIntention) {
         return
       }
@@ -601,20 +632,23 @@ function useSelectOrLiveModeSelectAndHover(
     (event: React.MouseEvent<HTMLDivElement>) => {
       const isLeftClick = event.button === 0
       const isRightClick = event.type === 'contextmenu' && event.detail === 0
-      const isDragIntention =
+      const isCanvasPanIntention =
         editorStoreRef.current.editor.keysPressed['space'] || event.button === 1
-      const hasInteractionSessionWithMouseMoved =
+
+      const draggingOverThreshold =
         editorStoreRef.current.editor.canvas.interactionSession?.interactionData?.type === 'DRAG'
           ? editorStoreRef.current.editor.canvas.interactionSession?.interactionData?.drag != null
           : false
+
       const hasInteractionSession = editorStoreRef.current.editor.canvas.interactionSession != null
       const hadInteractionSessionThatWasCancelled =
         interactionSessionHappened.current && !hasInteractionSession
 
       const activeControl = editorStoreRef.current.editor.canvas.interactionSession?.activeControl
+
       const mouseUpSelectionAllowed =
         didWeHandleMouseDown.current &&
-        !hadInteractionSessionThatWasCancelled &&
+        (!hadInteractionSessionThatWasCancelled || !draggedOverThreshold.current) &&
         (activeControl == null || activeControl.type === 'BOUNDING_AREA')
 
       if (event.type === 'mousedown') {
@@ -625,6 +659,7 @@ function useSelectOrLiveModeSelectAndHover(
         interactionSessionHappened.current = false
         // didWeHandleMouseDown is used to avoid selecting when closing text editing
         didWeHandleMouseDown.current = false
+        draggedOverThreshold.current = false
 
         if (!mouseUpSelectionAllowed) {
           // We should skip this mouseup
@@ -633,8 +668,8 @@ function useSelectOrLiveModeSelectAndHover(
       }
 
       if (
-        isDragIntention ||
-        hasInteractionSessionWithMouseMoved ||
+        isCanvasPanIntention ||
+        draggingOverThreshold ||
         !active ||
         !(isLeftClick || isRightClick)
       ) {
@@ -643,8 +678,13 @@ function useSelectOrLiveModeSelectAndHover(
       }
 
       const doubleClick = event.type === 'mousedown' && event.detail > 0 && event.detail % 2 === 0
+      const cmdMouseDown = event.type === 'mousedown' && event.metaKey
       const selectableViews = getSelectableViewsForSelectMode(event.metaKey, doubleClick)
-      const preferAlreadySelected = getPreferredSelectionForEvent(event.type, doubleClick)
+      const preferAlreadySelected = getPreferredSelectionForEvent(
+        event.type,
+        doubleClick,
+        event.metaKey,
+      )
       const foundTarget = findValidTarget(
         selectableViews,
         windowPoint(point(event.clientX, event.clientY)),
@@ -658,7 +698,7 @@ function useSelectOrLiveModeSelectAndHover(
       if (foundTarget != null || isDeselect) {
         if (
           event.button !== 2 &&
-          event.type !== 'mouseup' &&
+          (event.type !== 'mouseup' || cmdMouseDown) &&
           foundTarget != null &&
           draggingAllowed &&
           // grid has its own drag handling