"Show allocation triggers" button doesn't look very button-y or link-y

RESOLVED FIXED in Firefox 54

Status

P2
normal
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: fitzgen, Assigned: keremkat, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
Firefox 54
good-first-bug

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [good first bug][mentor=vp][lang=js])

Attachments

(4 attachments, 2 obsolete attachments)

And so a lot of people probably don't understand how to use it well.

It is in the sidebar in this screenshot: https://bug1233551.bmoattachments.org/attachment.cgi?id=8711077

(If you can't find it, that is more evidence in support of styling this thing better!)
Triaging. Filter on LULUGUBRIOUSUS.
Priority: -- → P2
Whiteboard: [good first bug][mentor=vp][lang=js]

Comment 2

3 years ago
Nick, I'd like to work on this bug. Could you please assign it to me?

Btw, is there are sequence of actions (e.g., a webpage) that'll help me reproduce a similar performance recording. I tried a couple and wasn't able to get the "Show allocation triggers" button in the sidebar for a non-incremental GC row.
Flags: needinfo?(nfitzgerald)
Octane should trigger some: https://octane-benchmark.googlecode.com/svn/latest/index.html

You can also try something like this on a local html page:

window.allocations = [];
function loop() {
  for (let i = 0; i < 10000; i++)
    window.allocations.push({});
  setTimeout(loop, 1);
}
setTimeout(loop, 1);
Assignee: nobody → maurya1985
Flags: needinfo?(nfitzgerald)

Updated

3 years ago
Version: unspecified → 46 Branch

Comment 4

3 years ago
Nick, currently the element is a label. What do you think about replacing the label with a button (rather than a link)?

Comment 5

3 years ago
Posted image prototype.png
something like this ...
Maurya, that looks pretty awesome to me! Jordan knows this code much better than I do, so redirecting mentorship his way.
Mentor: jsantell

Comment 7

3 years ago
Jordan, I'm not so experienced in JS. Can you help me with this?
I replaced a label with a button. But, the flow of execution no longer hits the click event handler.

Here's the old code commented out and the new code right below it: https://pastebin.mozilla.org/8861715
Flags: needinfo?(jsantell)

Comment 8

3 years ago
Jordan, someone on chat suggested styling the label as a button. So, I'm considering trying this out.
Hm not sure why the click event on the button wouldn't be happening anymore (handled in devtools/client/performance/modules/widgets/marker-details.js _onClick), unless the button element swallows the event..

Trying out your code, it does work for me; clicking on the button selects a new range of the timeline and switches to the allocations view, and looks good! The active state when pressing down on the button, though, looks like it's native OSX button styling (bright blue background, white text), but could probably use some of the styles from other buttons in this tool.

Does that help? Are you not seeing the button, and it's not switching views when clicking, in a profile from octane?
Flags: needinfo?(jsantell)

Comment 10

3 years ago
I tried it out again and without any luck. When I click on the "Show allocation triggers" button, the marker details sidebar becomes blank white entirely. It doesn't switch to the allocation view.

I checked the js code using the Browser Toolbox just to be sure that the correct and latest js code is being loaded by the browser. And, there's no problem here. So, I'm puzzled how it's working for you, but not for me!
Flags: needinfo?(jsantell)

Comment 11

3 years ago
I clobbered and rebuilt just to be sure, but that doesn't change the behavior.

Comment 12

3 years ago
I'm able to make a label appear like a button using the -moz-appearance CSS property. This might be a better approach since the button is swallowing the event. However, not sure why the results would vary from that of yours.
Flags: needinfo?(jsantell)

Comment 13

3 years ago
Attachment #8727159 - Flags: review?(jsantell)
Sorry it's taken so long to respond, Maurya! I tried out your patch, and gets us 90% there -- attach is a picture of the text color looking a bit too light when in the dark theme, and the picture on the right is when I click the button on the light theme -- so it seems like it just needs a few more tweaks to fix these issues. Thank you!

For more CSS changes, Victor Porof (vporof@mozilla.com) is probably a better bet to look over the code.
Attachment #8727159 - Flags: review?(jsantell)
Flags: needinfo?(vporof)

Comment 15

3 years ago
Jordan, I'll fix the issue in the dark theme.

But, is the display on the light theme also a problem? Or, is that only for comparison that you captured the screenshot?
Flags: needinfo?(jsantell)

Comment 16

3 years ago
Victor, do you have any suggestions on which existing styles I might be able to use for fixing the display on the dark theme in Mac? devtools-toolbarbutton seems quite similar to devtools-button. I'm a little new to CSS, so your suggestions will help.
The image of the light theme is when I click the button -- seems to adopt the OS's button select theme which is bright blue on OSX -- it should ideally look more similar to the other buttons in the tool
Flags: needinfo?(jsantell)

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 18

3 years ago
needinfo ping?
Make sure the element is a <xul:button> with class .devtools-toolbarbutton, and use a "command" event listener instead of a "click" handler.
Flags: needinfo?(vporof)
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Version: 46 Branch → unspecified

Comment 21

3 years ago
Victor, I tried your suggestion of using a button with a command event handler. However, the handler is not being called when the event is fired though I have an event listener registered for the command event. Too keep things simple, I tried adding a dummy event listener directly to the button element. It didn't work for me. The surprising thing is that it worked for 2 other people I was working with on the #devtools IRC.
Here's the code for that: https://pastebin.mozilla.org/8865079

While trying to debug if my event listeners are in the first place even getting registered, I noticed and filed this bug: Bug 1259070

I tried various approaches and am stuck here. Let me know if you have any suggestions. Also, should we add Bug 1259070 as "Depends on" to this bug?

Comment 22

3 years ago
^
Flags: needinfo?(vporof)

Comment 23

3 years ago
needinfo ping

Comment 24

3 years ago
Jordan, do you have any suggestions for the above problem that I'm running into?
Flags: needinfo?(jsantell)
Can't work on this right now, busy with Tofino.
Flags: needinfo?(vporof)
Sorry Maurya, its hard for me to get to this right now, but pinging Greg for ya
Flags: needinfo?(jsantell) → needinfo?(gtatum)
Hello Maurya, are you still interested in working on this? I kind of lost track of this in the shuffle of some of the team over to Project Tofino, and I only joined the team in January so I've still been figuring things out. I was looking through the comment thread again, but unfortunately all of your pastebin examples were deleted as they are probably too old. Could you provide the code samples so I can examine the problem?
Flags: needinfo?(gtatum)

Comment 28

3 years ago
Hi Greg, thanks for reaching out! I'm currently busy with a few other bugs on the iOS side. So, if anyone's interested meanwhile, I wouldn't mind giving up this bug.

The problem I had with the command event handler not being called when an event is fired seems to be isolated to my setup on a Windows 8 machine. Others on the chat channel who were helping me out with this problem were not able to reproduce it on their side.

So, I'd say that Victor Porof's suggestions in comment19 should be tried out on the patch that I had previously submitted.
Ok thanks! My thought on this is that it would probably be a better to go ahead and remove the XUL from this component anyway. I'll take you off of the bug, but leave it open for now.
Assignee: maurya1985 → nobody
Status: ASSIGNED → NEW
Mentor: jsantell

Comment 30

3 years ago
Is anyone actively working on this ? I could work on this if there is no one else already.
Hello nick, a mentor would be great too because it looks like there will be discussion needed on some steps.
Nope, no one is working on it, feel free to take it. I can mentor on this one. Does the existing discussion give you enough to get started?e
Mentor: gtatum

Comment 32

3 years ago
Thanks Greg, first I will need a link to see that (link/button) "Show allocation triggers" because Tim's link on comment 3 looks expired. 
I will inform if I need anything else further.
Here you go: http://chromium.github.io/octane/

Also CNN.com might generate enough garbage to cause the non-incremental GC to show up, without the crazy overhead of the octane tests. Oh, and if you can make sure to "need info" when you need something from me it'll help make sure that I get back to you in a timely manner. It's that little checkbox and dropdown next to the Save Changes button.

Comment 34

3 years ago
Hi Greg,
I tried your octane link and CNN too, but hardly found a Non-Incremental GC. But whatever I found, there was no button/label like show allocation triggers
see this http://prntscr.com/bv3s0e
please find a way so that I can see this label, then I will be able to modify it 
Thanks
Flags: needinfo?(gtatum)
Unfortunately for the case with this bug, non-Incremental GCs are pretty rare so they are hard to find one. CNN does seem like a good place to find them, I've had trouble with octane freezing the browser because the load is too much. I just did some testing on my end and it looks like the button doesn't show up if there aren't any recorded triggers. So only SOME of the Non-Incremental GCs have the button :-/ I'll double check with someone that that's really the case.

It might be worth manually adding that panel for the purpose of working on that button so there is a more consistent way to see what you are working on. I wonder if there is a mochitest that builds up that panel for you. You can run `./mach mochitest path/to/test.js --jsdebugger` and throw in debugger breakpoints to have a place a nice little repeatable workspace. Let me know if you want me to look in the tests and see if I can find a good test that might let you do that.
Flags: needinfo?(gtatum)
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Blocks: 1307838
Hi, I'd like to work on this bug,would u assign this to me?
(Assignee)

Comment 38

2 years ago
I have added a new CSS style and the button looks like a link with the provided patch. 

Please review the changes, also, if you have trouble generating the "Non-incremental GC" event with the "Show allocation changes" button, try running half of the the JetStream tests at http://browserbench.org/JetStream/
Attachment #8822915 - Flags: review?(gtatum)
(Assignee)

Comment 39

2 years ago
Added screenshots taken after 0001-devtools-marker-details-link-style.patch is applied.
Oh thanks! Would you mind using the custom-button class instead of adding a new one, and then placing it in devtools/client/themes/performance.css


custom-button doesn't appear to be used anywhere outside of the performance tools, and this way it will only affect the performance tools. https://dxr.mozilla.org/mozilla-central/search?q=custom-button&redirect=false

Comment 41

2 years ago
As Kerem has submitted a patch and seems to be on the right track,
[Once becomes able to set the correct button class name on the target button, the patch will be good to go]

Assigning on Kerem for now.
[We should not keep Unassigned bugs with patches attached]
Assignee: nobody → keremkat
Status: NEW → ASSIGNED
(Assignee)

Comment 42

2 years ago
Thank you for the quick response. As to the unassigned issue with a patch, that is my fault. I haven't asked to be assigned this issue before submitting the first patch.
Attachment #8822915 - Attachment is obsolete: true
Attachment #8822915 - Flags: review?(gtatum)
Attachment #8823697 - Flags: review?(gtatum)
(Assignee)

Comment 43

2 years ago
I have made the requested changes, could you take another look? Thanks.
Flags: needinfo?(gtatum)
Attachment #8727159 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Comment on attachment 8823697 [details] [diff] [review]
0002-devtools-marker-details-link-style.patch

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

That works for me.
Attachment #8823697 - Flags: review?(gtatum) → review+
(Assignee)

Comment 45

2 years ago
Oh great! Would you mind pushing the patch to the try server for testing?
Flags: needinfo?(gtatum)
Wow, my tooling is doing weird stuff. Thought I pushed it to try yesterday when I reviewed, now I've pushed it twice. Anyway here it is (twice):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c43fc67df226f4363f3a8020ea882b743764210d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff32efbd3f079691c5d4dbd8991d3a372cca9ff8
Flags: needinfo?(gtatum)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 47

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/104807b2a841
Change devtools "Show allocation triggers" button style to resemble a link. r=gtatum
Keywords: checkin-needed

Comment 48

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/104807b2a841
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I have reproduced this bug in nightly 46.0a1 (2016-01-25) on Deepin Linux, 64bit

This bug is now verified as fixed in Latest Beta 54.0b12 (64-bit)

Build ID 	20170529095024
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170531]

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.