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

VERIFIED FIXED in Firefox 55

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: nachtigall, Assigned: josh.pinkney, Mentored)

Tracking

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

54 Branch
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(1 attachment, 2 obsolete attachments)

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
Agreed, thanks for the report!

Honza
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [netmonitor]
@Eduardo: this report is related to what you were working on.
Do you have time to implement this feature-request?

Honza
Flags: needinfo?(mail)
Flags: qe-verify?
Whiteboard: [netmonitor] → [netmonitor-reserve]
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Hi, I'd like to take this one if its still available
Posted patch 1343803-bug-fix.patch (obsolete) — Splinter Review
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 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 }
Attachment #8843523 - Flags: feedback+
Assignee: nobody → josh.pinkney
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
Flags: needinfo?(mail)
Posted patch 1343803.patch (obsolete) — Splinter Review
I've just fixed the issues with the patch, let me know if there are any additional changes needed to be made!
Attachment #8843523 - Attachment is obsolete: true
Attachment #8844353 - Flags: review?(odvarko)
Status: NEW → ASSIGNED
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
Attachment #8844353 - Flags: review?(odvarko)
Posted patch 1343803.patchSplinter Review
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.
Attachment #8844353 - Attachment is obsolete: true
Attachment #8844566 - Flags: review?(odvarko)
Comment on attachment 8844566 [details] [diff] [review]
1343803.patch

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

Nice, thanks!

Honza
Attachment #8844566 - Flags: review?(odvarko) → review+
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6b642a9e7c52
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.1 - Mar 20
Priority: P3 → P1
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.