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

RESOLVED FIXED in Firefox 11

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 579060 [details] [diff] [review]
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.
Attachment #579060 - Flags: review?(gavin.sharp)
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?
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #579060 - Flags: review?(mak77)
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.
Attachment #579060 - Flags: review?(mak77) → review+
(Assignee)

Comment 4

6 years ago
> ::: 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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
(Assignee)

Updated

6 years ago
Attachment #579060 - Flags: review?(gavin.sharp)

Comment 5

6 years ago
It missed the Firefox 11 release for 25 minutes.
Target Milestone: Firefox 11 → Firefox 12

Comment 6

6 years ago
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
Target Milestone: Firefox 12 → Firefox 11

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
(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

6 years ago
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
(Assignee)

Comment 10

6 years ago
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

5 years ago
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

5 years ago
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?
(Assignee)

Comment 13

5 years ago
(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

5 years ago
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)
(Assignee)

Comment 15

5 years ago
(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

5 years ago
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?
(Assignee)

Comment 17

5 years ago
(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

5 years ago
I have done so in bug 765619.
You need to log in before you can comment on or make changes to this bug.