Skip to content

Issue #1818 - When searching in the stack chart, the matched nodes should be highlighted#2957

Closed
js636f wants to merge 4 commits intofirefox-devtools:mainfrom
js636f:issue1818
Closed

Issue #1818 - When searching in the stack chart, the matched nodes should be highlighted#2957
js636f wants to merge 4 commits intofirefox-devtools:mainfrom
js636f:issue1818

Conversation

@js636f
Copy link
Copy Markdown

@js636f js636f commented Oct 15, 2020

The initial proposal was to use a border to highlight the matched nodes in the stack chart. But as it turned out the nodes are too tiny and I couldn't find suitable border size. So I just draw another semitransparent rectangle at the top of the matched node. I don't think this is a production-ready solution (I didn't check performance, the appearance of the highlighting can and should be improved), but now we may discuss it and I will improve it.

Fixes #1818

┆Issue is synchronized with this Jira Task

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 15, 2020

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (64cdd7e) to head (aebecae).
⚠️ Report is 3339 commits behind head on main.

Files with missing lines Patch % Lines
src/components/stack-chart/Canvas.js 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2957   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         282      282           
  Lines       25333    25342    +9     
  Branches     6817     6821    +4     
=======================================
+ Hits        22435    22443    +8     
- Misses       2696     2697    +1     
  Partials      202      202           

☔ 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.

Copy link
Copy Markdown
Member

@canova canova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @js636f! I'm not sure about the current solution.

Black text on top of a dark blue rectangle doesn't look very accessible. Even I (as a person who has a fairly okay eyesight) have trouble reading the text of the highlighted node :) We can maybe find another way to highlight the matched node.
Also we don't use save and restore in the codebase yet, so it would be good to look t the performance impact of both these values and testing each frame with regexes.

@julienw @gregtatum what do you think about this highlight style?
Example profile

@julienw
Copy link
Copy Markdown
Contributor

julienw commented Oct 19, 2020

@julienw @gregtatum what do you think about this highlight style?

Definitely not accessible

Can we please have a look at the solution with a border please? :-)

@js636f
Copy link
Copy Markdown
Author

js636f commented Oct 20, 2020

@canova, @julienw Thanks for the feedback! OK, I'm working on this.

@js636f
Copy link
Copy Markdown
Author

js636f commented Oct 20, 2020

@canova, @julienw should it look like this? Or, perhaps I don't understand something and I have to add a frame only to the big nodes, but not to a tiny nodes (this is still something like 'minimal viable product', but the frame is bigger than these tiny nodes).

@julienw julienw self-requested a review October 27, 2020 12:10
@julienw
Copy link
Copy Markdown
Contributor

julienw commented Oct 27, 2020

Sorry it took so long to answer you.

TBH I like it this way :-) even for tiny nodes.

I think there may be a bug, see this example: I'm looking for "MessageLoop", and it looks like that only half of the matches have their border.

But otherwise I think we can move forward this way. What do you think @canova?

Copy link
Copy Markdown
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(removing the r request)

intH
);

if (searchStringsRegExp && searchStringsRegExp.test(text)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the searchStringsRegExp is shared, it needs to be "reset" when using it. I think this is the source of the bug I found. You can do this:

// Since the regexp is reused and likely global, let's make sure we reset it.
regExp.lastIndex = 0;

);

if (searchStringsRegExp && searchStringsRegExp.test(text)) {
ctx.strokeStyle = 'green';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the color could be somewhat softer. We can try the mozilla blue BLUE_60 for example. You can import it like this:

import { BLUE_60 } from 'photon-colors'

and use it directly.

@julienw
Copy link
Copy Markdown
Contributor

julienw commented Nov 9, 2020

Hey @js636f, is that still something you'd like to move forward to the finish line? That's totally OK if you don't, but then please tell us :-) Thanks for your feedback.

@js636f
Copy link
Copy Markdown
Author

js636f commented Nov 9, 2020

Hey, @julienw ! I'm a little busy, but anyway I hope I'll be able to finish this task. But if it's urgent and someone would like to continue work on it and use my code as a base - It's OK for me.

@julienw
Copy link
Copy Markdown
Contributor

julienw commented Dec 3, 2020

gently ping @js636f :)

@js636f
Copy link
Copy Markdown
Author

js636f commented Dec 3, 2020

Hello, @julienw I'm still hope to finish the task. If I will not be able to finish it in a week from now please give it to someone else. Thanks!

@julienw
Copy link
Copy Markdown
Contributor

julienw commented Jul 11, 2025

I still think it would be a valuable addition. But we need to highlight the full call node paths and not just the call nodes. So this needs some more work to be able to do that.

I'm keeping the PR open for now.

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

4 participants