found while testing bug 164990, using 2002.11.19.11 on 10.2.2. 0. make sure you have the Navigation > Tabbed Browsing pref "clicking a link with the Command key opens a new window" enabled. 1. cmd-click a bookmark in the toolbar, or cmd-double-click a bookmark in the sidebar. results: the link is opened in the current tab (or window, if you don't have multiple tabs in view). expected: the link should be opened in a new window. fortunately, cmd-clicking a link in the web page content still opens the link in a new window.

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

WFM with the 20040101 NB. Probably due to changes made in the new bookmarks manager code by David Haas Bug 212630 ?

works in new bm manager but not in history

cmd-click in the toolbar when the pref is set to open in a new window fails. works if the pref is set to open in a new tab. odd. probably something that can fall off the list for 0.8 if we run out of time.

not fixed by work done in 200378. want to take a look smfr?

off to 0.9

Simon: this doesn't look too hard. Me take? Command-click in toolbar now WFM Looks like just the history isn't working. Also looks like the history doesn't obey the "opens in new tab" option -- just opens in the active tab.

How's this? So, now that I have a patch, how do I proceed?

Seek reviews from me and pink.

Comment on attachment 160299 [details] [diff] [review] check for Command key in history view I would reassign this to myself, but it looks like I don't have the priviledge yet.

This looks good except for a minor formatting issue: I've been informed that the preferred style for Camino is not to have braces on conditional statements that contain only one line. So + if (![item isKindOfClass: [HistoryItem class]]) { + return; + } should be + if (![item isKindOfClass: [HistoryItem class]]) + return; Ditto for: + if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL]) { + [mBrowserWindowController openNewTabWithURL:url referrer:nil loadInBackground:loadInBackground]; + } + else { + [mBrowserWindowController openNewWindowWithURL:url referrer: nil loadInBackground:loadInBackground]; + } Otherwise it looks good and works as advertised, so if formatting is fixed, r=me@mollyandgeoff.com.

OK - though it's not my preferred style.

don't we have this code that checks the key and opens in a new window or tab already somewhere in the BWC? Can't we make a central access point that handles all this for anyone loading a url?

cc'ing geoff to see if he knows the answer to the quetion i asked.

I think I had a bottleneck that handled modifier keys when opening bookmarks; not sure if we can reuse/refactor for history.

The BWC doesn't quite have this logic from what I see -- there's something in the MainController [loadBookmarkLwithWindowController:blah], but it expects to take a BookmarkItem. I basically imitated the logic there.

(In reply to comment #14) > don't we have this code that checks the key and opens in a new window or tab > already somewhere in the BWC? Can't we make a central access point that handles > all this for anyone loading a url? The code in BWC doesn't check the key... it simply allows callers who've already checked the key and prefs to specify whether a url or search term should be opened in a new tab or window and in the foreground or background. I would perhaps be leery of checking the key where the URL is loaded because: 1. Command key during a drag may imply slightly different behavior than command key during text entry. 2. The URL load *may* just be far enough separated from the event handling that if we wait until the URL load to check the key the user could have released it by then, and we'd get difficult-to-reproduce behavior. IMO the modifier key status should be checked as close to the event handling as possible.

I got it So what's going on with the other review here? Is sfrase gonna do it, or should I reassign to someone else?

Comment on attachment 160600 [details] [diff] [review] fixed braces style sr=pink

landed