Fix: enable wheel zoom outside graph area #473
Conversation
…wisp#421 Enable scrolling to zoom in/out from anywhere on the graph container, not just over the graph nodes themselves. This matches the behavior of the geographic map mode. Fixes openwisp#421 fix: add safety check for getDom in tests
atif09
force-pushed
the
issues/421-enable-wheel-zoom-outside-graph
branch
from
9006bee to
1db2cc2
Compare
atif09
force-pushed
the
issues/421-enable-wheel-zoom-outside-graph
branch
from
6cc76fd to
a588f58
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I had tested this and it was working, which is good ✔️ !
- Let's fix the QA checks, try the following:
pip install -U --force-reinstall "openwisp-utils[qa] @ https://github.com/openwisp/openwisp-utils/tarball/master"
yarn lint:fix
openwisp-qa-format
-
We also need at least 1 automated browser test for this to protect ourselves against regressions.
-
Please @coderabbitai review. @atif09 please follow up with its reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request adds wheel-based zoom handling for areas outside the graph/map drawable surface. When a wheel event occurs on the chart DOM container but outside the graph/map area, the code now prevents the default behavior, locates the inner canvas element, and dispatches a synthetic wheel event centered on the canvas to enable zoom operations. This extends zoom functionality to the entire chart container area. Additionally, minor formatting adjustments were made throughout the file for readability without altering existing behavior. Sequence DiagramsequenceDiagram
actor User
participant ChartDOM as Chart Container<br/>(DOM)
participant Handler as Wheel Event<br/>Handler
participant Canvas as Canvas<br/>(Graph/Map)
User->>ChartDOM: Scroll wheel outside graph area
ChartDOM->>Handler: Trigger wheel event
Handler->>Handler: Check if outside graph/map
Handler->>Handler: preventDefault()
Handler->>Canvas: Find inner canvas element
Handler->>Canvas: Create & dispatch synthetic<br/>wheel event (centered)
Canvas->>Canvas: Process wheel event for zoom
Canvas->>User: Update zoom level
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.render.js:
- Around line 441-475: The wheel-forwarding handler must be made robust: stop
querying canvas via dom.querySelector("canvas") and instead get the ZRender
surface via self.echarts.getZr().dom (works for canvas and SVG), preserve and
forward all relevant event fields (deltaX, deltaY, deltaMode, ctrlKey, shiftKey,
altKey, metaKey, button, etc.) without inverting sign, and avoid
double-forwarding by detecting if the original event already originated on the
ZRender DOM (skip forwarding when e.target === zrDom or
zrDom.contains(e.target)). Also prevent listener accumulation by storing the
listener (e.g., this._wheelForwardHandler) and removing any previous listener
from dom before attaching a new one so it can be cleaned up on re-renders;
ensure you add the new listener with {passive:false} as before.
🧹 Nitpick comments (2)
src/js/netjsongraph.render.js (2)
2-7: Prettier is failing; please re-run formatter before mergeCI reports Prettier failures for this file; several hunks here are line-wrapping-only changes, so this is likely just a formatting mismatch with the repo’s Prettier config.
LGTM on the “wrap-only” changes, and the
getBBoxData.call(self, ...)this-binding is a good safety fix
- The graph series
typeselection refactor preserves behavior (Line 208-214).- Map node emphasis extraction/wrapping looks behavior-preserving (Line 276-280).
getBBoxData.call(self, ...)avoids accidentalthisissues ifgetBBoxDatarelies onthis(Line 668-672).- Cluster click zoom increment is bounded cleanly (Line 738-745).
- Duplicate-node warning is fine as-is (Line 856-858).
Also applies to: 208-214, 276-280, 397-400, 432-435, 668-672, 688-690, 708-710, 738-745, 856-858
674-681: Potentially incorrect “new links” filter: current logic can drop legitimate linksThis filter keeps a link only if both its
sourceis unseen and itstargetis unseen:!sourceLinkSet.has(link.source) && !targetLinkSet.has(link.target)That can wrongly reject a new edge where (for example) the
sourcewas seen before but the(source,target)pair is new. If the intent is “dedupe by edge”, consider keying by(source,target)instead of separate sets.Suggested approach: dedupe by edge key
-const sourceLinkSet = new Set(self.data.links.map((l) => l.source)); -const targetLinkSet = new Set(self.data.links.map((l) => l.target)); +const linkKey = (l) => `${l.source}→${l.target}`; +const linkKeySet = new Set(self.data.links.map(linkKey)); -const links = data.links.filter( - (link) => - !sourceLinkSet.has(link.source) && !targetLinkSet.has(link.target), -); +const links = data.links.filter((link) => !linkKeySet.has(linkKey(link)));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/netjsongraph.render.js
🧰 Additional context used
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/js/netjsongraph.render.js
[warning] 1-1: Code style issues found. Prettier check failed for this file. Run 'openwisp-qa-format' or 'Prettier --write' to fix.
[error] 1-1: Prettier check failed! Hint: did you forget to run openwisp-qa-format?
| // Enable wheel zoom outside graph area | ||
| const dom = self.echarts.getDom && self.echarts.getDom(); | ||
| if (dom) { | ||
| dom.addEventListener( | ||
| "wheel", | ||
| (e) => { | ||
| const rect = dom.getBoundingClientRect(); | ||
| if ( | ||
| e.clientX < rect.left || | ||
| e.clientX > rect.right || | ||
| e.clientY < rect.top || | ||
| e.clientY > rect.bottom | ||
| ) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| const canvas = dom.querySelector("canvas"); | ||
| if (canvas) { | ||
| const r = canvas.getBoundingClientRect(); | ||
| canvas.dispatchEvent( | ||
| new WheelEvent("wheel", { | ||
| bubbles: true, | ||
| cancelable: true, | ||
| view: window, | ||
| clientX: r.left + r.width / 2, | ||
| clientY: r.top + r.height / 2, | ||
| deltaY: -e.deltaY, | ||
| deltaMode: e.deltaMode, | ||
| }), | ||
| ); | ||
| } | ||
| }, | ||
| {passive: false}, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd netjsongraph.render.jsRepository: openwisp/netjsongraph.js
Length of output: 97
🏁 Script executed:
wc -l src/js/netjsongraph.render.jsRepository: openwisp/netjsongraph.js
Length of output: 101
🏁 Script executed:
sed -n '441,475p' src/js/netjsongraph.render.jsRepository: openwisp/netjsongraph.js
Length of output: 1107
🏁 Script executed:
sed -n '400,550p' src/js/netjsongraph.render.js | head -n 150Repository: openwisp/netjsongraph.js
Length of output: 4494
🏁 Script executed:
rg "graphRender" src/js/netjsongraph.render.js -A 2 -B 2Repository: openwisp/netjsongraph.js
Length of output: 303
🏁 Script executed:
rg "graphRender\(" src/js/ -B 3 -A 3Repository: openwisp/netjsongraph.js
Length of output: 952
🏁 Script executed:
rg "this.utils.graphRender\|this.graphRender" src/js/ -B 5 -A 2Repository: openwisp/netjsongraph.js
Length of output: 50
🏁 Script executed:
rg "graphRender" src/js/netjsongraph.js -B 3 -A 3Repository: openwisp/netjsongraph.js
Length of output: 921
🏁 Script executed:
rg "echarts.init\|setOption\|SVGRenderer\|CanvasRenderer" src/js/ -B 2 -A 2Repository: openwisp/netjsongraph.js
Length of output: 50
🏁 Script executed:
rg "echarts\." src/js/netjsongraph.js -A 1 | head -n 50Repository: openwisp/netjsongraph.js
Length of output: 421
🏁 Script executed:
sed -n '1,100p' src/js/netjsongraph.jsRepository: openwisp/netjsongraph.js
Length of output: 2808
🏁 Script executed:
rg "addEventListener.*wheel" src/js/ -B 5 -A 10Repository: openwisp/netjsongraph.js
Length of output: 50
🏁 Script executed:
rg "\.render\(" src/js/ -B 2 -A 2Repository: openwisp/netjsongraph.js
Length of output: 1210
🏁 Script executed:
rg "_containerWheelHandler\|removeEventListener.*wheel" src/js/ -B 2 -A 2Repository: openwisp/netjsongraph.js
Length of output: 50
🏁 Script executed:
rg "removeEventListener" src/js/ -B 2 -A 2Repository: openwisp/netjsongraph.js
Length of output: 50
🏁 Script executed:
rg "setData\|clear\|init" src/js/netjsongraph.core.js | head -n 30Repository: openwisp/netjsongraph.js
Length of output: 50
Wheel forwarding handler has multiple functional risks (renderer mismatch, listener accumulation, and event fidelity)
Key concerns:
- Renderer mismatch: ECharts can be initialized with SVG renderer (
svgRenderconfig option), makingdom.querySelector("canvas")returnnulland silently failing to forward wheel events. - Listener accumulation:
graphRender()is called on re-renders (data updates, mode switches) without removing previous listeners, adding duplicate wheel handlers. - Event fidelity: The synthetic event omits
deltaX, modifier keys (ctrlKey,shiftKey,altKey,metaKey), and potentially other fields. ThedeltaY: -e.deltaYinversion is unexplained. - No double-zoom guard: If the wheel event originates on the actual ECharts surface, it can still bubble to
domand get forwarded again.
Use chart.getZr().dom instead of querying for canvas (works with both SVG and canvas renderers), store the handler for cleanup across re-renders, check if the event already targets the ZRender surface, and preserve all event fields.
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around lines 441 - 475, The wheel-forwarding
handler must be made robust: stop querying canvas via
dom.querySelector("canvas") and instead get the ZRender surface via
self.echarts.getZr().dom (works for canvas and SVG), preserve and forward all
relevant event fields (deltaX, deltaY, deltaMode, ctrlKey, shiftKey, altKey,
metaKey, button, etc.) without inverting sign, and avoid double-forwarding by
detecting if the original event already originated on the ZRender DOM (skip
forwarding when e.target === zrDom or zrDom.contains(e.target)). Also prevent
listener accumulation by storing the listener (e.g., this._wheelForwardHandler)
and removing any previous listener from dom before attaching a new one so it can
be cleaned up on re-renders; ensure you add the new listener with
{passive:false} as before.
2 participants
Enable scrolling to zoom in/out from anywhere on the graph container, not just over the graph nodes themselves, this matches the behavior of the geographic map mode
Checklist
Reference to Existing Issue
Closes #421
Description of Changes
Screen recording
BEFORE
netjsongraph.1.mp4
AFTER
netjsongraph.mp4