946450 - [e10s] CMD+click on about:newtab link opens site in current tab, not a new background tab

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Open new tab.
2. CMD+click a site link on the page.

EXPECTED:
Site should open in a new background tab.

RESULT:
Site opens in the current tab. Right-clicking and selecting "Open Link in New Tab" works correctly on about:newtab.

This bug only affects the about:newtab page. CMD+click works correctly on regular pages' links.

Comment 1

[Allison Naaktgeboren :ally]

I vote m2. all paths to opening new tabs should work before GA (and keyboard shortcuts too).

Comment 2

[Matthew N. [:MattN]]

Mass-move to Firefox::New Tab Page.

Filter on new-tab-page-component.

Comment 3

[Olly Hodgson]

Additionally, clicking site links on e10s about:newtab with the middle mouse button doesn't open the link at all. It should open in a new background tab.

(Let me know if this should be a separate bug!)

Comment 4

[Tom S [:evilpie]]

Attachment: v1

We already have a nice function that does all the work of selecting the right target.

Comment 5

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8053a24cd4e3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8053a24cd4e3

Comment 7

[[:Aleksej]]

Ctrl+click

current tab: 2014-07-29-03-02-02-mozilla-central-firefox-34.0a1.en-US.linux-x86_64
new foreground tab: 2014-08-01-03-02-01-mozilla-central-firefox-34.0a1.ru.linux-x86_64
new foreground tab (normal sites: new background tab): 2014-08-05-03-03-00-mozilla-central-firefox-34.0a1.ru.linux-x86_64

Comment 8

[Tom S [:evilpie]]

Okay I see what you mean. It opens a new foreground tab instead of background tab. (Your description wasn't really that obvious)

Comment 9

[Tom S [:evilpie]]

So the problem seems to be that whereToOpenLink does the exact opposite of what normals happens when pressing CTRL. (You can actually notice this when clicking on the back button while pressing ctrl). I guess the function is just not the right one to use?

Normal Link:
ctrl + click = open new background tab
ctrl + shift + click = open new tab in foreground (i.e. switch to it)

What whereToOpenLink does:
ctrl + click = open new tab in foreground
ctrl + shift + click = open new background tab

Comment 10

[Tom S [:evilpie]]

Attachment: Fix background behavior

openLinkIn will generally prefer to load tabs in the foreground when "fromChrome" is true and "inBackground" is not set. By passing the value of "browser.tabs.loadInBackground" (default: true) we basically reverse that behavior and notch it in the direction of opening tabs in background by default, and in the foreground when shift is pressed.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8470863 [details] [diff] [review]
Fix background behavior

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

::: browser/base/content/browser.js
@@ +2577,5 @@
>      if (anchorTarget instanceof HTMLAnchorElement &&
>          anchorTarget.classList.contains("newtab-link")) {
>        event.preventDefault();
>        let where = whereToOpenLink(event, false, false);
> +      let inBackground = Services.prefs.getBoolPref("browser.tabs.loadInBackground");

nit - lets add a comment here similar to your bug comment 10 explaining why we do this.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8463624 [details] [diff] [review]
v1

Hmm, I just realized this was already marked as fixed and this patch is associated with the landing.

In the future lets do follow ups in follow up bugs.

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8470863 [details] [diff] [review]
Fix background behavior

I just realized that this is not the best way to fix this bug. I am going to open a new bug and mark this bug as fixed. Sorry for the trouble.

Comment 14

[[:Aleksej]]

Verified fixed with 2014-08-19-03-02-02-mozilla-central-firefox-34.0a1.ru.linux-x86_64.

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

Confirming this also works on Windows 7 64-bit, Mac OS X 10.8.5 and Ubuntu 12.04 using latest Nightly, build ID: 20141021030208.