1343803 - Middle-mouse click on [Learn More] should open tab without immediately switching to it

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jens]

I really like the new [Learn More] for Network Status Codes, one nit though ;)

STR:
Middle-mouse click on the [Learn More] in order to open in new tab (but in order to read later, not immediately)

ER:
Like when I middle-mouse click on any other link: Open in a background tab, that is, open new tab but the focus stays on the one that is open right now

AR:
Opens new tab and immediately switches to it

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Agreed, thanks for the report!

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Eduardo: this report is related to what you were working on.
Do you have time to implement this feature-request?

Honza

Comment 3

[Joshua Pinkney]

Hi, I'd like to take this one if its still available

Comment 4

[Joshua Pinkney]

Attachment: 1343803-bug-fix.patch

Hi, I've just fixed the bug. I made it so that middle mouse button opens in a new tab but doesn't bring you to the new tab while left click is still default

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8843523 [details] [diff] [review]
1343803-bug-fix.patch

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

::: devtools/client/netmonitor/shared/components/mdn-link.js
@@ +37,4 @@
>    e.preventDefault();
>  
>    let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +  

nit: whitespace

@@ +37,5 @@
>    e.preventDefault();
>  
>    let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +  
> +  if(e.button === 1){

nit: space after (

@@ +39,5 @@
>    let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +  
> +  if(e.button === 1){
> +    win.openUILinkIn(url, "tabshifted");
> +  }else{

nit: space after }

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the patch Joshua!

Please fix all the comments from Tim and ask me for review
(click on 'details' link next to your patch and use 'review' field)

Honza

Comment 7

[Joshua Pinkney]

Attachment: 1343803.patch

I've just fixed the issues with the patch, let me know if there are any additional changes needed to be made!

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8844353 [details] [diff] [review]
1343803.patch

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

I can't apply the patch on clean m-c clone. It looks like it's built on top of the previous patch.

Also, please see my inline comments.

Thanks!
Honza

::: devtools/client/netmonitor/shared/components/mdn-link.js
@@ +36,4 @@
>    e.stopPropagation();
>    e.preventDefault();
>  
> +  let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);  

nit: remove white-spaces at the end of the line

@@ +43,3 @@
>      win.openUILinkIn(url, "tab");
>    }
>  

nit: remove an empty line

Comment 9

[Joshua Pinkney]

Attachment: 1343803.patch

I redownloaded the source code and started again to make sure I didn't go over previous patches like I did last time. The code is exactly the same and I fixed both the formatting issues from the critiques.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8844566 [details] [diff] [review]
1343803.patch

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

Nice, thanks!

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b642a9e7c52
"Middle-mouse click on [Learn More] should open tab without immediately switching to it" r=honza

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b642a9e7c52

Comment 13

[Ciprian Georgiu, Desktop QA]

Reproduced the issue on Nightly (20170308030207) using STR from comment 0.

This is verified fixed on latest Nightly 55.0a1 (2017-03-09) across platforms:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS