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

Status: VERIFIED FIXED

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

Description

[Andrei Vaida [:avaida]]

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.

Comment 1

[Wilhem Barbier [:nounoursheureux]]

Attachment: bug1173775_right_click_doesnt_open_file.patch

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 :)

Comment 2

[Jordan Santell [:jsantell] [@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);
}

Comment 3

[Wilhem Barbier [:nounoursheureux]]

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)

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

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);

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Wilhem, are you still interested in working on this?

Comment 6

[Wilhem Barbier [: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 :)

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1173775-2.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=935be2304c00

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Put the comment #4 as a test patch, pushing against try

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Landed your changes with the tests, thanks Wilhem!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e24bd21c18fb
https://hg.mozilla.org/integration/fx-team/rev/0fdc8212f08f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e24bd21c18fb
https://hg.mozilla.org/mozilla-central/rev/0fdc8212f08f

Comment 12

[Nazir Ahmed Sabbir [:NaSb]]

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

Comment 13

[Mohammad Maruf Islam]

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.