fix(funnel/waterfall): respect textinfo token order by Abineshabee · Pull Request #7752 · plotly/plotly.js

Abineshabee · 2026-04-13T17:07:49Z

What this fixes

go.Funnel with textinfo="percent initial+value" was displaying
value before percent (e.g. "700 70.0%") regardless of the order
specified by the user.

Root cause

In src/traces/bar/plot.js, the calcTextinfo function pushed
funnel tokens (value, percent initial/previous/total) and
waterfall tokens (initial, delta, final) in a hardcoded
sequence, ignoring the order of tokens in the textinfo string.

Fix

Replaced hardcoded hasFlag() checks with a loop over parts[]
(the user-specified token order) for both funnel and waterfall traces.

Before

"percent initial+value" → "1000 100.0%" ❌

After

"percent initial+value" → "100.0% 1000" ✅

Notes

Backward compatible — existing behavior unchanged when token
order was already correct
label and text tokens still use fixed ordering and are
not modified in this PR
Fix applies to both funnel and waterfall traces as requested

emilykl · 2026-04-13T17:31:31Z

Hi @Abineshabee ! Thanks for the PR. This looks like it's on the right track; I'll take a review pass through the code. Let us know if you would like assistance with the failing test.

Could you please update the PR description to include screenshots or a codepen demonstrating the fix?

emilykl

Left a few comments, I'll take another look once you're ready @Abineshabee !

Abineshabee · 2026-04-13T20:34:06Z

@emilykl I've addressed both points:

Switched to for (var part in parts) loop syntax
Included label and text flags inside the ordered loop
so all tokens now respect user-specified order consistently.
Also normalized parts with .map(p => p.trim()) to handle
any whitespace edge cases. Ready for another look!

emilykl · 2026-04-13T20:49:43Z

Thanks @Abineshabee for the quick turnaround! Before I take another look, can you update the PR description to add screenshots of the fix in action, and also fix the failing test? I'm happy to assist with either, just let me know.

Abineshabee · 2026-04-13T20:55:54Z

@emilykl Updated the test with proper safety checks. Ready for another look!

Abineshabee · 2026-04-13T21:26:06Z

@emilykl The only remaining CI failure is isosurface_test.js @gl should display hover labels
which is a pre-existing flaky GL test unrelated to this PR.
It fails inconsistently across runs (different hover values each time)
and is not caused by my changes to src/traces/bar/plot.js.

All funnel and waterfall tests pass. Ready for review !

emilykl · 2026-04-15T17:47:27Z

Great! Taking another look.

emilykl · 2026-04-15T18:06:05Z

+    var parts = textinfo.split('+').map(function(p) {
+        return p.trim();
+    });


I double-checked, and we already validate flaglist items during the coerce step, so this trim() is not needed.

Suggested change

var parts = textinfo.split('+').map(function(p) {

return p.trim();

});

var parts = textinfo.split('+');

emilykl · 2026-04-15T21:39:30Z

+    for(var i in parts) {
+        var part = parts[i];
+
+        if(part === 'label' && hasFlag('label')) {


I don't think we need the hasFlag('...') checks at all, right? if(part === '...') should be sufficient, unless I'm missing something.

emilykl · 2026-04-15T21:49:00Z

+    if(isWaterfall) {
+        delta = +cdi.rawS || cdi.s;
+        final = cdi.v;
+        initial = final - delta;


I don't see any reason to calculate these separately at the top; might as well just put each calculation inside its corresponding if/else case, I think that's clearer.

emilykl · 2026-04-15T22:26:43Z

+            expect(textEls.length).toBeGreaterThan(0);
+
+            for(var i = 0; i < textEls.length; i++) {
+                var txt = textEls[i].textContent;
+                if(!txt || txt.length === 0) continue;
+
+                var percentIndex = txt.indexOf('%');
+                expect(percentIndex).toBeGreaterThan(-1);
+
+                var firstNumberIndex = txt.search(/\d/);
+                expect(firstNumberIndex).toBeGreaterThan(-1);
+
+                // percent sign must appear AFTER first digit (e.g. "70%")
+                // but BEFORE the value number that follows
+                expect(percentIndex).toBeGreaterThan(firstNumberIndex);
+
+                // value number must exist AFTER percent
+                var afterPercent = txt.slice(percentIndex + 1);
+                expect(afterPercent.match(/\d+/)).not.toBeNull();
+            }


@Abineshabee Instead of this approach, let's just assert the exact text content we expect, I think that will be much clearer.

i.e. something like

var textContent = Array.from(textEls).map((el) => el.textContent); expect(textContent).toBe([ "100%1000", "70%700", "40%400" ]);

emilykl · 2026-04-15T22:58:14Z

@Abineshabee This looks great! See my comments for additional small changes.

The new logic in calcTextInfo() I think is much easier to understand than the old logic, so that's a nice bonus to this fix.

There's one more chunk of work needed on this PR: Updating other trace types to respect token order in textinfo, so that textinfo works the same way for all traces.

Luckily there's only two other functions which need to be updated:

formatSliceLabel() in src/traces/pie/plot.js
formatSliceLabel() in src/traces/sunburst/plot.js

Both of those functions should be refactored similarly to calcTextInfo(), so that they also respect token order. Let me know if you run into any issues with that step, I'm happy to assist. Make sure to add a Jasmine test for sunburst and pie as well.

Thanks for working on this fix!

emilykl · 2026-04-15T22:58:56Z

Also: You're correct that the one failing test is unrelated to these changes. That test is sometimes flaky. If it's still failing after your next commits I'll look into it.

…urst traces

Abineshabee · 2026-04-16T15:06:34Z

emilykl · 2026-04-16T15:12:30Z

Thanks @Abineshabee for the quick turnaround! I'll take another look.

emilykl · 2026-04-16T15:13:07Z

+    for(var part in parts) {
+        part = parts[part];


Put this back to how it was previously, it's confusing to reuse the same variable name. Likewise for the other loops.

(I realize I suggested for(var part in parts) originally; that's my bad, I forgot how for loops work in js!)

Suggested change

for(var part in parts) {

part = parts[part];

for(var i in parts) {

part = parts[i];

emilykl · 2026-04-16T15:15:30Z

+    if(isFunnel) {
+        for(var p = 0; p < parts.length; p++) {
+            var f = parts[p];
+            if(f === 'percent initial' || f === 'percent previous' || f === 'percent total') nPercent++;
+        }
    }


I prefer this version from your previous iteration as well, it's easier to understand.

Suggested change

if(isFunnel) {

for(var p = 0; p < parts.length; p++) {

var f = parts[p];

if(f === 'percent initial' || f === 'percent previous' || f === 'percent total') nPercent++;

}

}

if(isFunnel) {

if(hasFlag('percent initial')) nPercent++;

if(hasFlag('percent previous')) nPercent++;

if(hasFlag('percent total')) nPercent++;

hasMultiplePercents = nPercent > 1;

}

emilykl · 2026-04-16T16:06:07Z

+            labels: ['Root', 'A', 'B'],
+            parents: ['', 'Root', 'Root'],
+            values: [0, 10, 20],
+            textinfo: 'value+label'


Add percent root to flaglist to ensure the root is handled correctly (should not appear in root).

Make sure to update expected values.

Suggested change

textinfo: 'value+label'

textinfo: 'percent root+value+label'

emilykl · 2026-04-16T16:07:56Z

+            expect(textContent[0]).toBe('100%1000');
+            expect(textContent[1]).toBe('70%700');
+            expect(textContent[2]).toBe('40%400');


Also check the length of textContent.

Same goes for the other new tests as well.

Suggested change

expect(textContent[0]).toBe('100%1000');

expect(textContent[1]).toBe('70%700');

expect(textContent[2]).toBe('40%400');

expect(textContent.length).toBe(3);

expect(textContent[0]).toBe('100%1000');

expect(textContent[1]).toBe('70%700');

expect(textContent[2]).toBe('40%400');

Abineshabee · 2026-04-16T16:14:22Z

Thank for your review and suggestions , now I am working on updates

emilykl · 2026-04-16T16:18:12Z

@Abineshabee I've finished my review pass! Just a few remaining items:

Some small tweaks to code (see comments)
Make sure to use the same for loop style in all 3 files where you have added one. I like the version below (for var i in parts) which you had used previously, but I don't care too much as long as it's consistent and doesn't reuse variable names.

for(var i in parts) {
        var part = parts[i];

There is a failing Jasmine test (should be able to restyle *textinfo*) which I think is expected due to this change. The test will need to be updated to expect the values in the new order.
There are some failing image tests in the test-baselines CI job which I believe are expected as well based on these changes. You will need to download the updated images from the CircleCI "Artifacts" tab (under test_images) and commit them to the repo in the test/image/baselines/ folder. Double-check that the changes are expected.

test(funnel): add test for textinfo token order

4c418a0

Abineshabee mentioned this pull request Apr 13, 2026

[BUG]: go.Funnel textinfo="percent initial+value" shows value before percent. #7751

Open

emilykl reviewed Apr 13, 2026

View reviewed changes

Comment thread src/traces/bar/plot.js Outdated

emilykl reviewed Apr 13, 2026

View reviewed changes

Comment thread src/traces/bar/plot.js Outdated

emilykl requested changes Apr 13, 2026

View reviewed changes

fix: correct loop structure and restore hasFlag checks

c05bb22

test: ensure robust validation of token order

4696313

test: use DOM text elements instead of calcdata tx

4f5d538

camdecoster assigned emilykl Apr 15, 2026

emilykl reviewed Apr 15, 2026

View reviewed changes

fix: respect textinfo token order for funnel, waterfall, pie and sunb…

66add11

…urst traces

emilykl reviewed Apr 16, 2026

View reviewed changes

Uh oh!

Conversation

Abineshabee commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this fixes

Root cause

Fix

Before

After

Notes

Uh oh!

emilykl commented Apr 13, 2026

Uh oh!

Uh oh!

Uh oh!

emilykl left a comment

Choose a reason for hiding this comment

Uh oh!

Abineshabee commented Apr 13, 2026

Uh oh!

emilykl commented Apr 13, 2026

Uh oh!

Abineshabee commented Apr 13, 2026

Uh oh!

Abineshabee commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilykl commented Apr 15, 2026

Uh oh!

emilykl Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilykl commented Apr 15, 2026

Uh oh!

emilykl commented Apr 15, 2026

Uh oh!

Abineshabee commented Apr 16, 2026

Uh oh!

emilykl commented Apr 16, 2026

Uh oh!

emilykl Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

emilykl Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Abineshabee commented Apr 16, 2026

Uh oh!

emilykl commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Abineshabee commented Apr 13, 2026 •

edited

Loading

Abineshabee commented Apr 13, 2026 •

edited

Loading

emilykl Apr 15, 2026 •

edited

Loading