Skip to content

Commit 1247380

Browse files
perf(maskeditor): read the brush container rect once per move, not per axis (#15384)
## Summary `brushLeft` and `brushTop` in `BrushCursor.vue` each called `containerRef.getBoundingClientRect()`. Both depend on `store.cursorPoint`, which changes on every mousemove, so painting in the mask editor cost **two forced synchronous layouts per pointer event** to obtain one rect. ## The change One `brushPosition` computed reads the rect once and returns both axes; `brushLeft` and `brushTop` read from it. ## Why this is safe The read stays inside a computed over `cursorPoint` deliberately. A `DOMRect` is not reactive, so nothing would invalidate a cached one — re-reading it as the cursor moves is precisely what keeps the offset correct while the mask editor dialog is dragged or resized. **So this changes how many reads happen, not when.** Dialog move and resize behave exactly as before, which is what makes it reviewable without an interactive pass over those paths. ## Tests Added `should read the container rect once per position, not once per axis`, which spies on the container's `getBoundingClientRect`. Verified it fails on current `main`: ``` AssertionError: left and top come from the same rect; reading it twice is two forced layouts per mousemove: expected "getBoundingClientRect" to be called 1 times, but got 2 times ``` 12 tests green, `oxfmt --check` and `oxlint` clean, `vue-tsc` reports nothing on either file. ## Review focus - The existing offset test (`should subtract container offset when containerRef is provided`) still passes unchanged, which is the evidence that positioning is untouched. - If anyone wants the rect cached across moves instead, that is a different and larger change: it needs a `ResizeObserver` plus a drag hook on the dialog, and it would alter behaviour rather than cost. Deliberately not attempted here.
1 parent f9ec97a commit 1247380

2 files changed

Lines changed: 73 additions & 19 deletions

File tree

src/components/maskeditor/BrushCursor.test.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { render, screen } from '@testing-library/vue'
2-
import { reactive } from 'vue'
2+
import { nextTick, reactive } from 'vue'
33
import { beforeEach, describe, expect, it, vi } from 'vitest'
44

55
import BrushCursor from '@/components/maskeditor/BrushCursor.vue'
@@ -140,6 +140,51 @@ describe('BrushCursor', () => {
140140
expect(style).toContain('left: 150px')
141141
expect(style).toContain('top: 220px')
142142
})
143+
144+
it('should read the container rect once per position, not once per axis', async () => {
145+
mockStore.cursorPoint = { x: 200, y: 300 }
146+
mockStore.panOffset = { x: 0, y: 0 }
147+
mockStore.brushSettings.size = 20
148+
mockStore.brushSettings.hardness = 1
149+
mockStore.zoomRatio = 1
150+
151+
const container = document.createElement('div')
152+
const readRect = vi
153+
.spyOn(container, 'getBoundingClientRect')
154+
.mockReturnValue({
155+
left: 30,
156+
top: 60,
157+
right: 0,
158+
bottom: 0,
159+
width: 0,
160+
height: 0,
161+
x: 0,
162+
y: 0,
163+
toJSON: () => ({})
164+
} as DOMRect)
165+
166+
renderCursor(container)
167+
168+
// `getBoundingClientRect` forces a synchronous layout and the cursor
169+
// moves on every mousemove, so reading it per axis doubled the cost for
170+
// one rect.
171+
expect(
172+
readRect,
173+
'left and top come from the same rect; reading it twice is two forced layouts per mousemove'
174+
).toHaveBeenCalledTimes(1)
175+
176+
mockStore.cursorPoint = { x: 201, y: 301 }
177+
await nextTick()
178+
179+
// The other half of the guarantee, and the one that keeps the offset
180+
// correct while the dialog is dragged: a DOMRect is not reactive, so
181+
// caching it across moves would pass the assertion above and silently
182+
// stop tracking the container.
183+
expect(
184+
readRect,
185+
'a moved cursor must re-read the rect; a cached one goes stale the moment the dialog moves'
186+
).toHaveBeenCalledTimes(2)
187+
})
143188
})
144189

145190
describe('gradient preview', () => {

src/components/maskeditor/BrushCursor.vue

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -56,27 +56,36 @@ const brushSize = computed(() => {
5656
return brushRadius.value * 2
5757
})
5858
59-
const brushLeft = computed(() => {
59+
/**
60+
* One rect read per move, not one per axis. `cursorPoint` changes on every
61+
* mousemove and `getBoundingClientRect` forces layout, so reading it separately
62+
* in `brushLeft` and `brushTop` cost two synchronous layouts per event for a
63+
* single rect.
64+
*
65+
* The read is deliberately still inside a computed over `cursorPoint`: the rect
66+
* is not reactive, and re-reading it as the cursor moves is what keeps the
67+
* offset correct while the dialog is dragged or resized. This halves the reads
68+
* without changing when they happen.
69+
*/
70+
const brushPosition = computed(() => {
6071
const dialogRect = containerRef?.getBoundingClientRect()
61-
const dialogOffsetLeft = dialogRect?.left || 0
62-
return (
63-
store.cursorPoint.x +
64-
store.panOffset.x -
65-
brushRadius.value -
66-
dialogOffsetLeft
67-
)
72+
return {
73+
left:
74+
store.cursorPoint.x +
75+
store.panOffset.x -
76+
brushRadius.value -
77+
(dialogRect?.left || 0),
78+
top:
79+
store.cursorPoint.y +
80+
store.panOffset.y -
81+
brushRadius.value -
82+
(dialogRect?.top || 0)
83+
}
6884
})
6985
70-
const brushTop = computed(() => {
71-
const dialogRect = containerRef?.getBoundingClientRect()
72-
const dialogOffsetTop = dialogRect?.top || 0
73-
return (
74-
store.cursorPoint.y +
75-
store.panOffset.y -
76-
brushRadius.value -
77-
dialogOffsetTop
78-
)
79-
})
86+
const brushLeft = computed(() => brushPosition.value.left)
87+
88+
const brushTop = computed(() => brushPosition.value.top)
8089
8190
const borderRadius = computed(() => {
8291
return store.brushSettings.type === BrushShape.Rect ? '0%' : '50%'

0 commit comments

Comments
 (0)