Closed Bug 1081245 Opened 5 years ago Closed 4 years ago

Make the call tree text select and copy-able.

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

defect

Tracking

(firefox46 fixed)

VERIFIED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: azakai, Assigned: peregrino, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css][polish-backlog][difficulty=easy])

Attachments

(1 file, 5 obsolete files)

It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.
Geckoprofiler has the ability to actually share the result pages as links so that others can view the same data. That will not obviously replace the need to copy-paste, but would also be very useful to have.
Nick, what do you think about this?
This is to make the call-tree select + copy-able?

Could we just add `user-select: text` in the CSS?
I don't know enough CSS to tell myself, but that sounds right.
This is a good first bug for a new contributor.

We need to make the contents of the profiler's call tree view[0] select-able and copy-able. This should be possible by adding the user-select[1] CSS property to the appropriate rule in the profiler's stylesheet[2].

Learn more about hacking on devtools here: https://wiki.mozilla.org/DevTools/Hacking

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/utils/tree-view.js#143
[1] https://developer.mozilla.org/en-US/docs/Web/CSS/user-select
[2] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/profiler.inc.css#278
Mentor: nfitzgerald
Whiteboard: [good first bug][lang=css]
Hi I am interested in helping with this bug, does a patch need to be written first for it to be assigned?
Thanks 
Jesal
Assignee: nobody → kjjcrm1
Status: NEW → ASSIGNED
Hi, Thanks for the assignment, I will read into this and try and help.

Thanks

Jesal
(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> This is a good first bug for a new contributor.
> 
> We need to make the contents of the profiler's call tree view[0] select-able
> and copy-able. This should be possible by adding the user-select[1] CSS
> property to the appropriate rule in the profiler's stylesheet[2].
> 
> Learn more about hacking on devtools here:
> https://wiki.mozilla.org/DevTools/Hacking
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/
> utils/tree-view.js#143
> [1] https://developer.mozilla.org/en-US/docs/Web/CSS/user-select
> [2]
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> profiler.inc.css#278

Hi Nick, 
I have had a look at the materials suggested. 
Where can I see this bug in action? How do I locate Firefox profiler? This so that I can code on my build and test it out?
Also I understand that the syntax needs to be added to the last-child and header items within the css?
But are do we also need to use user-select: all? as that refers to the highest ancestor in the tree? or are we defining each element with the css?

Thanks.

Jesal
You can open the profiler via the menu: Tools > Web Developer > Performance

When you record a profile of a page executing JS code, you get a call tree summary of the code. http://news.google.com is doing stuff a lot of the time, and tends to be a good test case if you just need activity. You can try it out there.

I don't think you need to do any last-child stuff with the selector, just match anything that has the .call-tree-cell or .call-tree-header classes.

I haven't actually done too much with user-select; I recommend trying both "text" and "all" and seeing which behavior feels more natural.
(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> You can open the profiler via the menu: Tools > Web Developer > Performance
> 
> When you record a profile of a page executing JS code, you get a call tree
> summary of the code. http://news.google.com is doing stuff a lot of the
> time, and tends to be a good test case if you just need activity. You can
> try it out there.
> 
> I don't think you need to do any last-child stuff with the selector, just
> match anything that has the .call-tree-cell or .call-tree-header classes.
> 
> I haven't actually done too much with user-select; I recommend trying both
> "text" and "all" and seeing which behavior feels more natural.

Thanks for the advice Nick, I will work on this and let you know how I get on. Hopefully all going well I will upload a patch within the next few days.

Jesal
Summary: Option to copy or export high-level profiler results → Make the call tree text select and copy-able.
Hi Nick. 

I have tried editing the css file. However I don't understand what needs to be selectable. As in the performance tab, where all the graphs are and the fields for total time and cost etc. These elements are already selectable? Which elements do you want to be copied. Is it where the function tree is ? this needs to be copied from the root and not the json data?

Thanks Jesal
Yes, the tree of functions.
Any updates here? This would be very useful to have.
Hi Nick, I followed the Mozilla DevRoom during the Fosdem and, as your friends suggested, I'd like to try to fix a simple bug. This seems to be a good one. 
Is it still assigned to Jesal or I can try to work on it?
Yeah, no word or patches from Jesal, feel free to work on this. Will wait to assign it until you have a work-in-progress patch attached.

See the backlog for context and relevant parts of the code base :)
Assignee: kjjcrm1 → nobody
Status: ASSIGNED → NEW
I added the `-moz-user-select: text` rule to the function cell, but the text is still not selectable. The selector seems ok, I used the browser toolbox to check it. Screenshot here: http://cl.ly/image/351g3J222W1d/call-tree-cell%20selection.png
Flags: needinfo?(nfitzgerald)
Assignee: nobody → tommaso.visconti
Status: NEW → ASSIGNED
(In reply to Tommaso Visconti [:tommyblue] from comment #16)
> Created attachment 8560066 [details] [diff] [review]
> added -moz-user-select, without effects
> 
> I added the `-moz-user-select: text` rule to the function cell, but the text
> is still not selectable. The selector seems ok, I used the browser toolbox
> to check it. Screenshot here:
> http://cl.ly/image/351g3J222W1d/call-tree-cell%20selection.png

Strange, I'd think that would work. Unfortunately, my CSS-fu is not great. I'm needinfo'ing some people who hopefully understand this stuff better than me.
Flags: needinfo?(vporof)
Flags: needinfo?(ntim007)
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(bgrinstead)
My guess is that the child labels don't use the "normal" text frame but the xul textbox frame, and that's because we're setting the value attribute. A simple workaround is to set the textcontent instead of the value at [0].
I'll check later today to be sure.

[0] : https://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/utils/tree-view.js#215
Flags: needinfo?(ntim007)
(In reply to Tim Nguyen [:ntim] from comment #18)
> ...
> I'll check later today to be sure.

So, this solution didn't work. I'm guessing XUL doesn't allow text selection. A solution would be to convert the layout to xhtml. I tried to use a <span> element with html namespace, but that didn't work.

Boris, do you know if there's a way to make xul text selectable ?
Flags: needinfo?(bzbarsky)
> I'm guessing XUL doesn't allow text selection

Which sort of XUL?  There are all sorts of different kinds of "XUL" things, some of which are more amenable to selection than others.  A standalone testcase with the sort of "XUL" involved here would probably enable me to answer the question...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #20)
> > I'm guessing XUL doesn't allow text selection
> 
> Which sort of XUL?  There are all sorts of different kinds of "XUL" things,
> some of which are more amenable to selection than others.  A standalone
> testcase with the sort of "XUL" involved here would probably enable me to
> answer the question...

XUL labels in this case. I'll attach a testcase later.
For a <xul:label>, using a child text node instead of a "value" attribute and styling it with "-moz-user-select: text" should work.
(In reply to Boris Zbarsky [:bz] from comment #22)
> For a <xul:label>, using a child text node instead of a "value" attribute
> and styling it with "-moz-user-select: text" should work.

Weird, suggested this earlier (comment 18),  and it didn't seem to work. Maybe that doesn't work in chrome documents ? Or some JS is conflicting here ?
(In reply to Tim Nguyen [:ntim] from comment #23)
> (In reply to Boris Zbarsky [:bz] from comment #22)
> > For a <xul:label>, using a child text node instead of a "value" attribute
> > and styling it with "-moz-user-select: text" should work.
> 
> Weird, suggested this earlier (comment 18),  and it didn't seem to work.
> Maybe that doesn't work in chrome documents ? Or some JS is conflicting here
> ?

Nevermind, that works perfectly, thanks !
So what needs to be done here :

- In [0], change mousedown to click (mousedown event is conflicting with text selection)
- Do what I mentioned in comment 18
- Set -moz-user-select: text; as well (what your patch does).

[0] : https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/AbstractTreeItem.jsm#358

Victor, is there an issue to change from mousedown to click ?
Nah. Mousedown was used because it felt faster.
Flags: needinfo?(vporof)
Looks like this got sorted out
Flags: needinfo?(bgrinstead)
Attached patch textSelection.patch (obsolete) — Splinter Review
Thanks to the comments (thank you guys!) I found how to select the text in the "Function" column. I also changed the background color of the selected text or it's impossible to recognize it when the row is selected (the background color was the same).

Nick, is this the expected result?

Probably people expect to select the whole table with CTRL+a or using the mouse (as it's possible to select an HTML table), I can do it replicating the edit for all the cells?
Attachment #8560066 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)
(In reply to Tommaso Visconti [:tommyblue] from comment #28)
> Probably people expect to select the whole table with CTRL+a or using the
> mouse (as it's possible to select an HTML table), I can do it replicating
> the edit for all the cells?

Let's keep this bug simple and land it quickly. We can file follow up bugs for things like ctrl+a :)
Flags: needinfo?(nfitzgerald)
Comment on attachment 8560924 [details] [diff] [review]
textSelection.patch

Review of attachment 8560924 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Flag me for review (in the attachment details) when you combine all of your changes into one patch.

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +354,5 @@
>  
>      let targetNode = this._targetNode = this._displaySelf(document, arrowNode);
>      targetNode.style.MozUserFocus = "normal";
>  
> +    targetNode.addEventListener("click", this._onClick);

So the issue is that onClick preventsDefault therefore killing our text selection? I think we can keep the snappy behavior that binding to mousedown gives if we simply stop calling preventDefault in onClick. Can you try this?

::: browser/themes/shared/devtools/profiler.inc.css
@@ +304,5 @@
>  .call-tree-item:focus .call-tree-cell {
>    -moz-border-end-color: var(--focus-cell-border-color);
>  }
>  
> +.call-tree-item .call-tree-cell[type=function] label {

Just double checking: this makes every cell select-able, not just the function cell, right? The function cell just needs special casing because of the label?

@@ +309,2 @@
>    cursor: text;
>    -moz-user-select: text;

This stuff is part of the changes as well, right? Make sure you are uploading all of the changes at once -- perhaps you need to squash patches together in mq?

@@ +310,5 @@
>    -moz-user-select: text;
>  }
>  
> +.call-tree-item .call-tree-cell[type=function] label::-moz-selection {
> +  background-color: var(--theme-highlight-orange);

Perfect use of the theme variables :)
Attachment #8560924 - Flags: feedback+
Hey Tommaso, have you had any time to work on this further?
Flags: needinfo?(tommaso.visconti)
Flags: needinfo?(tommaso.visconti)
Hey Jordan, I'm not sure what Nick asked me to do. It's already a single patch..
I tried to contact him on IRC some months ago without success.
Now I'm on a different field, but if I can help, please explain me what you think I should do :)
Flags: needinfo?(jsantell)
Which part of comment 30 are you not sure about?
Hi :fitzgen, when you say "when you combine all of your changes into one patch", what do you mean? The patch I submitted isn't already one patch?
(In reply to Tommaso Visconti [:tommyblue] from comment #34)
> Hi :fitzgen, when you say "when you combine all of your changes into one
> patch", what do you mean? The patch I submitted isn't already one patch?

Maybe I was wrong, but it seemed to me that the patch that you uploaded was based on top of some mystery patch that was not uploaded. There were things that were not marked as additions in your patch that did not exist in mozilla-central or fx-team and appeared to be additions for the purpose of fixing this bug. That led me to believe that you split the work into two or more patches (perhaps accidentally?) and only attached one of the patches.
I think fitzgen answered my ni?
Flags: needinfo?(jsantell)
Hi :fitzgen, if that happened, I don't know why :)
I made the patch using instruction from Mozilla docs and using the code from fx-team repo. The patch seems ok to me, but I'm for sure not an expert...
This would still be very useful for Emscripten. Any updates here?

If it's tricky to get the data via copy-paste, maybe Emscripten should just process the json info from the profiler? Is there documentation on the structure of that, and of how we would compute the high-level data (what is visible in the profiler) from it?
Alon, what kind of data are you looking to get? CSV-style text of each call tree row? Saving the profile will get you a giant JSON object, and requires a handful of manipulation and expansion to get useful data out of it (we have utilities for this, but some overhead of learning how to use it)
Flags: needinfo?(azakai)
Just how much CPU time is spent in each method should be enough. Then Emscripten can do PGO optimization, by knowing which methods are cold and can be optimized less aggressively.
Flags: needinfo?(azakai)
Tommaso, are you still interested in working on this?
Flags: needinfo?(tommaso.visconti)
Sorry Jordan,
I'm really busy at the moment
Flags: needinfo?(tommaso.visconti)
Assignee: tommaso.visconti → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][polish-backlog]
Whiteboard: [good first bug][lang=css][polish-backlog] → [good first bug][lang=css][polish-backlog][difficulty=easy]
Priority: -- → P3
Attached patch Updated patch (obsolete) — Splinter Review
I tried to update the patch as the code seems to have moved. Let me know if this is what's needed and/or any comments you may have.
Attachment #8560924 - Attachment is obsolete: true
Attachment #8681082 - Flags: feedback?(nfitzgerald)
Comment on attachment 8681082 [details] [diff] [review]
Updated patch

Review of attachment 8681082 [details] [diff] [review]:
-----------------------------------------------------------------

This makes the text copy-able and select-able, but when I copy-paste the text it all comes out on a single line.

I guess we could land this and format the text nicely as a follow up...
Attachment #8681082 - Flags: feedback?(nfitzgerald) → feedback+
yeah, I saw that everything came out as a single line, but didn't know how the output was expected. Is there a way to add line breaks and tabulations for the selected text?
(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #45)
> yeah, I saw that everything came out as a single line, but didn't know how
> the output was expected. Is there a way to add line breaks and tabulations
> for the selected text?

Unfortunately, I don't know. Maybe someone else does.
Alon, how do you expect the selected text to be copied? With the patch I updated the result is that all the columns and rows can be selected and copied, but the text comes as a single line without tabulations between column values nor line breaks between rows.
Flags: needinfo?(azakai)
As long as there is a way to figure out what is what, it should be fine - I can write a little parser. Just needs to be possible to know where indentation increases and decreases, I guess.
Flags: needinfo?(azakai)
Aded tabulations to indicate columns and line breaks for rows. Also, nested rows have spaces in the front of the description to indicate that.
Attachment #8685819 - Flags: review?(nfitzgerald)
Assignee: nobody → colmeiro
Status: NEW → ASSIGNED
Comment on attachment 8685819 [details] [diff] [review]
Make the call tree text select and copy-able

Review of attachment 8685819 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Hernan!

::: devtools/client/performance/modules/widgets/tree-view.js
@@ +283,5 @@
> +    lastDescrtiption.textContent = lastDescrtiption.textContent + "\n";
> +
> +    // Add spaces as frameLevel indicators in case the row is selected and
> +    // copied. These spaces won't be displayed in the cell content.
> +    let firstDescrtiption = cell.querySelector('description:first-of-type');

Nit: typos all through this new block, "descrtiption" should be "description"
Attachment #8685819 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #50)
> Nit: typos all through this new block, "descrtiption" should be "description"

Heh, my bad. Was a bit late in the night and didn't see it ;) I'll fix it when I get back home. Is the format ok?
Attachment #8685819 - Attachment is obsolete: true
Attachment #8686343 - Flags: review+
Hi, I am new to fixing bugs and I would like to work on the bug please.
(In reply to Pas from comment #55)
> Hi, I am new to fixing bugs and I would like to work on the bug please.

I didn't abandon this one, it was backed out because I broke some tests. Was planning to fix them over this weekend :)
Attached patch Updated testsSplinter Review
Updated the broken tests that caused the backout. Re-requesting review as the chenges to the tests where quite a few.
Attachment #8681082 - Attachment is obsolete: true
Attachment #8686343 - Attachment is obsolete: true
Attachment #8696861 - Flags: review?(nfitzgerald)
Comment on attachment 8696861 [details] [diff] [review]
Updated tests

Review of attachment 8696861 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8696861 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/mozilla-central/rev/6b31012ac6d2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED, FIXED -> VERIFIED

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

STR:
It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.

ER:
1. It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

2. There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

3. The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.

AR:

1. Verified. Ctrl+A and Ctrl+C working.
2. Not clear.
3. Not clear.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.