Highlight matched nodes in the stack chart when searching#5935
Highlight matched nodes in the stack chart when searching#5935fatadel wants to merge 1 commit intofirefox-devtools:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5935 +/- ##
==========================================
- Coverage 85.37% 85.31% -0.06%
==========================================
Files 322 322
Lines 32069 32129 +60
Branches 8814 8836 +22
==========================================
+ Hits 27378 27412 +34
- Misses 4260 4286 +26
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Looks acceptable to me visually. Not super pretty but good enough :) |
|
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:
|
|
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. |
|
@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. |
Yeah it sounds more visually appealing to me too :) |
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. |
|
Sounds good, let me re-implement with dimming then. |
6d0f011 to
7c6932b
Compare
|
Done ✅ Screen.Recording.2026-04-09.at.13.25.57.mov |
37fd131 to
c9c8f24
Compare
|
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) |
|
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) 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 |
c9c8f24 to
91c3350
Compare
|
Thanks everyone for your feedback! Could you please have another look? Now I use a neutral color to make nodes look dimmed and I change the font colors too, keeping the nodes accessible. Should work fine in both light and dark modes. This is close to the way Chrome does (@canova's screenshot above). Here is the link. |
|
@fatadel I'm still seeing the dimmed nodes failing AA contrast (using https://vispero.com/lp/color-contrast-checker/): 4.4:1 in light mode, and 3:1 in dark mode (see screenshots below)
|
|
@nchevobbe That's strange tbh. I did the same measurements and I got the following. Are you sure you're checking correctly in the light mode? I will tweak the font colors in the dark mode.
|
91c3350 to
800a532
Compare
|
Now the dark mode should also pass the contrast ratio tests. |
Ah, must have picked a subpixel area which didn't hold the correct color. Looks fine given your screenshot then :)
Nice, thanks! Would you have a link / screenshot for it? |
|
It looks fine on Windows light and dark High Contrast Mode |
Sure, here you are.
|
Sounds good 👍🏻 Did you already add it, or should I? |
|
that's great, thanks!
I added an item in the list of tasks there :) |
800a532 to
dcde905
Compare
There was a problem hiding this comment.
Thanks! The UI-wise it looks good to me. I believe the performance-wise we can improve it, but the suggestion I made below is a much larger change. And I didn't notice a lot of perf issues using the deploy preview. So I'm okay with landing it this way and filing an issue for the improvements.
| for ( | ||
| let callNodeIndex = 0; | ||
| callNodeIndex < callNodeTable.length; | ||
| callNodeIndex++ | ||
| ) { |
There was a problem hiding this comment.
Hmm, I thought a lot about this Set and the for loop here. I'm a bit scared of its performance impact and think it could be improved.
The possible issues I can see:
- There is no caching, we are rebuilding this set on every draw.
- It adds another loop, and we are doing this check before here with
funcMatchesSearch:profiler/src/profile-logic/profile-data.ts
Lines 1812 to 1816 in d5eb4f2
funcMatchesSearchhere is a BitSet of all the funcTable. And we mostly care about the funcTable. Currently it uses callNodeIndex, but there is no reason not to check funcIndex below as we already have it. - The set can get pretty big on large profiles.
- If flame graph needs it, we will need to move it out so we don't do it twice.
But this is a lot more architectural changes than this small change...
funcMatchesSearch is an local variable, which needs to be extracted and memoized. And then we can use that extracted BitSet in 2 places without computing it twice. And this would allow us store it and have a selector like getSearchMatchedFuncs. Then instead of passing searchStringsRegExp here, we would pass that bitset to this component. And the component only does checkBit(searchMatchedFuncs, currentFuncIndex) instead.
Since this is a much bigger change, I'm okay with landing this one. But let's at least file a bug about the improvements we can make.
dcde905 to
181fe4b
Compare







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.