Closed Bug 1173775 Opened 6 years ago Closed 6 years ago

Right-clicking a filename from the Call Tree opens its source in a new window

Categories

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

40 Branch
defect

Tracking

(firefox39 affected, firefox40 affected, firefox41 affected, firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox39 --- affected
firefox40 --- affected
firefox41 --- affected
firefox44 --- verified

People

(Reporter: avaida, Assigned: jsantell, Mentored)

Details

(Whiteboard: [good first bug][lang=js][bugday-20151104])

Attachments

(2 files)

Reproducible on:
* Aurora 40.0a2 (2015-06-10)
* Nightly 41.0a1 (2015-06-10)
* Firefox 36.0.3 (20150319201009)

Affected platforms:
Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Steps to reproduce:
1. Launch Firefox.
2. Open a random website, e.g. https://foxyeah.mozilla.org/
3. Open Developer Tools → Performance
4. Record performance for a few seconds then stop the profiler.
5. Click "Call Tree" and then right-click any filename from that section.

Expected result:
Consistent with other Developer Tools, right-clicking a filename does nothing.

Actual result:
Right-clicking a file opens its source in a new window, just as left-clicking it.

Additional notes:
* This is not a regression, the behavior goes back to older Firefox version that use the older Performance tools. 
* For example, in Firefox 36.0.3, clicking a filename displayed below the graph will open its source in Debugger and it will also bring up the context menu in it.
Priority: -- → P3
Whiteboard: [good first bug][lang=js]
I'm basically checking if the click is a right click before opening the file. Should I write tests for this change ? If yes, where are they located ? Thanks :)
Attachment #8646408 - Flags: review?(jsantell)
Comment on attachment 8646408 [details] [diff] [review]
bug1173775_right_click_doesnt_open_file.patch

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

Thanks, Wilhem! I think we can add a simple test for this in test/browser_profiler_tree-view-06.js, to ensure that it does not fire the link event. Looking around to fake a right click, this should work

EventUtils.synthesizeMouseAtCenter(D.target.querySelector(".call-tree-url"), { button: 2, type: "mousedown" }, window);

And just ensure that the event was not called. Let me know if you have any questions and thank you again!

::: browser/devtools/performance/modules/widgets/tree-view.js
@@ +374,5 @@
>     */
>    _onUrlClick: function(e) {
>      e.preventDefault();
>      e.stopPropagation();
> +    if(e.button == 0) this.root.emit("link", this);

nit: style rules:
if (e.button === 0) {
  this.root.emit("link", this);
}
Attachment #8646408 - Flags: review?(jsantell)
How can I check if the event is fired ? The only solution that I see is waiting a certain amount of time and then check if the event has been fired, but I am not familiar at all with Mochitest/Mochikit so I don't know how to do this :/ . Here is an idea, tell me if I'm wrong: 

bool event_fired = false;
D.target.addEventListener('link', function() {
  event_fired = true
});
setTimeout(function() {
  is(event_fired, true, "the 'link' event is not fired when right-clicking");
  finish();
}, 500);

This is pseudo-code and I haven't tried it but I don't know if this is the right way to do it (and I think it isn't :P)
well, it's a bit weird -- we want to test that the event is NOT fired, so yeah, it's a little weird and fragile. But luckily it'll fail on our CI tests since it's a time based thing, so fragility works in our favor in this case.

We can use the utility in tests to just wait an amount of time after clicking, so something like this should work -- let me know if you have other questions!

let linkCalled = false;
function handler () {
  linkCalled = true;
}

treeRoot.on("link", handler);

// Fire the right click
EventUtils.synthesizeMouseAtCenter(D.target.querySelector(".call-tree-url"), { button: 2, type: "mousedown" }, window);

// Wait 200ms
yield idleWait(200);

// Ensure link was not called
ok(!linkCalled, "`link` event not yet fired")

// Fire left click
EventUtils.sendMouseEvent({ type: "mousedown" }, D.target.querySelector(".call-tree-url"));

// Wait until linkCalled is true, should occur quickly, if not synchronously
yield waitUntil(() => linkCalled);

treeRoot.off("link", handler);
Wilhem, are you still interested in working on this?
Flags: needinfo?(nounoursheureux)
I'm sorry it took so long, I was busy with school and other projects. I'm having a problem with the synthesizeMouseAtCenter function, I tried to write the test but it looks like this function isn't firing the "mousedown" event. To be sure I replaced sendMouseEvent({type: "mousedown"}, D.target.querySelector(".call-tree-url")) with synthesizeMouseAtCenter(D.target.querySelector(".call-tree-url"), {button: 0, type: "mousedown"}, window) in the first test and now it is timing out. Maybe I misunderstood what this function is supposed to do ? Thanks for your help :)
Flags: needinfo?(nounoursheureux) → needinfo?(jsantell)
Attached patch 1173775-2.patchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=935be2304c00
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Flags: needinfo?(jsantell)
Attachment #8670533 - Flags: review?(vporof)
Put the comment #4 as a test patch, pushing against try
Attachment #8670533 - Flags: review?(vporof) → review+
Landed your changes with the tests, thanks Wilhem!
https://hg.mozilla.org/mozilla-central/rev/e24bd21c18fb
https://hg.mozilla.org/mozilla-central/rev/0fdc8212f08f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Reproduced this bug on Nightly 41.0a1 (2015-06-10); (Build ID: 20150610030209) in Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Developer Edition 44.0a2 (2015-11-03)

Build ID: 20151103004217
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
QA Whiteboard: [bugday-20151104]
I have reproduced this bug with Firefox 41.0a1 (Build: 20150610030209)on 
windows 8.1 pro 64-bit with the instructions from comment 0 .

Verified as fixed with Latest Firefox Aurora 44.0a2 (Build ID: 20151105004045)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

As it it also verified on Linux (Comment 12),Marking this bug as verified.
Status: RESOLVED → VERIFIED
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][bugday-20151104]
QA Whiteboard: [bugday-20151104] → [bugday-20151104] [bugday-20151116]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.