Skip to content

fix(sankey): keep explicitly-positioned and snapped nodes inside the plot area - #7978

Open
CAOShurong wants to merge 14 commits into
plotly:masterfrom
CAOShurong:codex/sankey-7946-clip-nodes-in-bounds
Open

fix(sankey): keep explicitly-positioned and snapped nodes inside the plot area#7978
CAOShurong wants to merge 14 commits into
plotly:masterfrom
CAOShurong:codex/sankey-7946-clip-nodes-in-bounds

Conversation

@CAOShurong

Copy link
Copy Markdown
Contributor

Summary

Fixes #7946 — Sankey nodes are clipped at the bottom edge when positioned with node.x / node.y, or when a snap collision cascade walks a column past height.

There are two independent causes, both addressed:

  1. Explicit positioning centers the node on y * height without bounds checking, so a node requested at y = 1 renders half outside the plot area (plotly.js/src/traces/sankey/render.js, "Force node position"). The fix clamps the node rect to [0, height] when it would overflow either edge.
  2. snap collision cascade only ever pushes overlapping nodes downward, so a clustered column gets walked straight off the bottom edge even when there is empty space above to absorb the correction. The fix adds a bottom-bounded upward pass (resolveCollisionsBottomToTop) run after the existing top-to-bottom pass.

Test plan

  • Added a jasmine regression test (test/jasmine/tests/sankey_test.js) reproducing both cases from the issue:
    • explicit position with arrangement: "fixed" (node at y = 0.98) — asserts the node rect stays inside the plot area;
    • arrangement: "snap" with an overlapping column — asserts no node rect overflows the bottom edge.
  • Local run of the sankey suite: the two new specs pass. (The drag-interaction specs fail under headless Chrome in this environment regardless of this change — pre-existing, unrelated to the fix; full CI on the PR validates the rest.)
  • A standalone geometry harness exercising the exact algorithm passes both repros (pre-fix overflow of 61.9px on Repro 1 → 0 after the fix; Repro 2's previously-invisible node now renders at the bottom edge).

Notes

  • Behavior for non-edge positions and arrangement: "snap" columns with room above is unchanged.
  • No visual-baseline images change (positions only shift for the previously-clipped edge cases).

The node.pad warning read the clamped padding back through
sankey.nodePadding(). In @plotly/d3-sankey@0.7.x that getter returns
the post-layout clamped value, but since 0.12.x (upstream split of dy/py)
it returns the configured value, so the comparison is never true and the
warning never fires after a dependency upgrade.

Measure the smallest vertical gap between consecutive nodes in any one
column instead; this works regardless of the installed d3-sankey
version. Regression tests cover both the clamped and non-clamped paths.

Refs plotly#7832
The inner loop reused the file-scope `i` before its var declaration
line, tripping biome's noInvalidUseBeforeDeclaration, and the PR was
missing its changelog draftlog entry.
The karma viewport size does not determine the plot size, so the
small-figure case rendered at default dimensions and never clamped.
Set explicit layout width/height (and small margins) instead, and use
the standard createGraphDiv() helper.
A 24-node chain places one node per column, so no column ever holds two
nodes and the padding can never clamp regardless of figure size. A
one-to-many star puts all 24 sinks in one column (verified against the
real d3-sankey layout: effective min gap 2.32 at 480x80).
Lib.warn is variadic; pushing only the first argument made
warnings[0][0] a single character instead of the message prefix.
At 24 sink nodes a 900-high figure still clamps the padding (effective
gap 26.7 < 30); 880 was verified against the real layout to leave
enough room. Also fix the leftover single-arg spy in this test.
With 24 sink nodes the padding only stops clamping once the plot area
is ~1000px high (verified against the real layout: gap 27.3 at 900,
exactly 30 at 1000).
CAOShurong added a commit to CAOShurong/plotly.js that referenced this pull request Aug 24, 2026
Avoids reusing the outer-scope loop counter in the effective-pad
measurement loop.
…plot area (plotly.js plotly#7946)

Explicit node positions centered on y=0/y=1 (and the snap collision
cascade) could push node rects past the top/bottom edge, clipping them
outside the plot area (plotly.js plotly#7946).

- Clamp explicitly-positioned nodes to [0, height] in the Force-node-position
  block so a node centered on an edge stays fully inside.
- Add resolveCollisionsBottomToTop, a bottom-bounded upward pass run after the
  existing top-to-bottom pass for arrangement:snap, so an overlapping column is
  absorbed upward instead of being walked off the bottom edge.

Adds a jasmine regression test covering both repro cases.
@CAOShurong
CAOShurong force-pushed the codex/sankey-7946-clip-nodes-in-bounds branch from 33c1eb3 to d1ccdd2 Compare August 24, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Sankey nodes are clipped at the bottom edge when positioned with node.x / node.y

1 participant