175629 - Loading links in background tabs should work for bookmarks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Cyd]

When "Load links in background" is enabled in the Tabbed Browsing preferences,
middle-clicking on a link loads the link in a new tab in the background.
However, middle-clicking on a bookmark in the bookmarks menu still loads the
link in a new tab in the foreground. It should load in the background instead.

Comment 1

[Shawn Lindsay]

Dup 175335?

Comment 2

[Cyd]

Nope. 175335 refers to an opened bookmark overwriting an existing tab. I am
talking about an opened bookmark opening in a foreground tab, instead of a
background tab. (Toggle "load links in background" and middle click on normal
links to see the difference between foreground and background tab loading behavior.)

Comment 3

[Simon Paquet [:sipaq]]

*** Bug 176081 has been marked as a duplicate of this bug. ***

Comment 4

[Shawn Lindsay]

I see.  Bookmarks opened in new tabs grab focus.  Reproducible here.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021023 Phoenix/0.3,
build 2002102308

Comment 5

[Gabriel Seah]

This happens in Mozilla too.

Except since middle clicking bookmarks in the Personal Toolbar Folder doesn't open them in new tabs, you need to choose "open in new tab" from the context menu of the bookmark.

I suggest changing "Product" to "Browser"

Comment 6

[David Tenser [:djst]]

Just for the record, this was intentonal behavior by David Hyatt. He posted in
the old mz forums about it and got feedback from people. Actually, most people
thought it was better to be consistent, but he thought that you rarely want to
open a bookmark in a background tab, so it should override the pref. 

I'm all for consistency so I'd vote for this bug to be fixed.

Comment 7

[David Tenser [:djst]]

OS -> All

Comment 8

[Pham]

*** Bug 205091 has been marked as a duplicate of this bug. ***

Comment 9

[Blake Ross]

We don't even have UI for this anymore.  (Is the hidden pref still in by
default, or is it all handled by TBE now?)

Comment 10

[David Tenser [:djst]]

Blake: We do have UI for it. General > Windows and Tabs.

Comment 11

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 12

[Jon Henry]

Attachment: Proposed patch

I see in comment 6 that this was originally by design, but it's annoyed me
since I first noticed it months ago so I've finally gone ahead and done up a
little patch.  I've tested this with every combination of the "Load links in
background" and "replace vs. append tab groups" prefs and (at least to me) what
happens in each case with this patch applied is the logical thing. Can we
please reconsider this design decision?

Comment 13

[Jon Henry]

Comment on attachment 138322 [details] [diff] [review]
Proposed patch

Could you please review this, Pierre?

Comment 14

[Mike Connor [:mconnor]]

*** Bug 230408 has been marked as a duplicate of this bug. ***

Comment 15

[Jon Henry]

Comment on attachment 138322 [details] [diff] [review]
Proposed patch

Canceling review request, I found a minor bug in the patch. Basically, I didn't
notice that openNewTabWith sends the current page as the referrer, which we
don't want for bookmark links.

Comment 16

[Jon Henry]

Attachment: Patch v1.1

Updated patch that does not send referrer information.

Comment 17

[David Tenser [:djst]]

Bugs like these will never block a release. Setting the 1.0 flag for this minor
"bug" (design decision) is just silly. -ing.

Comment 18

[Robin Whittleton]

Sorry David, I thought about it more after I'd set it and realised it was stupid
(especially seeing the Target of 'After Firefox 1.0') but couldn't work out how
to remove the flag.

Comment 19

[Jon Henry]

Comment on attachment 138845 [details] [diff] [review]
Patch v1.1

Mike, is this something you could take a look at? It would seem Pierre is not
actively doing much in the way of reviews...

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 138845 [details] [diff] [review]
Patch v1.1

>Index: mozilla/browser/components/bookmarks/content/bookmarks.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v
>+      // open link in new tab
>       var tab = browser.addTab(url);
>-      browser.selectedTab = tab;
>-      browser.focus();
>+
>+      if (!loadInBackground)
>+        browser.selectedTab = tab;
>+

why aren't we going to focus here?  if its conditional now, fine, but that
shouldn't change the action we take when said condition is met.  (This isn't
always going to cause problems, but it can under certain focus circumstances
(especially keyboard navigation))

I also think we need to go with a separate pref at this point, defaulting to
current behaviour (open in foreground).  Whether to add UI for said pref is a
secondary issue.  Reasoning being that a) many people, myself included, expect
and prefer current behaviour and b) its really late to change commonly used
behaviour.  Making it possible to open bookmarks in the background is
reasonable, but making a change to the UE at this stage is going to be
problematic (It would be nice to hook up key modifiers too, but that's another
bug)

>@@ -624,17 +631,18 @@
>       if (index == index0)
>         return;
> 
>-      // Select the first tab in the group.
>-      var tabs = browser.mTabContainer.childNodes;
>-      browser.selectedTab = tabs[index0];
>+      // focus the first tab if prefs say to
>+      if (!loadInBackground || doReplace) {
>+        // Select the first tab in the group.
>+        var tabs = browser.mTabContainer.childNodes;
>+        browser.selectedTab = tabs[index0];
>+        browser.focus();
>+      }

again, still should be a different pref, but I like this, good consistency

so we basically need a new pref like browser.tabs.loadBookmarksInBackground
with a default of false set in firefox.js (browser.tabs.loadInBackground of
course comes from seamonkey, when you can't open bookmarks in tabs from the
main window anyway)

once we have it in, we can argue about UI for it :)

Comment 21

[Jon Henry]

That sounds reasonable Mike, thanks for taking the time to look at this. I'll
work on this when I get a chance in the coming weeks.

Comment 22

[Jon Henry]

Attachment: Revised patch

Patch revised to incorporate Mike's comments. Implements
"browser.tabs.loadBookmarksInBackground", which defaults to false.

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 149228 [details] [diff] [review]
Revised patch

good, minus the indenting nit in the first bit of the patch, but I can fix that
on checkin :)

will land this at some point later on

Comment 24

[Jon Henry]

Thanks for the quick review ;). At first glance, it looks like bug 236864
already deals with hooking up control and shift modifiers to bookmarks. If/when
I try to tackle that, is that the right bug to continue this work in?

Comment 25

[Mike Connor [:mconnor]]

checked in branch and trunk, 2004-05-28 10:18

Comment 26

[Dorando]

If loadBookmarksInBackground is enabled, opening a bookmark won't correctly give
the focus back to the browser, but only remove it from the current element. Load
a document which is long enough to have a scrollbar and try scroll the document
via the keyboard after you've middle-clicked a bookmark to see what I mean.

Changing browser.focus(); to browser.click(); (for both openOneBookmark and
openGroupBookmark) will make it work again without any ill side effects it seems
(While I actually did expect some)...

Should I file another bug for this?

Comment 27

[Mike Connor [:mconnor]]

*** Bug 249549 has been marked as a duplicate of this bug. ***

Comment 28

[Worcester12345]

Can the bug originator confirm the fix? Thanks.

Comment 29

[Worcester12345]

(In reply to comment #28)
> Can the bug originator confirm the fix? Thanks.

Cyd?

Comment 30

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 31

[David McRitchie]

Looks like Bug 469456 – browser.tabs.loadInBackground and browser.tabs.loadBookmarksInBackground could be one pref
addresses the underlying problem.