Skip to content

Commit c69ba4e

Browse files
authored
Fix area select regression (#9197)
Fixes #9182 Now we are careful when to handle events - before or after updating intersected set. Added general E2E and detailed unit tests for selecting. Also discovered, that sometimes the test are running while nodes are not yet in right positions. Added an instruction ensuring the position is right.
1 parent d232b92 commit c69ba4e

File tree

6 files changed

+222
-7
lines changed

6 files changed

+222
-7
lines changed

app/gui2/e2e/actions.ts

+5
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ export async function goToGraph(page: Page) {
1313
await expect(page.locator('.App')).toBeVisible()
1414
// Wait until nodes are loaded.
1515
await customExpect.toExist(locate.graphNode(page))
16+
// Wait for position initialization
17+
await expect(locate.graphNode(page).first()).toHaveCSS(
18+
'transform',
19+
'matrix(1, 0, 0, 1, -16, -16)',
20+
)
1621
}
1722

1823
// =================

app/gui2/e2e/customExpect.ts

+6
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,9 @@ export function toExist(locator: Locator) {
1111
export function toBeSelected(locator: Locator) {
1212
return expect(locator).toHaveClass(/(?<=^| )selected(?=$| )/)
1313
}
14+
15+
export module not {
16+
export function toBeSelected(locator: Locator) {
17+
return expect(locator).not.toHaveClass(/(?<=^| )selected(?=$| )/)
18+
}
19+
}

app/gui2/e2e/locate.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ export const toggleFullscreenButton = or(enterFullscreenButton, exitFullscreenBu
116116
// === Nodes ===
117117

118118
declare const nodeLocatorBrand: unique symbol
119-
type Node = Locator & { [nodeLocatorBrand]: never }
119+
export type Node = Locator & { [nodeLocatorBrand]: never }
120120

121121
export function graphNode(page: Page | Locator): Node {
122122
return page.locator('.GraphNode') as Node

app/gui2/e2e/selectingNodes.spec.ts

+58
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { expect, test } from '@playwright/test'
2+
import assert from 'assert'
3+
import { nextTick } from 'vue'
4+
import * as actions from './actions'
5+
import * as customExpect from './customExpect'
6+
import * as locate from './locate'
7+
8+
test('Selecting nodes by click', async ({ page }) => {
9+
await actions.goToGraph(page)
10+
const node1 = locate.graphNodeByBinding(page, 'five')
11+
const node2 = locate.graphNodeByBinding(page, 'ten')
12+
await customExpect.not.toBeSelected(node1)
13+
await customExpect.not.toBeSelected(node2)
14+
15+
await locate.graphNodeIcon(node1).click()
16+
await customExpect.toBeSelected(node1)
17+
await customExpect.not.toBeSelected(node2)
18+
19+
await locate.graphNodeIcon(node2).click()
20+
await customExpect.not.toBeSelected(node1)
21+
await customExpect.toBeSelected(node2)
22+
23+
await page.waitForTimeout(600) // Avoid double clicks
24+
await locate.graphNodeIcon(node1).click({ modifiers: ['Shift'] })
25+
await customExpect.toBeSelected(node1)
26+
await customExpect.toBeSelected(node2)
27+
28+
await locate.graphNodeIcon(node2).click()
29+
await customExpect.not.toBeSelected(node1)
30+
await customExpect.toBeSelected(node2)
31+
32+
await page.mouse.click(200, 200)
33+
await customExpect.not.toBeSelected(node1)
34+
await customExpect.not.toBeSelected(node2)
35+
})
36+
37+
test('Selecting nodes by area drag', async ({ page }) => {
38+
await actions.goToGraph(page)
39+
const node1 = locate.graphNodeByBinding(page, 'five')
40+
const node2 = locate.graphNodeByBinding(page, 'ten')
41+
await customExpect.not.toBeSelected(node1)
42+
await customExpect.not.toBeSelected(node2)
43+
44+
const node1BBox = await node1.locator('.selection').boundingBox()
45+
const node2BBox = await node2.boundingBox()
46+
assert(node1BBox)
47+
assert(node2BBox)
48+
await page.mouse.move(node1BBox.x - 50, node1BBox.y - 50)
49+
await page.mouse.down()
50+
await page.mouse.move(node1BBox.x - 49, node1BBox.y - 49)
51+
await expect(page.locator('.SelectionBrush')).toBeVisible()
52+
await page.mouse.move(node2BBox.x + node2BBox.width, node2BBox.y + node2BBox.height)
53+
await customExpect.toBeSelected(node1)
54+
await customExpect.toBeSelected(node2)
55+
await page.mouse.up()
56+
await customExpect.toBeSelected(node1)
57+
await customExpect.toBeSelected(node2)
58+
})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { Rect } from '@/util/data/rect'
2+
import { Vec2 } from '@/util/data/vec2'
3+
import { expect, test } from 'vitest'
4+
import { proxyRefs, ref, type Ref } from 'vue'
5+
import { useSelection } from '../selection'
6+
7+
function selectionWithMockData(sceneMousePos?: Ref<Vec2>) {
8+
const rects = new Map()
9+
rects.set(1, Rect.FromBounds(1, 1, 10, 10))
10+
rects.set(2, Rect.FromBounds(20, 1, 30, 10))
11+
rects.set(3, Rect.FromBounds(1, 20, 10, 30))
12+
rects.set(4, Rect.FromBounds(20, 20, 30, 30))
13+
const selection = useSelection(
14+
proxyRefs({ sceneMousePos: sceneMousePos ?? ref(Vec2.Zero), scale: 1 }),
15+
rects,
16+
0,
17+
)
18+
selection.setSelection(new Set([1, 2]))
19+
return selection
20+
}
21+
22+
// TODO[ao]: We should read the modifiers from bindings.ts, but I don't know how yet.
23+
24+
test.each`
25+
click | modifiers | expected
26+
${1} | ${[]} | ${[1]}
27+
${3} | ${[]} | ${[3]}
28+
${1} | ${['Shift']} | ${[2]}
29+
${3} | ${['Shift']} | ${[1, 2, 3]}
30+
${1} | ${['Mod', 'Shift']} | ${[1, 2]}
31+
${3} | ${['Mod', 'Shift']} | ${[1, 2, 3]}
32+
${1} | ${['Mod', 'Shift']} | ${[1, 2]}
33+
${3} | ${['Mod', 'Shift']} | ${[1, 2, 3]}
34+
${1} | ${['Alt', 'Shift']} | ${[2]}
35+
${3} | ${['Alt', 'Shift']} | ${[1, 2]}
36+
${1} | ${['Mod', 'Alt', 'Shift']} | ${[2]}
37+
${3} | ${['Mod', 'Alt', 'Shift']} | ${[1, 2, 3]}
38+
`('Selection by single click at $click with $modifiers', ({ click, modifiers, expected }) => {
39+
const selection = selectionWithMockData()
40+
// Position is zero, because this method should not depend on click position
41+
selection.handleSelectionOf(mockPointerEvent('click', Vec2.Zero, modifiers), new Set([click]))
42+
expect(Array.from(selection.selected)).toEqual(expected)
43+
})
44+
45+
const areas: Record<string, Rect> = {
46+
left: Rect.FromBounds(0, 0, 10, 30),
47+
right: Rect.FromBounds(20, 0, 30, 30),
48+
top: Rect.FromBounds(0, 0, 30, 10),
49+
bottom: Rect.FromBounds(0, 20, 30, 30),
50+
all: Rect.FromBounds(0, 0, 30, 30),
51+
}
52+
53+
test.each`
54+
areaId | modifiers | expected
55+
${'left'} | ${[]} | ${[1, 3]}
56+
${'right'} | ${[]} | ${[2, 4]}
57+
${'top'} | ${[]} | ${[1, 2]}
58+
${'bottom'} | ${[]} | ${[3, 4]}
59+
${'all'} | ${[]} | ${[1, 2, 3, 4]}
60+
${'left'} | ${['Shift']} | ${[1, 2, 3]}
61+
${'right'} | ${['Shift']} | ${[1, 2, 4]}
62+
${'top'} | ${['Shift']} | ${[]}
63+
${'bottom'} | ${['Shift']} | ${[1, 2, 3, 4]}
64+
${'all'} | ${['Shift']} | ${[1, 2, 3, 4]}
65+
${'left'} | ${['Mod', 'Shift']} | ${[1, 2, 3]}
66+
${'right'} | ${['Mod', 'Shift']} | ${[1, 2, 4]}
67+
${'top'} | ${['Mod', 'Shift']} | ${[1, 2]}
68+
${'bottom'} | ${['Mod', 'Shift']} | ${[1, 2, 3, 4]}
69+
${'all'} | ${['Mod', 'Shift']} | ${[1, 2, 3, 4]}
70+
${'left'} | ${['Alt', 'Shift']} | ${[2]}
71+
${'right'} | ${['Alt', 'Shift']} | ${[1]}
72+
${'top'} | ${['Alt', 'Shift']} | ${[]}
73+
${'bottom'} | ${['Alt', 'Shift']} | ${[1, 2]}
74+
${'all'} | ${['Alt', 'Shift']} | ${[]}
75+
${'left'} | ${['Mod', 'Alt', 'Shift']} | ${[2, 3]}
76+
${'right'} | ${['Mod', 'Alt', 'Shift']} | ${[1, 4]}
77+
${'top'} | ${['Mod', 'Alt', 'Shift']} | ${[]}
78+
${'bottom'} | ${['Mod', 'Alt', 'Shift']} | ${[1, 2, 3, 4]}
79+
${'all'} | ${['Mod', 'Alt', 'Shift']} | ${[3, 4]}
80+
`('Selection by dragging $area area with $modifiers', ({ areaId, modifiers, expected }) => {
81+
const area = areas[areaId]!
82+
const dragCase = (start: Vec2, stop: Vec2) => {
83+
const mousePos = ref(start)
84+
const selection = selectionWithMockData(mousePos)
85+
86+
selection.events.pointerdown(mockPointerEvent('pointerdown', mousePos.value, modifiers))
87+
selection.events.pointermove(mockPointerEvent('pointermove', mousePos.value, modifiers))
88+
mousePos.value = stop
89+
selection.events.pointermove(mockPointerEvent('pointermove', mousePos.value, modifiers))
90+
expect(selection.selected).toEqual(new Set(expected))
91+
selection.events.pointerdown(mockPointerEvent('pointerup', mousePos.value, modifiers))
92+
expect(selection.selected).toEqual(new Set(expected))
93+
}
94+
95+
// We should select same set of nodes, regardless of drag direction
96+
dragCase(new Vec2(area.left, area.top), new Vec2(area.right, area.bottom))
97+
dragCase(new Vec2(area.right, area.bottom), new Vec2(area.left, area.top))
98+
dragCase(new Vec2(area.left, area.bottom), new Vec2(area.right, area.top))
99+
dragCase(new Vec2(area.right, area.top), new Vec2(area.left, area.bottom))
100+
})
101+
102+
class MockPointerEvent extends MouseEvent {
103+
currentTarget: EventTarget | null
104+
pointerId: number
105+
constructor(type: string, options: MouseEventInit & { currentTarget?: Element | undefined }) {
106+
super(type, options)
107+
this.currentTarget = options.currentTarget ?? null
108+
this.pointerId = 4
109+
}
110+
}
111+
112+
function mockPointerEvent(type: string, pos: Vec2, modifiers: string[]): PointerEvent {
113+
const modifiersSet = new Set(modifiers)
114+
const event = new MockPointerEvent(type, {
115+
altKey: modifiersSet.has('Alt'),
116+
ctrlKey: modifiersSet.has('Mod'),
117+
shiftKey: modifiersSet.has('Shift'),
118+
metaKey: modifiersSet.has('Meta'),
119+
clientX: pos.x,
120+
clientY: pos.y,
121+
button: 0,
122+
buttons: 1,
123+
currentTarget: document.createElement('div'),
124+
}) as PointerEvent
125+
return event
126+
}

app/gui2/src/composables/selection.ts

+26-6
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ import type { PortId } from '@/providers/portInfo.ts'
77
import { type NodeId } from '@/stores/graph'
88
import type { Rect } from '@/util/data/rect'
99
import type { Vec2 } from '@/util/data/vec2'
10-
import { computed, proxyRefs, reactive, ref, shallowRef } from 'vue'
10+
import { computed, proxyRefs, reactive, ref, shallowRef, watch, type Ref } from 'vue'
1111

1212
export type SelectionComposable<T> = ReturnType<typeof useSelection<T>>
1313
export function useSelection<T>(
14-
navigator: NavigatorComposable,
14+
navigator: { sceneMousePos: Vec2 | null; scale: number },
1515
elementRects: Map<T, Rect>,
1616
margin: number,
1717
callbacks: {
@@ -126,13 +126,19 @@ export function useSelection<T>(
126126
const pointer = usePointer((_pos, event, eventType) => {
127127
if (eventType === 'start') {
128128
readInitiallySelected()
129-
} else if (pointer.dragging && anchor.value == null) {
130-
anchor.value = navigator.sceneMousePos?.copy()
129+
} else if (pointer.dragging) {
130+
if (anchor.value == null) {
131+
anchor.value = navigator.sceneMousePos?.copy()
132+
}
133+
selectionEventHandler(event)
131134
} else if (eventType === 'stop') {
135+
if (anchor.value == null) {
136+
// If there was no drag, we want to handle "clicking-off" selected nodes.
137+
selectionEventHandler(event)
138+
}
132139
anchor.value = undefined
133140
initiallySelected.clear()
134141
}
135-
selectionEventHandler(event)
136142
})
137143

138144
return proxyRefs({
@@ -154,6 +160,20 @@ export function useSelection<T>(
154160

155161
function countCommonInSets(a: Set<unknown>, b: Set<unknown>): number {
156162
let count = 0
157-
for (const item in a) count += +b.has(item)
163+
for (const item of a) count += +b.has(item)
158164
return count
159165
}
166+
167+
if (import.meta.vitest) {
168+
const { test, expect } = import.meta.vitest
169+
test.each`
170+
left | right | expected
171+
${[]} | ${[]} | ${0}
172+
${[3]} | ${[]} | ${0}
173+
${[]} | ${[3]} | ${0}
174+
${[3]} | ${[3]} | ${1}
175+
${[1, 2, 3]} | ${[2, 3, 4]} | ${2}
176+
`('Count common in sets $left and $right', ({ left, right, expected }) => {
177+
expect(countCommonInSets(new Set(left), new Set(right))).toBe(expected)
178+
})
179+
}

0 commit comments

Comments
 (0)