From b792c9c85a49f1b2510fb7cc48782bfe9418c79f Mon Sep 17 00:00:00 2001 From: Justin Donaldson Date: Sat, 7 Mar 2026 16:40:17 -0800 Subject: [PATCH 1/3] Fix sankey nodes being clipped at bottom edge with user-positioned nodes When nodes are positioned near y=1.0 via trace.node.x/y, they can extend past the bottom of the plot area. Additionally, resolveCollisionsTopToBottom pushes overlapping nodes downward without checking if the last node exceeds the available height. This fix: 1. Clamps yCenter when positioning nodes so they stay within [0, height] 2. Shifts columns upward after collision resolution if the bottom node extends past the plot height Fixes bottom clipping in sankey diagrams with arrangement="snap" and user-specified node positions. --- src/traces/sankey/render.js | 19 +++++++++++-- .../mocks/sankey_x_y_bottom_clipping.json | 23 ++++++++++++++++ test/jasmine/tests/sankey_test.js | 27 +++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 test/image/mocks/sankey_x_y_bottom_clipping.json diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js index a07d18a9c9e..b1e2ff3f54b 100644 --- a/src/traces/sankey/render.js +++ b/src/traces/sankey/render.js @@ -200,6 +200,17 @@ function sankeyModel(layout, d, traceIndex) { } y = node.y1 + nodePad; } + // If the last node extends past the bottom, shift the whole column up + if(n > 0) { + var lastNode = nodes[n - 1]; + if(lastNode.y1 > height) { + dy = lastNode.y1 - height; + for(i = 0; i < n; ++i) { + nodes[i].y0 -= dy; + nodes[i].y1 -= dy; + } + } + } }); } @@ -251,8 +262,12 @@ function sankeyModel(layout, d, traceIndex) { graph.nodes[i].x1 = pos[0] + nodeThickness / 2; var nodeHeight = graph.nodes[i].y1 - graph.nodes[i].y0; - graph.nodes[i].y0 = pos[1] - nodeHeight / 2; - graph.nodes[i].y1 = pos[1] + nodeHeight / 2; + var yCenter = pos[1]; + // Clamp so node doesn't extend past bottom or top + yCenter = Math.max(yCenter, nodeHeight / 2); + yCenter = Math.min(yCenter, height - nodeHeight / 2); + graph.nodes[i].y0 = yCenter - nodeHeight / 2; + graph.nodes[i].y1 = yCenter + nodeHeight / 2; } } if(trace.arrangement === 'snap') { diff --git a/test/image/mocks/sankey_x_y_bottom_clipping.json b/test/image/mocks/sankey_x_y_bottom_clipping.json new file mode 100644 index 00000000000..bbc2f58bd74 --- /dev/null +++ b/test/image/mocks/sankey_x_y_bottom_clipping.json @@ -0,0 +1,23 @@ +{ + "data": [ + { + "type": "sankey", + "arrangement": "snap", + "node": { + "label": ["A", "B", "C", "D"], + "x": [0.1, 0.1, 0.5, 0.9], + "y": [0.5, 0.95, 0.95, 0.95] + }, + "link": { + "source": [0, 0, 1, 2], + "target": [1, 2, 3, 3], + "value": [5, 3, 5, 3] + } + } + ], + "layout": { + "title": { "text": "Sankey with bottom-edge nodes" }, + "width": 600, + "height": 400 + } +} diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 76fa0e9f27c..1dece998400 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -578,6 +578,33 @@ describe('sankey tests', function() { .then(done, done.fail); }); + it('prevents nodes from being clipped at the bottom edge', function(done) { + var mockBottom = require('../../image/mocks/sankey_x_y_bottom_clipping.json'); + var mockCopy = Lib.extendDeep({}, mockBottom); + + Plotly.newPlot(gd, mockCopy) + .then(function() { + var nodeRects = document.querySelectorAll('.sankey-node .node-rect'); + var sankeyLayer = document.querySelector('.sankey'); + var sankeyRect = sankeyLayer.getBoundingClientRect(); + + for(var i = 0; i < nodeRects.length; i++) { + var rect = nodeRects[i].getBoundingClientRect(); + // Every node's bottom edge must be within the sankey area + expect(rect.bottom).not.toBeGreaterThan( + sankeyRect.bottom + 1, // 1px tolerance + 'node ' + i + ' extends past the bottom edge' + ); + // Every node's top edge must be within the sankey area + expect(rect.top).not.toBeLessThan( + sankeyRect.top - 1, // 1px tolerance + 'node ' + i + ' extends past the top edge' + ); + } + }) + .then(done, done.fail); + }); + it('resets each subplot to its initial view (ie. x, y groups) via modebar button', function(done) { var mockCopy = Lib.extendDeep({}, require('../../image/mocks/sankey_subplots_circular')); From 42da017ce2fd2ce8a46dea4d841b60fe0e6231c6 Mon Sep 17 00:00:00 2001 From: Justin Donaldson Date: Sat, 7 Mar 2026 16:41:20 -0800 Subject: [PATCH 2/3] Add draftlog for #7725 --- draftlogs/7725_fix.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 draftlogs/7725_fix.md diff --git a/draftlogs/7725_fix.md b/draftlogs/7725_fix.md new file mode 100644 index 00000000000..45b670d124e --- /dev/null +++ b/draftlogs/7725_fix.md @@ -0,0 +1 @@ + - Fix sankey nodes being clipped at the bottom edge when using user-positioned nodes (`node.x`/`node.y`) near `y=1.0` or with `arrangement="snap"` collision resolution [[#7725](https://github.com/plotly/plotly.js/pull/7725)] From 9e4dc615570f8f1559c11fab7bed3c873809ad1d Mon Sep 17 00:00:00 2001 From: Justin Donaldson Date: Thu, 6 Aug 2026 20:07:29 -0700 Subject: [PATCH 3/3] Resolve sankey bottom-edge collisions with a bottom-to-top pass Replaces the whole-column shift added earlier in this branch with the missing counterpart to resolveCollisionsTopToBottom. resolveCollisionsTopToBottom only ever moves nodes down, so a column of user-positioned nodes clustered near y=1 gets walked off the bottom of the plot area. The bundled @plotly/d3-sankey resolves this in its own resolveCollisions by following the downward pass with a bottom-bounded upward one (src/sankey.js:292-299); render.js declares a function with the same name as the dep's downward helper but implements only that half. Add resolveCollisionsBottomToTop, bounded by `height`, and call it after the downward pass. Because it only moves a node when node.y1 exceeds the running bound, nodes that already fit stay put and the overflow propagates upward no further than it must -- unlike shifting the whole column by the full overflow, which moves the top node even when it had no need to move and can push a tight column off the top edge instead. Also switch the explicit-position clamp to Lib.constrain. Its swapped-bounds branch pins a node taller than the plot area to the top edge (y0 = 0), whereas the previous Math.max-then-Math.min ordering let the min win in that case and produced y0 = height - nodeHeight, i.e. clipping at the top. Tests: replace the single mock-driven test with two, one per code path (arrangement 'fixed' for the centring clamp, 'snap' for the cascade), and measure node rects against _fullLayout._size rather than the '.sankey' layer's bounding box. Node rects are descendants of that layer and an SVG group's box is the union of its children, so the previous assertions grew to contain any overflow and could not fail. Drops the image mock, which needed a baseline PNG the jasmine test did not use. --- src/traces/sankey/render.js | 43 ++++++---- .../mocks/sankey_x_y_bottom_clipping.json | 23 ----- test/jasmine/tests/sankey_test.js | 84 ++++++++++++++----- 3 files changed, 93 insertions(+), 57 deletions(-) delete mode 100644 test/image/mocks/sankey_x_y_bottom_clipping.json diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js index b1e2ff3f54b..cee6d7ec362 100644 --- a/src/traces/sankey/render.js +++ b/src/traces/sankey/render.js @@ -200,16 +200,28 @@ function sankeyModel(layout, d, traceIndex) { } y = node.y1 + nodePad; } - // If the last node extends past the bottom, shift the whole column up - if(n > 0) { - var lastNode = nodes[n - 1]; - if(lastNode.y1 > height) { - dy = lastNode.y1 - height; - for(i = 0; i < n; ++i) { - nodes[i].y0 -= dy; - nodes[i].y1 -= dy; - } - } + }); + } + + // Push any overlapping nodes up, bounded by the bottom of the plot area. + // Counterpart to resolveCollisionsTopToBottom: that pass only ever moves + // nodes down, so a column clustered near the bottom edge gets walked off + // the plot area. Mirrors the like-named helper in @plotly/d3-sankey, which + // bounds its result on both edges. Nodes that already fit do not move. + function resolveCollisionsBottomToTop(columns) { + columns.forEach(function(nodes) { + var node; + var dy; + var y = height; + var i; + nodes.sort(function(a, b) { + return a.y0 - b.y0; + }); + for(i = nodes.length - 1; i >= 0; --i) { + node = nodes[i]; + dy = node.y1 - y; + if(dy > 1e-6) node.y0 -= dy, node.y1 -= dy; + y = node.y0 - nodePad; } }); } @@ -262,10 +274,12 @@ function sankeyModel(layout, d, traceIndex) { graph.nodes[i].x1 = pos[0] + nodeThickness / 2; var nodeHeight = graph.nodes[i].y1 - graph.nodes[i].y0; - var yCenter = pos[1]; - // Clamp so node doesn't extend past bottom or top - yCenter = Math.max(yCenter, nodeHeight / 2); - yCenter = Math.min(yCenter, height - nodeHeight / 2); + // Keep the node inside the plot area: trace.node.y positions the + // node's centre, so y near 1 would put half the node below the + // bottom edge. When the node is taller than the plot area the + // bounds invert and Lib.constrain pins it to the top edge, which + // beats letting it hang off the top. + var yCenter = Lib.constrain(pos[1], nodeHeight / 2, height - nodeHeight / 2); graph.nodes[i].y0 = yCenter - nodeHeight / 2; graph.nodes[i].y1 = yCenter + nodeHeight / 2; } @@ -274,6 +288,7 @@ function sankeyModel(layout, d, traceIndex) { nodes = graph.nodes; var columns = snapToColumns(nodes); resolveCollisionsTopToBottom(columns); + resolveCollisionsBottomToTop(columns); } // Update links sankey.update(graph); diff --git a/test/image/mocks/sankey_x_y_bottom_clipping.json b/test/image/mocks/sankey_x_y_bottom_clipping.json deleted file mode 100644 index bbc2f58bd74..00000000000 --- a/test/image/mocks/sankey_x_y_bottom_clipping.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "data": [ - { - "type": "sankey", - "arrangement": "snap", - "node": { - "label": ["A", "B", "C", "D"], - "x": [0.1, 0.1, 0.5, 0.9], - "y": [0.5, 0.95, 0.95, 0.95] - }, - "link": { - "source": [0, 0, 1, 2], - "target": [1, 2, 3, 3], - "value": [5, 3, 5, 3] - } - } - ], - "layout": { - "title": { "text": "Sankey with bottom-edge nodes" }, - "width": 600, - "height": 400 - } -} diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 1dece998400..a75e50e2055 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -578,29 +578,73 @@ describe('sankey tests', function() { .then(done, done.fail); }); - it('prevents nodes from being clipped at the bottom edge', function(done) { - var mockBottom = require('../../image/mocks/sankey_x_y_bottom_clipping.json'); - var mockCopy = Lib.extendDeep({}, mockBottom); + // Measure node rects against the plot area from _fullLayout._size, which + // is what plot.js passes to the renderer as its `height`. Do NOT measure + // against the '.sankey' layer's bounding box: node rects are descendants + // of it, and an SVG group's box is the union of its children, so it grows + // to contain any overflow and the assertion can never fail. + function expectNodesWithinPlotArea(gd, msg) { + var gs = gd._fullLayout._size; + var gdTop = gd.getBoundingClientRect().top; + var plotTop = gdTop + gs.t; + var plotBottom = plotTop + gs.h; + var nodeRects = gd.querySelectorAll('.sankey-node .node-rect'); + + expect(nodeRects.length).toBeGreaterThan(0, msg + ': found no node rects'); + + for(var i = 0; i < nodeRects.length; i++) { + var rect = nodeRects[i].getBoundingClientRect(); + expect(rect.bottom).toBeLessThan(plotBottom + 1, + msg + ': node ' + i + ' extends ' + (rect.bottom - plotBottom).toFixed(1) + + 'px past the bottom of the plot area'); + expect(rect.top).toBeGreaterThan(plotTop - 1, + msg + ': node ' + i + ' extends ' + (plotTop - rect.top).toFixed(1) + + 'px past the top of the plot area'); + } + } - Plotly.newPlot(gd, mockCopy) + it('keeps an explicitly positioned node inside the plot area', function(done) { + // arrangement 'fixed' skips collision resolution, so this covers the + // node.y centring path on its own. Node B is requested at y = 0.98; + // unclamped it overflows by nodeHeight/2 - (1 - 0.98) * height. + Plotly.newPlot(gd, [{ + type: 'sankey', + arrangement: 'fixed', + node: { + label: ['A', 'B', 'C'], + x: [0.1, 0.1, 0.9], + y: [0.3, 0.98, 0.5], + pad: 10 + }, + link: {source: [0, 1], target: [2, 2], value: [10, 10]} + }], {width: 600, height: 300, margin: {l: 10, r: 10, t: 10, b: 10}}) .then(function() { - var nodeRects = document.querySelectorAll('.sankey-node .node-rect'); - var sankeyLayer = document.querySelector('.sankey'); - var sankeyRect = sankeyLayer.getBoundingClientRect(); - - for(var i = 0; i < nodeRects.length; i++) { - var rect = nodeRects[i].getBoundingClientRect(); - // Every node's bottom edge must be within the sankey area - expect(rect.bottom).not.toBeGreaterThan( - sankeyRect.bottom + 1, // 1px tolerance - 'node ' + i + ' extends past the bottom edge' - ); - // Every node's top edge must be within the sankey area - expect(rect.top).not.toBeLessThan( - sankeyRect.top - 1, // 1px tolerance - 'node ' + i + ' extends past the top edge' - ); + expectNodesWithinPlotArea(gd, 'fixed arrangement'); + }) + .then(done, done.fail); + }); + + it('keeps a snapped column inside the plot area when collisions cascade', function(done) { + // Every y is <= 0.92, so the centring clamp alone would not prevent + // clipping here: B, C and D share a column and overlap, and resolving + // those collisions downward walks the column off the bottom edge. + Plotly.newPlot(gd, [{ + type: 'sankey', + arrangement: 'snap', + node: { + label: ['A', 'B', 'C', 'D', 'E'], + x: [0.1, 0.5, 0.5, 0.5, 0.9], + y: [0.5, 0.80, 0.86, 0.92, 0.5], + pad: 10 + }, + link: { + source: [0, 0, 0, 1, 2, 3], + target: [1, 2, 3, 4, 4, 4], + value: [8, 8, 8, 8, 8, 8] } + }], {width: 600, height: 400, margin: {l: 10, r: 10, t: 10, b: 10}}) + .then(function() { + expectNodesWithinPlotArea(gd, 'snap arrangement'); }) .then(done, done.fail); });