From 5fb6575ed5f8d75ab7a59ad754071ef25128f105 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 07:59:29 +0800 Subject: [PATCH 1/9] fix(sankey): derive effective node.pad clamp from layout geometry 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 #7832 --- src/traces/sankey/render.js | 26 ++++++++++++-- test/jasmine/tests/sankey_test.js | 59 +++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js index 83f8bbb2f98..b797d1e783b 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(i = 1; i < column.length; i++) { + var gap = column[i][0] - column[i - 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 diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index d82ecdf9296..146c22536e7 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -95,8 +95,67 @@ 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 = [{ + type: 'sankey', + layoutversion: 2, + domain: {x: [0, 1], y: [0, 1]}, + node: { + label: Array.from({length: 24}, function(_, i) { return 'n' + i; }), + pad: 30, + thickness: 10 + }, + link: { + source: Array.from({length: 23}, function(_, i) { return i; }), + target: Array.from({length: 23}, function(_, i) { return i + 1; }), + value: Array.from({length: 23}, function() { return 1; }) + } + }]; + + it('warns when the figure is too small for node.pad', function(done) { + var warnings = []; + spyOn(Lib, 'warn').and.callFake(function(msg) { + warnings.push(msg); + }); + + var gd = createGraphDiv('pad-warn-small', 300, 100); + Plotly.newPlot(gd, Lib.extendDeep([], padMock)) + .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(msg) { + warnings.push(msg); + }); + + var gd = createGraphDiv('pad-warn-large', 700, 900); + Plotly.newPlot(gd, Lib.extendDeep([], padMock)) + .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); From 46737ec8606b7d742ecc0f260a6e8f811026c2c5 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:11:22 +0800 Subject: [PATCH 2/9] fix(sankey): declare loop counter locally; add draftlog 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. --- draftlogs/7977_fix.md | 1 + src/traces/sankey/render.js | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) create mode 100644 draftlogs/7977_fix.md diff --git a/draftlogs/7977_fix.md b/draftlogs/7977_fix.md new file mode 100644 index 00000000000..56a42eef789 --- /dev/null +++ b/draftlogs/7977_fix.md @@ -0,0 +1 @@ + - Fix `node.pad` reduction warning so it derives the effective (post-clamp) padding from the laid-out node geometry instead of reading `sankey.nodePadding()`, which since @plotly/d3-sankey 0.12.x returns the configured value and made the warning never fire [[#7977](https://github.com/plotly/plotly.js/pull/7977)] diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js index b797d1e783b..87116e31d99 100644 --- a/src/traces/sankey/render.js +++ b/src/traces/sankey/render.js @@ -106,8 +106,8 @@ function sankeyModel(layout, d, traceIndex) { }); for(var key in columns) { var column = columns[key].sort(function(a, b) { return a[0] - b[0]; }); - for(i = 1; i < column.length; i++) { - var gap = column[i][0] - column[i - 1][1]; + for(var n = 1; n < column.length; n++) { + var gap = column[n][0] - column[n - 1][1]; if(gap < effectivePad) effectivePad = gap; } } From 7c6adaaecaeb23317a64d7e1e21d0e604c09e37a Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:21:21 +0800 Subject: [PATCH 3/9] test(sankey): drive pad-warning tests by explicit layout size 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. --- test/jasmine/tests/sankey_test.js | 48 +++++++++++++++++++------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 146c22536e7..3d9d3ef6877 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -105,21 +105,27 @@ describe('sankey tests', function () { // 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 = [{ - type: 'sankey', - layoutversion: 2, - domain: {x: [0, 1], y: [0, 1]}, - node: { - label: Array.from({length: 24}, function(_, i) { return 'n' + i; }), - pad: 30, - thickness: 10 - }, - link: { - source: Array.from({length: 23}, function(_, i) { return i; }), - target: Array.from({length: 23}, function(_, i) { return i + 1; }), - value: Array.from({length: 23}, function() { return 1; }) + var padMock = { + data: [{ + type: 'sankey', + layoutversion: 2, + node: { + label: Array.from({length: 24}, function(_, i) { return 'n' + i; }), + pad: 30, + thickness: 10 + }, + link: { + source: Array.from({length: 23}, function(_, i) { return i; }), + target: Array.from({length: 23}, function(_, i) { return i + 1; }), + value: Array.from({length: 23}, 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 = []; @@ -127,8 +133,11 @@ describe('sankey tests', function () { warnings.push(msg); }); - var gd = createGraphDiv('pad-warn-small', 300, 100); - Plotly.newPlot(gd, Lib.extendDeep([], padMock)) + 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 '); @@ -145,8 +154,11 @@ describe('sankey tests', function () { warnings.push(msg); }); - var gd = createGraphDiv('pad-warn-large', 700, 900); - Plotly.newPlot(gd, Lib.extendDeep([], padMock)) + var fig = Lib.extendDeep({}, padMock); + fig.layout.width = 700; + fig.layout.height = 900; + var gd = createGraphDiv(); + Plotly.newPlot(gd, fig) .then(function() { expect(warnings.length).toEqual(0); return Plotly.purge(gd); From c277ec7d6ff2d5700e100e9796d1c938ad4f9c0c Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:30:41 +0800 Subject: [PATCH 4/9] test(sankey): use star topology so clamping is actually exercised 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). --- test/jasmine/tests/sankey_test.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 3d9d3ef6877..5e251f40f1d 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -115,9 +115,12 @@ describe('sankey tests', function () { thickness: 10 }, link: { - source: Array.from({length: 23}, function(_, i) { return i; }), - target: Array.from({length: 23}, function(_, i) { return i + 1; }), - value: Array.from({length: 23}, function() { return 1; }) + // 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: { From 708b03dfe5bd8a63e5271e41145ede0c16d8b2a2 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:38:41 +0800 Subject: [PATCH 5/9] test(sankey): collect all Lib.warn arguments in the spy Lib.warn is variadic; pushing only the first argument made warnings[0][0] a single character instead of the message prefix. --- test/jasmine/tests/sankey_test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 5e251f40f1d..bbebaafba18 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -132,8 +132,9 @@ describe('sankey tests', function () { it('warns when the figure is too small for node.pad', function(done) { var warnings = []; - spyOn(Lib, 'warn').and.callFake(function(msg) { - warnings.push(msg); + 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); From 17b036fd8826fea885e238d45c222a6fdee1d8c7 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:47:16 +0800 Subject: [PATCH 6/9] test(sankey): size the non-clamped case to genuinely fit node.pad 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. --- test/jasmine/tests/sankey_test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index bbebaafba18..7e0decd907e 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -159,8 +159,8 @@ describe('sankey tests', function () { }); var fig = Lib.extendDeep({}, padMock); - fig.layout.width = 700; - fig.layout.height = 900; + fig.layout.width = 480; + fig.layout.height = 880; var gd = createGraphDiv(); Plotly.newPlot(gd, fig) .then(function() { From c6821f7a0e168708a5d7d8d85b124f2d1f20587a Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 08:57:16 +0800 Subject: [PATCH 7/9] test(sankey): raise the non-clamped case height to 1000 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). --- test/jasmine/tests/sankey_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 7e0decd907e..d1908f53008 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -160,7 +160,7 @@ describe('sankey tests', function () { var fig = Lib.extendDeep({}, padMock); fig.layout.width = 480; - fig.layout.height = 880; + fig.layout.height = 1000; var gd = createGraphDiv(); Plotly.newPlot(gd, fig) .then(function() { From 31eb47d433de28320d3c1f4b57bcc300ad15a113 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 09:10:42 +0800 Subject: [PATCH 8/9] test(sankey): drop stray layoutversion key from the mock --- test/jasmine/tests/sankey_test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index d1908f53008..69f56b394a1 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -108,7 +108,6 @@ describe('sankey tests', function () { var padMock = { data: [{ type: 'sankey', - layoutversion: 2, node: { label: Array.from({length: 24}, function(_, i) { return 'n' + i; }), pad: 30, From 43b55799272230407cb269ffd684b22272c98131 Mon Sep 17 00:00:00 2001 From: Shurong Cao Date: Mon, 24 Aug 2026 09:21:03 +0800 Subject: [PATCH 9/9] test(sankey): use a short sink column for the non-clamped case --- test/jasmine/tests/sankey_test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js index 69f56b394a1..6feb2eae349 100644 --- a/test/jasmine/tests/sankey_test.js +++ b/test/jasmine/tests/sankey_test.js @@ -153,13 +153,19 @@ describe('sankey tests', function () { it('does not warn when the figure fits node.pad', function(done) { var warnings = []; - spyOn(Lib, 'warn').and.callFake(function(msg) { - warnings.push(msg); + 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() {