Skip to content

Highlight matched nodes in the stack chart when searching#5935

Open
fatadel wants to merge 1 commit intofirefox-devtools:mainfrom
fatadel:highlight-node-1818
Open

Highlight matched nodes in the stack chart when searching#5935
fatadel wants to merge 1 commit intofirefox-devtools:mainfrom
fatadel:highlight-node-1818

Conversation

@fatadel
Copy link
Copy Markdown
Contributor

@fatadel fatadel commented Apr 2, 2026

Closes #1818.

When a search string is active, draw a BLUE_60 border around stack chart boxes whose function name matches. Ancestor call nodes in the path of a match also get a thinner border to highlight the full call node path.


Before reviewing the code changes, I would like you to have a look visually and tell me if that is what you want to see 🤓

This is the profile from the STR of the original issue with the changes of this PR.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.45%. Comparing base (46e3594) to head (c9c8f24).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5935   +/-   ##
=======================================
  Coverage   85.45%   85.45%           
=======================================
  Files         321      321           
  Lines       32068    32090   +22     
  Branches     8745     8835   +90     
=======================================
+ Hits        27403    27423   +20     
- Misses       4234     4236    +2     
  Partials      431      431           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fatadel fatadel requested review from canova and julienw April 2, 2026 16:01
@mstange
Copy link
Copy Markdown
Contributor

mstange commented Apr 2, 2026

Before reviewing the code changes, I would like you to have a look visually and tell me if that is what you want to see 🤓

Looks acceptable to me visually. Not super pretty but good enough :)

@canova
Copy link
Copy Markdown
Member

canova commented Apr 7, 2026

Hm, I'm surprised that the ancestors are also highlighted. I think it adds more visual noise than value. I think it might be better to highlight only the matching nodes, or to see a prototype of it. For example, we also only highlight the matching nodes in the call tree. @mstange wdyt?

For example, looking at chrome's devtools: when you search something, it shows only the matching nodes with their real color:

Screenshot 2026-04-07 at 12 42 49

@canova
Copy link
Copy Markdown
Member

canova commented Apr 7, 2026

Also visualization-wise, I was thinking about dimming the unmatched nodes (graying them out) like chrome does. But I'm not so sure if it's better than the current one yet.

@fatadel
Copy link
Copy Markdown
Contributor Author

fatadel commented Apr 7, 2026

@canova I've initially implemented this feature highlighting only the target nodes but then I've noticed this comment from @julienw and therefore re-implemented to highlight the whole path :) I am open to any solution. I like the one you've shared tbh.

@fatadel
Copy link
Copy Markdown
Contributor Author

fatadel commented Apr 7, 2026

Also visualization-wise, I was thinking about dimming the unmatched nodes (graying them out) like chrome does. But I'm not so sure if it's better than the current one yet.

Yeah it sounds more visually appealing to me too :)

@canova
Copy link
Copy Markdown
Member

canova commented Apr 7, 2026

@canova I've initially implemented this feature highlighting only the target nodes but then I've noticed this comment from @julienw and therefore re-implemented to highlight the whole path :) I am open to any solution. I like the one you've shared tbh.

Oh interesting. I don't think I agree with that comment tbh. Since we do the filtering already, only the roots with matching child will be included in the filtered stack chart. I don't really see the benefit of having these highlighted. Happy to hear counter arguments though.

@julienw
Copy link
Copy Markdown
Contributor

julienw commented Apr 7, 2026

@canova I've initially implemented this feature highlighting only the target nodes but then I've noticed this comment from @julienw and therefore re-implemented to highlight the whole path :) I am open to any solution. I like the one you've shared tbh.

Oh interesting. I don't think I agree with that comment tbh. Since we do the filtering already, only the roots with matching child will be included in the filtered stack chart. I don't really see the benefit of having these highlighted. Happy to hear counter arguments though.

I think that highlighting just the child works when the child's node is wide enough to be seen, but when it's super small to the point that it's barely visible or even not visible, then it's difficult to understand what's going on IMO. That's why I was mentioning this possible solution.
But if you're dimming the non-ancestor nodes, that might work too! It's worth trying

@fatadel
Copy link
Copy Markdown
Contributor Author

fatadel commented Apr 7, 2026

Sounds good, let me re-implement with dimming then.

@fatadel fatadel force-pushed the highlight-node-1818 branch 2 times, most recently from 6d0f011 to 7c6932b Compare April 9, 2026 11:23
@fatadel
Copy link
Copy Markdown
Contributor Author

fatadel commented Apr 9, 2026

Done ✅
Looks like this. Please review :)

Screen.Recording.2026-04-09.at.13.25.57.mov

@fatadel fatadel force-pushed the highlight-node-1818 branch from 7c6932b to 37fd131 Compare April 9, 2026 12:44
  When a search string is active, reduce the opacity of stack chart
  nodes whose function name does not match. This makes matched nodes
  visually stand out while non-matching nodes fade into the background.
@fatadel fatadel force-pushed the highlight-node-1818 branch from 37fd131 to c9c8f24 Compare April 9, 2026 12:49
@fatadel
Copy link
Copy Markdown
Contributor Author

fatadel commented Apr 9, 2026

The video above is slightly invalid because it was recorded with dimming opacity of 0.2 and currently it's set to 0.5 (after discussing with @canova)

@nchevobbe
Copy link
Copy Markdown
Member

In the #5935 (comment) screencast, the dimmed text is definitely not accessible (I'm seeing some light grey text on white background with 1.4:1 contrast ratio)
If the text is going to be displayed, I'd argue it should be visible
I see that for those dimmed nodes, you're tweaking the opacity; in the past I found it quite hard to then work out proper contrast ratio when custom opacity are applied, I'd rather have specific dimmed background/foreground colors. Also, this would make it easier to handle High Contrast Mode too, where we can use https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/system-color#accentcolor:~:text=GrayText to convey the "dimmed" aspect. Finally, it would be nice to see how this work in dark mode, to make sure the contrasts are good too :)

Ideally, for accessibility, any state change shouldn't be conveyed only by a change in colors, so maybe here we should flip things: keep the node as they are now, and make the one that matches the search very visible, e.g. with a 2px outline, or a specific icon ?

@nchevobbe
Copy link
Copy Markdown
Member

Ideally, for accessibility, any state change shouldn't be conveyed only by a change in colors, so maybe here we should flip things: keep the node as they are now, and make the one that matches the search very visible, e.g. with a 2px outline, or a specific icon ?

or maybe we remove the background color from the "dimmed" nodes and only have a border on them. They would appear quite differently from the matching ones and it would be less tricky to find the right colors. We could still tweak the dimmed node text color so they feel less important

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.

When searching in the stack chart, the matched nodes should be highlighted

5 participants