diff --git a/draftlogs/7978_fix.md b/draftlogs/7978_fix.md new file mode 100644 index 00000000000..f707c91a066 --- /dev/null +++ b/draftlogs/7978_fix.md @@ -0,0 +1 @@ + - Fix `sankey` nodes positioned near or below the bottom of the plot area being clipped: explicitly-positioned node rectangles are now clamped to the plot bounds, and the `snap` collision pass gained a bottom-bounded upward sweep so columns can no longer walk past the plot edge [[#7978](https://github.com/plotly/plotly.js/pull/7978)] diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js index 83f8bbb2f98..45601719291 100644 --- a/src/traces/sankey/render.js +++ b/src/traces/sankey/render.js @@ -90,8 +90,30 @@ function sankeyModel(layout, d, traceIndex) { var graph = sankey(); - if(sankey.nodePadding() < nodePad) { - Lib.warn('node.pad was reduced to ', sankey.nodePadding(), ' to fit within the figure.'); + // Derive the effective (post-clamp) node padding from the laid-out node + // geometry instead of reading it back through `sankey.nodePadding()`. + // In @plotly/d3-sankey@0.7.x that getter returned the clamped value after + // the layout ran, but since 0.12.x it returns the user-configured value + // (upstream split `dy` from `py`), so a getter-based check would never + // fire. Measuring the smallest vertical gap between consecutive nodes in + // any one column is version-independent. See #7832. + var effectivePad = nodePad; + var columns = {}; + graph.nodes.forEach(function(node) { + var col = Math.round(node.x0); + if(!columns[col]) columns[col] = []; + columns[col].push([node.y0, node.y1]); + }); + for(var key in columns) { + var column = columns[key].sort(function(a, b) { return a[0] - b[0]; }); + for(var n = 1; n < column.length; n++) { + var gap = column[n][0] - column[n - 1][1]; + if(gap < effectivePad) effectivePad = gap; + } + } + + if(effectivePad < nodePad) { + Lib.warn('node.pad was reduced to ', effectivePad, ' to fit within the figure.'); } // Counters for nested loops @@ -220,6 +242,34 @@ function sankeyModel(layout, d, traceIndex) { }); } + // Push any nodes that overflow the bottom edge back up so the whole + // column stays inside the plot area. Counterpart to + // resolveCollisionsTopToBottom: with `arrangement: "snap"` a downward + // cascade can walk the last node(s) straight past `height`, even when + // there is empty space above to absorb the correction. + function resolveCollisionsBottomToTop(columns) { + columns.forEach(function(nodes) { + var node; + var dy; + var y = height; + var n = nodes.length; + var i; + nodes.sort(function(a, b) { + return b.y0 - a.y0; + }); + for(i = 0; i < n; ++i) { + node = nodes[i]; + if(node.y1 <= y) { + // No overflow at the bottom edge + } else { + dy = (node.y1 - y); + if(dy > 1e-6) node.y0 -= dy, node.y1 -= dy; + } + y = node.y0 - nodePad; + } + }); + } + // Group nodes into columns based on their x position function snapToColumns(nodes) { // Sort nodes by x position @@ -268,14 +318,27 @@ 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 y0 = pos[1] - nodeHeight / 2; + var y1 = pos[1] + nodeHeight / 2; + // Keep the node fully inside the plot area: a node centered + // exactly on the top/bottom edge (y = 0 / y = 1) would + // otherwise render half outside it. + if(y0 < 0) { + y0 = 0; + y1 = nodeHeight; + } else if(y1 > height) { + y1 = height; + y0 = height - nodeHeight; + } + graph.nodes[i].y0 = y0; + graph.nodes[i].y1 = y1; } } if(trace.arrangement === 'snap') { nodes = graph.nodes; var columns = snapToColumns(nodes); resolveCollisionsTopToBottom(columns); + resolveCollisionsBottomToTop(columns); } // Update links sankey.update(graph); diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index d82ecdf9296..e822cae0471 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -95,8 +95,88 @@ describe('sankey tests', function () { }); }); + describe('sankey global defaults', function () { it('should not coerce trace opacity', function () { + + + describe('node.pad reduction warning', function() { + // The warning must be driven by the effective (post-clamp) padding, + // not by reading `sankey.nodePadding()` back, which since + // @plotly/d3-sankey@0.12.x returns the configured value instead of + // the clamped one - see #7832. + var padMock = { + data: [{ + type: 'sankey', + node: { + label: Array.from({length: 24}, function(_, i) { return 'n' + i; }), + pad: 30, + thickness: 10 + }, + link: { + // star topology: one source feeding 24 sinks puts all 24 + // sink nodes in a single column, so a small figure must + // clamp the padding + source: Array.from({length: 24}, function() { return 0; }), + target: Array.from({length: 24}, function(_, i) { return i + 1; }), + value: Array.from({length: 24}, function() { return 1; }) + } + }], + layout: { + width: 500, + height: 500, + margin: {l: 10, r: 10, t: 10, b: 10} + } + }; + + it('warns when the figure is too small for node.pad', function(done) { + var warnings = []; + spyOn(Lib, 'warn').and.callFake(function() { + // collect all arguments, as Lib.warn is variadic + warnings.push(Array.prototype.slice.call(arguments)); + }); + + var fig = Lib.extendDeep({}, padMock); + fig.layout.width = 200; + fig.layout.height = 100; + var gd = createGraphDiv(); + Plotly.newPlot(gd, fig) + .then(function() { + expect(warnings.length).toEqual(1); + expect(warnings[0][0]).toBe('node.pad was reduced to '); + expect(warnings[0][1]).toBeLessThan(30); + return Plotly.purge(gd); + }) + .then(function() { destroyGraphDiv(gd); }) + .then(done, done.fail); + }); + + it('does not warn when the figure fits node.pad', function(done) { + var warnings = []; + spyOn(Lib, 'warn').and.callFake(function() { + // collect all arguments, as Lib.warn is variadic + warnings.push(Array.prototype.slice.call(arguments)); + }); + + var fig = Lib.extendDeep({}, padMock); + fig.layout.width = 480; + fig.layout.height = 1000; + // keep the sink column short enough that pad=30 always fits + fig.data[0].node.label = Array.from({length: 8}, function(_, i) { return 'n' + i; }); + fig.data[0].link.source = Array.from({length: 7}, function() { return 0; }); + fig.data[0].link.target = Array.from({length: 7}, function(_, i) { return i + 1; }); + fig.data[0].link.value = Array.from({length: 7}, function() { return 1; }); + var gd = createGraphDiv(); + Plotly.newPlot(gd, fig) + .then(function() { + expect(warnings.length).toEqual(0); + return Plotly.purge(gd); + }) + .then(function() { destroyGraphDiv(gd); }) + .then(done, done.fail); + }); + }); + (fix(sankey): derive effective node.pad clamp from layout geometry) var gd = Lib.extendDeep({}, mock); supplyAllDefaults(gd); @@ -2279,5 +2359,88 @@ describe('sankey layout generators', function () { expect(circularLinks.length).toEqual(89, 'right number of circular links'); }); }); + + describe('keeps explicitly-positioned / snapped nodes inside the plot area (plotly.js #7946)', function() { + function plotArea(gd) { + var fl = gd._fullLayout; + var rect = gd.getBoundingClientRect(); + return { + left: rect.left + fl.margin.l, + top: rect.top + fl.margin.t, + right: rect.left + fl.width - fl.margin.r, + bottom: rect.top + fl.height - fl.margin.b + }; + } + + function assertNodesInside(gd, msg) { + var pa = plotArea(gd); + var eps = 1.5; + d3SelectAll('.sankey .node-rect').each(function() { + var r = this.getBoundingClientRect(); + expect(r.top).toBeGreaterThan(pa.top - eps); + expect(r.bottom).toBeLessThan(pa.bottom + eps); + expect(r.left).toBeGreaterThan(pa.left - eps); + expect(r.right).toBeLessThan(pa.right + eps); + }); + } + + it('does not clip an explicitly-positioned node near the bottom edge', function(done) { + var gd = createGraphDiv(); + var fig = { + data: [{ + type: 'sankey', + arrangement: 'fixed', + node: { + label: ['A', 'B at y=0.98', '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] + } + }], + layout: { + width: 600, + height: 300, + margin: {l: 10, r: 10, t: 10, b: 10} + } + }; + Plotly.newPlot(gd, fig) + .then(function() { assertNodesInside(gd, 'repro1'); }) + .then(done, done.fail); + }); + + it('does not cascade snapped nodes past the bottom edge', function(done) { + var gd = createGraphDiv(); + var fig = { + data: [{ + 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] + } + }], + layout: { + width: 600, + height: 400, + margin: {l: 10, r: 10, t: 10, b: 10} + } + }; + Plotly.newPlot(gd, fig) + .then(function() { assertNodesInside(gd, 'repro2'); }) + .then(done, done.fail); + }); + }); }); });