707672 - openUILink[In] behavior should not be determined by browser.tabs.loadBookmarksInBackground

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Maybe we'll want a new pref for this (probably not), but using browser.tabs.loadBookmarksInBackground for all openUILink[In] calls -- unless the caller opts out, as a few started doing -- is confusing and doesn't seem to make much sense.

Comment 1

[Marco Bonardo [:mak]]

I think I'd prefer if that pref would die and we'd just respect browser.tabs.loadInBackground, bug 469456 was about that. We may change PlacesUIUtils to respect that pref instead?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #1)
> I think I'd prefer if that pref would die and we'd just respect
> browser.tabs.loadInBackground, bug 469456 was about that. We may change
> PlacesUIUtils to respect that pref instead?

I'm not sure if that's what users would expect, but if so, PlacesUIUtils should do this and it can happen in a followup bug.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 579060 [details] [diff] [review]
patch

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

Provided all tests pass (that means we probably lack a open bookmarks in background test), it looks correct.

::: browser/base/content/browser.js
@@ +8907,5 @@
>        gBrowser.hideTab(newTab);
>        gBrowser.replaceTabWithWindow(newTab);
>        break;
>      case "tabshifted":
>        loadInBackground = !loadInBackground;

wouldn't be simpler now to have:

case "tabshifted":
  // put some comment here.
  break;
case "tab":
  gBrowser.selectedTab = newTab;
  break

since now the initial value is known, there is no longer the need to flip it and fallback, should be easier to understand too.

Comment 4

[Dão Gottwald [:dao]]

> ::: browser/base/content/browser.js
> @@ +8907,5 @@
> >        gBrowser.hideTab(newTab);
> >        gBrowser.replaceTabWithWindow(newTab);
> >        break;
> >      case "tabshifted":
> >        loadInBackground = !loadInBackground;
> 
> wouldn't be simpler now to have:
> 
> case "tabshifted":
>   // put some comment here.
>   break;
> case "tab":
>   gBrowser.selectedTab = newTab;
>   break
> 
> since now the initial value is known, there is no longer the need to flip it
> and fallback, should be easier to understand too.

done

http://hg.mozilla.org/mozilla-central/rev/85d5d52ae629

Comment 5

[Scoobidiver (away)]

It missed the Firefox 11 release for 25 minutes.

Comment 6

[Scoobidiver (away)]

I was wrong about the Firefox 11 target because this changeset is between:
* The latest changeset in Nightly 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b.
* The first changeset in 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654

Comment 7

[nightstalkerz]

I think this patch has broken something else.

Middle clicking on an item in the url bar will now always focus the new tab.

I have these about:config entries set:
browser.tabs.loadBookmarksInBackground;true
browser.tabs.loadDivertedInBackground;true
browser.tabs.loadInBackground;true

Comment 8

[Dão Gottwald [:dao]]

(In reply to nightstalkerz from comment #7)
> I think this patch has broken something else.
> 
> Middle clicking on an item in the url bar will now always focus the new tab.
> 
> I have these about:config entries set:
> browser.tabs.loadBookmarksInBackground;true
> browser.tabs.loadDivertedInBackground;true
> browser.tabs.loadInBackground;true

That's the expected outcome, unless you're saying middle clicks in the url bar popup should be treated like middle clicks on, say, the history and bookmarks menus? This probably makes sense, I think; feel free to file a bug on that.

Comment 9

[Yuichi Torinome]

It is not only the item of the url bar.

Back Forward button
Reload button
Context menu "Back"
Context menu "Forward"
Context menu "Reload"
Context menu "View Image"
Context menu "View Background Image"
...etc

Comment 10

[Dão Gottwald [:dao]]

That's expected. It doesn't make sense to let browser.tabs.loadBookmarksInBackground govern these things. It may make sense to add a new pref for them, but I'm not sure about that.

Comment 11

[adrianna.pinska]

Did this change also cause new tabs opened for search bar results to start loading in the foreground? I could have sworn that they used to open in the background, and I find the current behaviour frustrating.

Is there an existing bug which raises the issue of adding a new preference for configuring this?  I can't seem to find anything related.

Comment 12

[Alex Vallat]

Is it intentional that this has changed the behaviour for openUILink, as used by many extensions? The advice has been that if you are putting a command in a menu, you use this method to open a tab respecting the user's combination of hotkeys and mouse buttons: https://developer.mozilla.org/en/Code_snippets/Tabbed_browser#Opening_a_URL_in_the_correct_window.2Ftab

However, now it seems that if you middle click or ctrl+click a command using this code, the tab will always open in the foreground rather than respecting the user's preferences.

For an easy example, use https://addons.mozilla.org/en-US/firefox/addon/plain-text-links/ which adds a menu command that uses openUILink. Previously, middle clicking the command would open the tab in the background (assuming that preference set), now it's always foreground.

Surely changing the behaviour of all extensions using the recommended method of opening links is not intended?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Alex Vallat from comment #12)
> Is it intentional that this has changed the behaviour for openUILink, as
> used by many extensions? The advice has been that if you are putting a
> command in a menu, you use this method to open a tab respecting the user's
> combination of hotkeys and mouse buttons:
> https://developer.mozilla.org/en/Code_snippets/
> Tabbed_browser#Opening_a_URL_in_the_correct_window.2Ftab
> 
> However, now it seems that if you middle click or ctrl+click a command using
> this code, the tab will always open in the foreground rather than respecting
> the user's preferences.
> 
> For an easy example, use
> https://addons.mozilla.org/en-US/firefox/addon/plain-text-links/ which adds
> a menu command that uses openUILink. Previously, middle clicking the command
> would open the tab in the background (assuming that preference set), now
> it's always foreground.
> 
> Surely changing the behaviour of all extensions using the recommended method
> of opening links is not intended?

openUILink still opens a background tab when holding Shift. The default behavior of opening a foreground tab when middle clicking also didn't change, so the behavior you saw was probably due to you toggling browser.tabs.loadBookmarksInBackground.

Comment 14

[Alex Vallat]

It's the browser.tabs.loadInBackground preference that it should be obeying.

With browser.tabs.loadInBackground = false:
 Middle click on a link opens tab in foreground
 Shift+Middle click on a link opens tab in background
 Middle click on openUILink (tested using Plain Text Links) opens tab in foreground
 Shift+Middle click on openUILink opens tab in background

This is all correct.

With browser.tabs.loadInBackground = true:
 Middle click on a link opens tab in background
 Shift+Middle click on a link opens tab in foregound
 Middle click on openUILink STILL opens tab in foreground
 Shift+Middle click on openUILink STILL opens tab in background

The openUILink behaviour is reversed with respect to the normal link behaviour.

(these tests performed on Aurora 14.0a2, with loadBookmarksInBackground and loadDivertedInBackground both left false)

Comment 15

[Dão Gottwald [:dao]]

(In reply to Alex Vallat from comment #14)
> It's the browser.tabs.loadInBackground preference that it should be obeying.

It never did that.

Comment 16

[adrianna.pinska]

OK, as of Firefox 13, it looks like it's still impossible to revert to the previous behaviour of loading search bar results in background tabs (except by holding shift).

I would like *all* new tabs to load in the background (with the sole exception of blank new tabs).  The search bar currently doesn't respect any of the existing preferences for loading new tabs in the background, including the latest one which governs the behaviour of the context menu search.

If you insist that this behaviour should not fall under any of the four existing preferences, could you please add a new one?

Comment 17

[Dão Gottwald [:dao]]

(In reply to adrianna.pinska from comment #16)
> If you insist that this behaviour should not fall under any of the four
> existing preferences,

Yes.

> could you please add a new one?

If you file a new bug and make your case there, we can consider it.

Comment 18

[adrianna.pinska]

I have done so in bug 765619.