Skip to content

Forward scroll unless focused #6597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jtydhr88 merged 5 commits into from
Feb 1, 2026
Merged

Forward scroll unless focused #6597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02424d9
Forward scroll unless focused
benceruleanlu Nov 5, 2025
3e13237
test: add tests for focus-dependent wheel capture behavior (#8458)
christian-byrne Jan 30, 2026
5c2030d
Merge origin/main into bl-visual-impala-358
benceruleanlu Jan 30, 2026
ae84b22
fix: keep ctrl+wheel zoom when focused
benceruleanlu Jan 30, 2026
0b28200
fix: focus Load3D scene on pointerdown
benceruleanlu Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/components/load3d/Load3DScene.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
ref="container"
class="relative h-full w-full min-h-[200px]"
data-capture-wheel="true"
@pointerdown.stop
tabindex="-1"
@pointerdown.stop="focusContainer"
@pointermove.stop
@pointerup.stop
@mousedown.stop
Expand Down Expand Up @@ -45,6 +46,10 @@ const props = defineProps<{

const container = ref<HTMLElement | null>(null)

function focusContainer() {
container.value?.focus()
}

const { isDragging, dragMessage, handleDragOver, handleDragLeave, handleDrop } =
useLoad3dDrag({
onModelDrop: async (file) => {
Expand Down
67 changes: 67 additions & 0 deletions src/renderer/core/canvas/useCanvasInteractions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,72 @@ describe('useCanvasInteractions', () => {
expect(mockEvent.preventDefault).not.toHaveBeenCalled()
expect(mockEvent.stopPropagation).not.toHaveBeenCalled()
})
it('should forward wheel events to canvas when capture element is NOT focused', () => {
const { get } = useSettingStore()
vi.mocked(get).mockReturnValue('legacy')

const captureElement = document.createElement('div')
captureElement.setAttribute('data-capture-wheel', 'true')
const textarea = document.createElement('textarea')
captureElement.appendChild(textarea)
document.body.appendChild(captureElement)

const { handleWheel } = useCanvasInteractions()
const mockEvent = createMockWheelEvent()
Object.defineProperty(mockEvent, 'target', { value: textarea })

handleWheel(mockEvent)

expect(mockEvent.preventDefault).toHaveBeenCalled()
expect(mockEvent.stopPropagation).toHaveBeenCalled()

document.body.removeChild(captureElement)
})

it('should NOT forward wheel events when capture element IS focused', () => {
const { get } = useSettingStore()
vi.mocked(get).mockReturnValue('legacy')

const captureElement = document.createElement('div')
captureElement.setAttribute('data-capture-wheel', 'true')
const textarea = document.createElement('textarea')
captureElement.appendChild(textarea)
document.body.appendChild(captureElement)
textarea.focus()

const { handleWheel } = useCanvasInteractions()
const mockEvent = createMockWheelEvent()
Object.defineProperty(mockEvent, 'target', { value: textarea })

handleWheel(mockEvent)

expect(mockEvent.preventDefault).not.toHaveBeenCalled()
expect(mockEvent.stopPropagation).not.toHaveBeenCalled()

document.body.removeChild(captureElement)
})

it('should forward ctrl+wheel to canvas when capture element IS focused in standard mode', () => {
const { get } = useSettingStore()
vi.mocked(get).mockReturnValue('standard')

const captureElement = document.createElement('div')
captureElement.setAttribute('data-capture-wheel', 'true')
const textarea = document.createElement('textarea')
captureElement.appendChild(textarea)
document.body.appendChild(captureElement)
textarea.focus()

const { handleWheel } = useCanvasInteractions()
const mockEvent = createMockWheelEvent(true)
Object.defineProperty(mockEvent, 'target', { value: textarea })

handleWheel(mockEvent)

expect(mockEvent.preventDefault).toHaveBeenCalled()
expect(mockEvent.stopPropagation).toHaveBeenCalled()

document.body.removeChild(captureElement)
})
Comment on lines +203 to +224
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test only checks ctrlKey but the implementation also supports metaKey (for macOS Command key). Consider adding a test case for metaKey to ensure both keyboard modifiers work correctly on different platforms.

Copilot uses AI. Check for mistakes.
Comment on lines +158 to +224
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for standard navigation mode with a regular wheel event (no Ctrl/Meta) when the capture element is NOT focused. This scenario should test whether the wheel event is properly forwarded to the canvas for panning, which is a key part of the PR's goal.

Copilot uses AI. Check for mistakes.
Comment on lines +158 to +224
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use real WheelEvent instances so the instanceof paths are exercised.

The new tests use a plain object cast to WheelEvent, so event instanceof WheelEvent is false and the wheel-specific forwarding path isn’t actually exercised. Consider constructing real WheelEvent instances and spying on preventDefault/stopPropagation (or assert defaultPrevented/cancelBubble) to ensure the behavior is covered.

✅ Example helper update (uses real WheelEvent) 
function createMockWheelEvent(ctrlKey = false, metaKey = false): WheelEvent {
  const event = new WheelEvent('wheel', { ctrlKey, metaKey })
  vi.spyOn(event, 'preventDefault')
  vi.spyOn(event, 'stopPropagation')
  return event
}

Based on learnings: Aim for behavioral coverage of critical and new features.

🤖 Prompt for AI Agents 
In `@src/renderer/core/canvas/useCanvasInteractions.test.ts` around lines 158 -
224, The tests in useCanvasInteractions.test.ts are creating plain objects cast
to WheelEvent so event instanceof WheelEvent is false and the wheel-specific
code path in useCanvasInteractions.handleWheel isn't exercised; update the
createMockWheelEvent helper to construct real WheelEvent instances (e.g., new
WheelEvent('wheel', { ctrlKey, metaKey })) and spy on
preventDefault/stopPropagation (or assert defaultPrevented/cancelBubble) so
handleWheel behavior is actually covered when calling
useCanvasInteractions().handleWheel.

})
})
38 changes: 22 additions & 16 deletions src/renderer/core/canvas/useCanvasInteractions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,31 @@ export function useCanvasInteractions() {
() => !(canvasStore.canvas?.read_only ?? false)
)

/**
* Returns true if the wheel event target is inside an element that should
* capture wheel events AND that element (or a descendant) currently has focus.
*
* This allows two-finger panning to continue over inputs until the user has
* actively focused the widget, at which point the widget can consume scroll.
*/
const wheelCapturedByFocusedElement = (event: WheelEvent): boolean => {
const target = event.target as HTMLElement | null
const captureElement = target?.closest('[data-capture-wheel="true"]')
const active = document.activeElement as Element | null

return !!(captureElement && active && captureElement.contains(active))
}

const shouldForwardWheelEvent = (event: WheelEvent): boolean =>
!wheelCapturedByFocusedElement(event) ||
(isStandardNavMode.value && (event.ctrlKey || event.metaKey))
Comment on lines +44 to +46
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid forwarding wheel from non-focusable capture zones

Because shouldForwardWheelEvent now returns true whenever the capture element is not focused, any data-capture-wheel element that can’t receive focus will always have its wheel events intercepted by the capture-phase listener (@wheel.capture="canvasInteractions.forwardEventToCanvas" in GraphCanvas.vue). That means the original target never sees the wheel event (preventDefault + stopPropagation happen in capture), so widgets that rely on wheel without focus will stop working. A concrete example is Load3DScene.vue, which marks the container with data-capture-wheel but doesn’t make it focusable; after this change, wheel zoom/pan in the 3D viewer will never fire unless the element is made focusable or the capture logic treats non-focusable elements as captured.

Useful? React with 👍 / 👎.

Copy link
Member Author

@benceruleanlu benceruleanlu Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true.

Copy link
Member Author

@benceruleanlu benceruleanlu Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's actually incredible that ChatGPT was the one that came up with the best review comment here. This is actually something that's insane. I don't know how it managed to catch this one where like CodeRabbit and Copilot both missed it

Copy link
Member Author

@benceruleanlu benceruleanlu Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically the behavior I'm leaning towards right now is that the user would need to click on "Load3D Scene" for them to be able to pan it. I think that's perfectly reasonable because that's really the only solution that lets us let the user pan across the graph without having their pan be stopped in his tracks the second it manages to go over the "Load3D Scene" widget. I think that's reasonable and I made that change

Copy link
Contributor

@christian-byrne christian-byrne Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how does it feel to use? Maybe we can focus the 3D scene when clicking the node.

Copy link
Member Author

@benceruleanlu benceruleanlu Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels fine? Then deselection would open another discussion. And I think that complicates things by introducing widget specific exceptions to a global rule.

Comment on lines +29 to +46
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Prefer function declarations for the new pure helpers.

Both helpers are pure and can be declared as functions to match the repo guideline.
[replacements below keep behavior unchanged]

♻️ Suggested refactor 
-  const wheelCapturedByFocusedElement = (event: WheelEvent): boolean => {
+  function wheelCapturedByFocusedElement(event: WheelEvent): boolean {
     const target = event.target as HTMLElement | null
     const captureElement = target?.closest('[data-capture-wheel="true"]')
     const active = document.activeElement as Element | null

     return !!(captureElement && active && captureElement.contains(active))
   }

-  const shouldForwardWheelEvent = (event: WheelEvent): boolean =>
-    !wheelCapturedByFocusedElement(event) ||
-    (isStandardNavMode.value && (event.ctrlKey || event.metaKey))
+  function shouldForwardWheelEvent(event: WheelEvent): boolean {
+    return (
+      !wheelCapturedByFocusedElement(event) ||
+      (isStandardNavMode.value && (event.ctrlKey || event.metaKey))
+    )
+  }

As per coding guidelines: Do not use function expressions if it is possible to use function declarations instead.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Returns true if the wheel event target is inside an element that should
* capture wheel events AND that element (or a descendant) currently has focus.
*
* This allows two-finger panning to continue over inputs until the user has
* actively focused the widget, at which point the widget can consume scroll.
*/
const wheelCapturedByFocusedElement = (event: WheelEvent): boolean => {
const target = event.target as HTMLElement | null
const captureElement = target?.closest('[data-capture-wheel="true"]')
const active = document.activeElement as Element | null
return !!(captureElement && active && captureElement.contains(active))
}
const shouldForwardWheelEvent = (event: WheelEvent): boolean =>
!wheelCapturedByFocusedElement(event) ||
(isStandardNavMode.value && (event.ctrlKey || event.metaKey))
/**
* Returns true if the wheel event target is inside an element that should
* capture wheel events AND that element (or a descendant) currently has focus.
*
* This allows two-finger panning to continue over inputs until the user has
* actively focused the widget, at which point the widget can consume scroll.
*/
function wheelCapturedByFocusedElement(event: WheelEvent): boolean {
const target = event.target as HTMLElement | null
const captureElement = target?.closest('[data-capture-wheel="true"]')
const active = document.activeElement as Element | null
return !!(captureElement && active && captureElement.contains(active))
}
function shouldForwardWheelEvent(event: WheelEvent): boolean {
return (
!wheelCapturedByFocusedElement(event) ||
(isStandardNavMode.value && (event.ctrlKey || event.metaKey))
)
}
🤖 Prompt for AI Agents 
In `@src/renderer/core/canvas/useCanvasInteractions.ts` around lines 29 - 46,
Convert the two helper function expressions to function declarations: change the
const wheelCapturedByFocusedElement = (event: WheelEvent): boolean => { ... }
into function wheelCapturedByFocusedElement(event: WheelEvent): boolean { ... }
and change the const shouldForwardWheelEvent = (event: WheelEvent): boolean =>
... into function shouldForwardWheelEvent(event: WheelEvent): boolean { return
... } so they follow the repo guideline for pure helpers while preserving
existing behavior and references.


/**
* Handles wheel events from UI components that should be forwarded to canvas
* when appropriate (e.g., Ctrl+wheel for zoom in standard mode)
*/
const handleWheel = (event: WheelEvent) => {
// Check if the wheel event is from an element that wants to capture wheel events
const target = event.target as HTMLElement
const captureElement = target?.closest('[data-capture-wheel="true"]')

if (captureElement) {
// Element wants to capture wheel events, don't forward to canvas
return
}
if (!shouldForwardWheelEvent(event)) return

// In standard mode, Ctrl+wheel should go to canvas for zoom
if (isStandardNavMode.value && (event.ctrlKey || event.metaKey)) {
Expand Down Expand Up @@ -87,14 +99,8 @@ export function useCanvasInteractions() {
const forwardEventToCanvas = (
event: WheelEvent | PointerEvent | MouseEvent
) => {
// Check if the wheel event is from an element that wants to capture wheel events
const target = event.target as HTMLElement
const captureElement = target?.closest('[data-capture-wheel="true"]')

if (captureElement) {
// Element wants to capture wheel events, don't forward to canvas
return
}
// Honor wheel capture only when the element is focused
if (event instanceof WheelEvent && !shouldForwardWheelEvent(event)) return
Comment on lines +102 to +103
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forwardEventToCanvas function has new wheel event capture logic but lacks dedicated test coverage. While handleWheel tests indirectly exercise this path, consider adding explicit tests for forwardEventToCanvas to verify it properly respects the focus-based wheel capture when called directly (not just through handleWheel).

Copilot uses AI. Check for mistakes.

const canvasEl = app.canvas?.canvas
if (!canvasEl) return
Expand Down