mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
fix: resolve subgraph node slot link misalignment during workflow load (#9121)
## Summary Fix subgraph node slot connector links appearing misaligned after workflow load, caused by a transform desync between LiteGraph's internal canvas transform and the Vue TransformPane's CSS transform. ## Changes - **What**: Changed `syncNodeSlotLayoutsFromDOM` to use DOM-relative measurement (slot position relative to its parent `[data-node-id]` element) instead of absolute canvas-space conversion via `clientPosToCanvasPos`. This makes the slot offset calculation independent of the global canvas transform, eliminating the frame-lag desync that occurred when `fitView()` updated `lgCanvas.ds` before the Vue CSS transform caught up. - **Cleanup**: Removed the unreachable fallback path that still used `clientPosToCanvasPos` when the parent node element wasn't found (every slot element is necessarily a child of a `[data-node-id]` element — if `closest()` fails the element is detached and measuring is meaningless). This also removed the `conv` parameter from `syncNodeSlotLayoutsFromDOM` and `flushScheduledSlotLayoutSync`, and the `useSharedCanvasPositionConversion` import. - **Test**: Added a Playwright browser test that loads a subgraph workflow with `workflowRendererVersion: "LG"` (triggering the 1.2x scale in `ensureCorrectLayoutScale`) as a template (triggering `fitView`), and verifies all slot connector positions are within bounds of their parent node element. ## Review Focus - The core change is in `useSlotElementTracking.ts` — the new measurement approach uses `getBoundingClientRect()` on both the slot and its parent node element, then divides by `currentScale` to get canvas-space offsets. This is simpler and more robust than the previous approach. - SubgraphNodes were disproportionately affected because they are relatively static and don't often trigger `ResizeObserver`-based re-syncs that would eventually correct stale offsets. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9121-fix-resolve-subgraph-node-slot-link-misalignment-during-workflow-load-3106d73d365081eca413c84f2e0571d6) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <448862+DrJKL@users.noreply.github.com> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
This commit is contained in:
132
browser_tests/tests/subgraphSlotAlignment.spec.ts
Normal file
132
browser_tests/tests/subgraphSlotAlignment.spec.ts
Normal file
@@ -0,0 +1,132 @@
|
||||
import { readFileSync } from 'fs'
|
||||
import { resolve } from 'path'
|
||||
|
||||
import { expect } from '@playwright/test'
|
||||
|
||||
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
|
||||
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
|
||||
|
||||
interface SlotMeasurement {
|
||||
key: string
|
||||
offsetX: number
|
||||
offsetY: number
|
||||
}
|
||||
|
||||
interface NodeSlotData {
|
||||
nodeId: string
|
||||
isSubgraph: boolean
|
||||
nodeW: number
|
||||
nodeH: number
|
||||
slots: SlotMeasurement[]
|
||||
}
|
||||
|
||||
/**
|
||||
* Regression test for link misalignment on SubgraphNodes when loading
|
||||
* workflows with workflowRendererVersion: "LG".
|
||||
*
|
||||
* Root cause: ensureCorrectLayoutScale scales nodes by 1.2x for LG workflows,
|
||||
* and fitView() updates lgCanvas.ds immediately. The Vue TransformPane's CSS
|
||||
* transform lags by a frame, causing clientPosToCanvasPos to produce wrong
|
||||
* slot offsets. The fix uses DOM-relative measurement instead.
|
||||
*/
|
||||
test.describe(
|
||||
'Subgraph slot alignment after LG layout scale',
|
||||
{ tag: ['@subgraph', '@canvas'] },
|
||||
() => {
|
||||
test('slot positions stay within node bounds after loading LG workflow', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
const SLOT_BOUNDS_MARGIN = 20
|
||||
await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', true)
|
||||
|
||||
const workflowPath = resolve(
|
||||
import.meta.dirname,
|
||||
'../assets/subgraphs/basic-subgraph.json'
|
||||
)
|
||||
const workflow = JSON.parse(
|
||||
readFileSync(workflowPath, 'utf-8')
|
||||
) as ComfyWorkflowJSON
|
||||
workflow.extra = {
|
||||
...workflow.extra,
|
||||
workflowRendererVersion: 'LG'
|
||||
}
|
||||
|
||||
await comfyPage.page.evaluate(
|
||||
(wf) =>
|
||||
window.app!.loadGraphData(wf as ComfyWorkflowJSON, true, true, null, {
|
||||
openSource: 'template'
|
||||
}),
|
||||
workflow
|
||||
)
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
// Wait for slot elements to appear in DOM
|
||||
await comfyPage.page.locator('[data-slot-key]').first().waitFor()
|
||||
|
||||
const result: NodeSlotData[] = await comfyPage.page.evaluate(() => {
|
||||
const nodes = window.app!.graph._nodes
|
||||
const slotData: NodeSlotData[] = []
|
||||
|
||||
for (const node of nodes) {
|
||||
const nodeId = String(node.id)
|
||||
const nodeEl = document.querySelector(
|
||||
`[data-node-id="${nodeId}"]`
|
||||
) as HTMLElement | null
|
||||
if (!nodeEl) continue
|
||||
|
||||
const slotEls = nodeEl.querySelectorAll('[data-slot-key]')
|
||||
if (slotEls.length === 0) continue
|
||||
|
||||
const slots: SlotMeasurement[] = []
|
||||
|
||||
const nodeRect = nodeEl.getBoundingClientRect()
|
||||
for (const slotEl of slotEls) {
|
||||
const slotRect = slotEl.getBoundingClientRect()
|
||||
const slotKey = (slotEl as HTMLElement).dataset.slotKey ?? 'unknown'
|
||||
slots.push({
|
||||
key: slotKey,
|
||||
offsetX: slotRect.left + slotRect.width / 2 - nodeRect.left,
|
||||
offsetY: slotRect.top + slotRect.height / 2 - nodeRect.top
|
||||
})
|
||||
}
|
||||
|
||||
slotData.push({
|
||||
nodeId,
|
||||
isSubgraph: !!node.isSubgraphNode?.(),
|
||||
nodeW: nodeRect.width,
|
||||
nodeH: nodeRect.height,
|
||||
slots
|
||||
})
|
||||
}
|
||||
|
||||
return slotData
|
||||
})
|
||||
|
||||
const subgraphNodes = result.filter((n) => n.isSubgraph)
|
||||
expect(subgraphNodes.length).toBeGreaterThan(0)
|
||||
|
||||
for (const node of subgraphNodes) {
|
||||
for (const slot of node.slots) {
|
||||
expect(
|
||||
slot.offsetX,
|
||||
`Slot ${slot.key} on node ${node.nodeId}: X offset ${slot.offsetX} outside node width ${node.nodeW}`
|
||||
).toBeGreaterThanOrEqual(-SLOT_BOUNDS_MARGIN)
|
||||
expect(
|
||||
slot.offsetX,
|
||||
`Slot ${slot.key} on node ${node.nodeId}: X offset ${slot.offsetX} outside node width ${node.nodeW}`
|
||||
).toBeLessThanOrEqual(node.nodeW + SLOT_BOUNDS_MARGIN)
|
||||
|
||||
expect(
|
||||
slot.offsetY,
|
||||
`Slot ${slot.key} on node ${node.nodeId}: Y offset ${slot.offsetY} outside node height ${node.nodeH}`
|
||||
).toBeGreaterThanOrEqual(-SLOT_BOUNDS_MARGIN)
|
||||
expect(
|
||||
slot.offsetY,
|
||||
`Slot ${slot.key} on node ${node.nodeId}: Y offset ${slot.offsetY} outside node height ${node.nodeH}`
|
||||
).toBeLessThanOrEqual(node.nodeH + SLOT_BOUNDS_MARGIN)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
)
|
||||
@@ -24,12 +24,6 @@ vi.mock('@/scripts/app', () => ({
|
||||
app: { canvas: { graph: mockGraph, setDirty: vi.fn() } }
|
||||
}))
|
||||
|
||||
vi.mock('@/composables/element/useCanvasPositionConversion', () => ({
|
||||
useSharedCanvasPositionConversion: () => ({
|
||||
clientPosToCanvasPos: ([x, y]: [number, number]) => [x, y]
|
||||
})
|
||||
}))
|
||||
|
||||
const NODE_ID = 'test-node'
|
||||
const SLOT_INDEX = 0
|
||||
|
||||
@@ -51,15 +45,47 @@ function createWrapperComponent(type: 'input' | 'output') {
|
||||
})
|
||||
}
|
||||
|
||||
function createSlotElement(): HTMLElement {
|
||||
const container = document.createElement('div')
|
||||
container.dataset.nodeId = NODE_ID
|
||||
container.getBoundingClientRect = () =>
|
||||
({
|
||||
left: 0,
|
||||
top: 0,
|
||||
right: 200,
|
||||
bottom: 100,
|
||||
width: 200,
|
||||
height: 100,
|
||||
x: 0,
|
||||
y: 0,
|
||||
toJSON: () => ({})
|
||||
}) as DOMRect
|
||||
document.body.appendChild(container)
|
||||
|
||||
const el = document.createElement('div')
|
||||
el.getBoundingClientRect = () =>
|
||||
({
|
||||
left: 10,
|
||||
top: 30,
|
||||
right: 20,
|
||||
bottom: 40,
|
||||
width: 10,
|
||||
height: 10,
|
||||
x: 10,
|
||||
y: 30,
|
||||
toJSON: () => ({})
|
||||
}) as DOMRect
|
||||
container.appendChild(el)
|
||||
|
||||
return el
|
||||
}
|
||||
|
||||
/**
|
||||
* Mount the wrapper, set the element ref, and wait for slot registration.
|
||||
*/
|
||||
async function mountAndRegisterSlot(type: 'input' | 'output') {
|
||||
const wrapper = mount(createWrapperComponent(type))
|
||||
const slotEl = document.createElement('div')
|
||||
slotEl.getBoundingClientRect = vi.fn(() => new DOMRect(100, 200, 16, 16))
|
||||
document.body.append(slotEl)
|
||||
wrapper.vm.el = slotEl
|
||||
wrapper.vm.el = createSlotElement()
|
||||
await nextTick()
|
||||
flushScheduledSlotLayoutSync()
|
||||
return wrapper
|
||||
@@ -186,17 +212,19 @@ describe('useSlotElementTracking', () => {
|
||||
|
||||
it('skips slot layout writeback when measured slot geometry is unchanged', () => {
|
||||
const slotKey = getSlotKey(NODE_ID, SLOT_INDEX, true)
|
||||
const slotEl = document.createElement('div')
|
||||
document.body.append(slotEl)
|
||||
slotEl.getBoundingClientRect = vi.fn(() => new DOMRect(100, 200, 16, 16))
|
||||
const slotEl = createSlotElement()
|
||||
|
||||
const registryStore = useNodeSlotRegistryStore()
|
||||
const node = registryStore.ensureNode(NODE_ID)
|
||||
|
||||
const expectedX = 15
|
||||
const expectedY = 35 - LiteGraph.NODE_TITLE_HEIGHT
|
||||
|
||||
node.slots.set(slotKey, {
|
||||
el: slotEl,
|
||||
index: SLOT_INDEX,
|
||||
type: 'input',
|
||||
cachedOffset: { x: 108, y: 208 }
|
||||
cachedOffset: { x: expectedX, y: expectedY }
|
||||
})
|
||||
|
||||
const slotSize = LiteGraph.NODE_SLOT_HEIGHT
|
||||
@@ -205,10 +233,10 @@ describe('useSlotElementTracking', () => {
|
||||
nodeId: NODE_ID,
|
||||
index: SLOT_INDEX,
|
||||
type: 'input',
|
||||
position: { x: 108, y: 208 },
|
||||
position: { x: expectedX, y: expectedY },
|
||||
bounds: {
|
||||
x: 108 - halfSlotSize,
|
||||
y: 208 - halfSlotSize,
|
||||
x: expectedX - halfSlotSize,
|
||||
y: expectedY - halfSlotSize,
|
||||
width: slotSize,
|
||||
height: slotSize
|
||||
}
|
||||
|
||||
@@ -8,7 +8,6 @@
|
||||
import { onMounted, onUnmounted, watch } from 'vue'
|
||||
import type { Ref } from 'vue'
|
||||
|
||||
import { useSharedCanvasPositionConversion } from '@/composables/element/useCanvasPositionConversion'
|
||||
import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
||||
import { getSlotKey } from '@/renderer/core/layout/slots/slotIdentifier'
|
||||
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
|
||||
@@ -102,10 +101,9 @@ export function flushScheduledSlotLayoutSync() {
|
||||
completePendingSlotSync()
|
||||
return
|
||||
}
|
||||
const conv = useSharedCanvasPositionConversion()
|
||||
for (const nodeId of Array.from(pendingNodes)) {
|
||||
pendingNodes.delete(nodeId)
|
||||
syncNodeSlotLayoutsFromDOM(nodeId, conv)
|
||||
syncNodeSlotLayoutsFromDOM(nodeId)
|
||||
}
|
||||
|
||||
// Keep pending sync active until at least one measurable slot layout has
|
||||
@@ -115,19 +113,33 @@ export function flushScheduledSlotLayoutSync() {
|
||||
completePendingSlotSync()
|
||||
}
|
||||
|
||||
export function syncNodeSlotLayoutsFromDOM(
|
||||
nodeId: string,
|
||||
conv?: ReturnType<typeof useSharedCanvasPositionConversion>
|
||||
) {
|
||||
export function syncNodeSlotLayoutsFromDOM(nodeId: string) {
|
||||
const nodeSlotRegistryStore = useNodeSlotRegistryStore()
|
||||
const node = nodeSlotRegistryStore.getNode(nodeId)
|
||||
if (!node) return
|
||||
const nodeLayout = layoutStore.getNodeLayoutRef(nodeId).value
|
||||
if (!nodeLayout) return
|
||||
|
||||
const batch: Array<{ key: string; layout: SlotLayout }> = []
|
||||
// Find the node's DOM element for relative offset measurement.
|
||||
// Using DOM-relative measurement avoids the transform desync issue where
|
||||
// lgCanvas.ds (used by clientPosToCanvasPos) can diverge from the
|
||||
// TransformPane's CSS transform during workflow loading (e.g., after
|
||||
// fitView or ensureCorrectLayoutScale). Both the slot and node elements
|
||||
// share the same DOM transform, so their pixel difference divided by the
|
||||
// effective scale yields a correct canvas-space offset regardless of
|
||||
// whether the TransformPane has flushed its latest transform to the DOM.
|
||||
const closestNode = node.slots
|
||||
.values()
|
||||
.next()
|
||||
.value?.el.closest('[data-node-id]')
|
||||
const nodeEl = closestNode instanceof HTMLElement ? closestNode : null
|
||||
const nodeRect = nodeEl?.getBoundingClientRect()
|
||||
const effectiveScale =
|
||||
nodeRect && nodeLayout.size.width > 0
|
||||
? nodeRect.width / nodeLayout.size.width
|
||||
: 0
|
||||
|
||||
const positionConv = conv ?? useSharedCanvasPositionConversion()
|
||||
const batch: Array<{ key: string; layout: SlotLayout }> = []
|
||||
|
||||
for (const [slotKey, entry] of node.slots) {
|
||||
const rect = getSlotElementRect(entry.el)
|
||||
@@ -142,13 +154,23 @@ export function syncNodeSlotLayoutsFromDOM(
|
||||
rect.left + rect.width / 2,
|
||||
rect.top + rect.height / 2
|
||||
]
|
||||
const [x, y] = positionConv.clientPosToCanvasPos(screenCenter)
|
||||
const centerCanvas = { x, y }
|
||||
|
||||
// Cache offset relative to node position for fast updates later
|
||||
if (!nodeRect || effectiveScale <= 0) continue
|
||||
|
||||
// DOM-relative measurement: compute offset from the node element's
|
||||
// top-left corner in canvas units. The node element is rendered at
|
||||
// (position.x, position.y - NODE_TITLE_HEIGHT), so the Y offset
|
||||
// must subtract NODE_TITLE_HEIGHT to be relative to position.y.
|
||||
entry.cachedOffset = {
|
||||
x: centerCanvas.x - nodeLayout.position.x,
|
||||
y: centerCanvas.y - nodeLayout.position.y
|
||||
x: (screenCenter[0] - nodeRect.left) / effectiveScale,
|
||||
y:
|
||||
(screenCenter[1] - nodeRect.top) / effectiveScale -
|
||||
LiteGraph.NODE_TITLE_HEIGHT
|
||||
}
|
||||
|
||||
const centerCanvas = {
|
||||
x: nodeLayout.position.x + entry.cachedOffset.x,
|
||||
y: nodeLayout.position.y + entry.cachedOffset.y
|
||||
}
|
||||
|
||||
const nextLayout = createSlotLayout({
|
||||
|
||||
Reference in New Issue
Block a user