Fix bug 333765 (along with 290212, 317952 and some others). Cmd-clicking and shift-clicking was broken in the bookmark menu (and context menus). There were also issues for 3rd party mice that had a button tied to cmd/shift click.

We now work around the problem by using alternate menuitems for all bookmark menuitems. r=hwaara, sr=pinkerton. Patch by froodian (stridey@gmail.com)

camino/src/application/MainController.h

@@ -52,8 +52,9 @@
 
 typedef enum EBookmarkOpenBehavior
 {
-  eBookmarkOpenBehavior_Preferred,        // opens the bookmark based on key combo and prefs
-  eBookmarkOpenBehavior_ForceReuse,       // ignore prefs and key combo, reuse the current browser 
+  eBookmarkOpenBehavior_Preferred,     // Reuse current window/tab, unless there isn't one, then open a new one
+  eBookmarkOpenBehavior_NewPreferred,  // Open in new window or tab based on prefs
+  eBookmarkOpenBehavior_ForceReuse,
   eBookmarkOpenBehavior_NewWindow,
   eBookmarkOpenBehavior_NewTab
 };
@@ -187,8 +188,7 @@ typedef enum EBookmarkOpenBehavior
 - (NSView*)getSavePanelView;
 - (NSWindow*)getFrontmostBrowserWindow;
 
-- (void)loadBookmark:(BookmarkItem*)item withWindowController:(BrowserWindowController*)browserWindowController openBehavior:(EBookmarkOpenBehavior)behavior;
-
+- (void)loadBookmark:(BookmarkItem*)item withBWC:(BrowserWindowController*)browserWindowController openBehavior:(EBookmarkOpenBehavior)behavior reverseBgToggle:(BOOL)reverseBackgroundPref;
 - (void)displayPreferencesWindow:(id)sender;
 - (BOOL)isMainWindowABrowserWindow;
 

camino/src/application/MainController.mm

@@ -494,8 +494,11 @@ const int kReuseWindowOnAE = 2;
   [mDockMenu setBookmarkFolder:[[BookmarkManager sharedBookmarkManager] dockMenuFolder]];
 }
 
-// a central place for bookmark opening logic.
-- (void)loadBookmark:(BookmarkItem*)item withWindowController:(BrowserWindowController*)browserWindowController openBehavior:(EBookmarkOpenBehavior)behavior
+// a central place for bookmark opening logic
+- (void)loadBookmark:(BookmarkItem*)item
+             withBWC:(BrowserWindowController*)browserWindowController
+        openBehavior:(EBookmarkOpenBehavior)behavior
+     reverseBgToggle:(BOOL)reverseBackgroundPref
 {
   if (!browserWindowController)
     browserWindowController = [self getMainWindowBrowserController];
@@ -503,30 +506,26 @@ const int kReuseWindowOnAE = 2;
   BOOL openInNewWindow = (browserWindowController == nil);
   BOOL openInNewTab = NO;
   BOOL newTabInBackground = NO;
-  
+
   BOOL loadNewTabsInBackgroundPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
-  
-  // if shift is held down, reverse the "open new tab/window with focus"-behavior.
-  if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask)
+
+  // if the caller requests it, reverse the "open new tab/window in background" behavior.
+  if (reverseBackgroundPref)
     loadNewTabsInBackgroundPref = !loadNewTabsInBackgroundPref;
-  
+
   NSWindow* behindWindow = nil;
 
+  // eBookmarkOpenBehavior_Preferred not specified, since it uses all the default behaviors
   switch (behavior)
   {
-    case eBookmarkOpenBehavior_Preferred:
-      {
-        BOOL cmdKeyDown = (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0);
-        if (cmdKeyDown)
-          if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL]) {
-            openInNewTab = YES;
-            newTabInBackground = loadNewTabsInBackgroundPref;
-          }
-          else {
-            openInNewWindow = YES;
-            if (loadNewTabsInBackgroundPref)
-              behindWindow = [browserWindowController window];
-          }
+    case eBookmarkOpenBehavior_NewPreferred:
+      if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL]) {
+        openInNewTab = YES;
+        newTabInBackground = loadNewTabsInBackgroundPref;
+      } else {
+        openInNewWindow = YES;
+        if (loadNewTabsInBackgroundPref)
+          behindWindow = [browserWindowController window];
       }
       break;
 
@@ -535,7 +534,7 @@ const int kReuseWindowOnAE = 2;
       openInNewWindow = NO;
       newTabInBackground = NO;
       break;
-      
+
     case eBookmarkOpenBehavior_NewTab:
       openInNewTab = YES;
       newTabInBackground = browserWindowController && loadNewTabsInBackgroundPref;
@@ -1228,7 +1227,16 @@ Otherwise, we return the URL we originally got. Right now this supports .url,
 -(IBAction) openMenuBookmark:(id)aSender
 {
   BookmarkItem*  item = [aSender representedObject];
-  [self loadBookmark:item withWindowController:[self getMainWindowBrowserController] openBehavior:eBookmarkOpenBehavior_Preferred];
+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
+  BOOL reverseBackgroundPref = NO;
+
+  if ([aSender isAlternate]) {
+    reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
+    if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask)
+      openBehavior = eBookmarkOpenBehavior_NewPreferred;
+  }
+
+  [self loadBookmark:item withBWC:[self getMainWindowBrowserController] openBehavior:openBehavior reverseBgToggle:reverseBackgroundPref];
 }
 
 //

camino/src/bookmarks/BookmarkButton.mm

@@ -165,24 +165,30 @@
 {
   BrowserWindowController* brController = [[self window] windowController];
   BookmarkItem *item = [self bookmarkItem];
+  BOOL reverseBGPref = ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0;
+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
+  if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
+    openBehavior = eBookmarkOpenBehavior_NewPreferred;
 
-  [[NSApp delegate] loadBookmark:item withWindowController:brController openBehavior:eBookmarkOpenBehavior_Preferred];
+  [[NSApp delegate] loadBookmark:item withBWC:brController openBehavior:openBehavior reverseBgToggle:reverseBGPref];
 }
 
 -(IBAction)openBookmarkInNewTab:(id)aSender
 {
   BrowserWindowController* brController = [[self window] windowController];
   BookmarkItem *item = [self bookmarkItem];
+  BOOL reverseBGPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
 
-  [[NSApp delegate] loadBookmark:item withWindowController:brController openBehavior:eBookmarkOpenBehavior_NewTab];
+  [[NSApp delegate] loadBookmark:item withBWC:brController openBehavior:eBookmarkOpenBehavior_NewTab reverseBgToggle:reverseBGPref];
 }
 
 -(IBAction)openBookmarkInNewWindow:(id)aSender
 {
   BrowserWindowController* brController = [[self window] windowController];
   BookmarkItem *item = [self bookmarkItem];
+  BOOL reverseBGPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
 
-  [[NSApp delegate] loadBookmark:item withWindowController:brController openBehavior:eBookmarkOpenBehavior_NewWindow];
+  [[NSApp delegate] loadBookmark:item withBWC:brController openBehavior:eBookmarkOpenBehavior_NewWindow reverseBgToggle:reverseBGPref];
 }
 
 - (IBAction)copyURLs:(id)aSender

camino/src/bookmarks/BookmarkManager.mm

@@ -114,6 +114,8 @@ enum {
 
 - (BOOL)readOperaFile:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder;
 
+- (NSMenuItem*)shiftAlternateMenuItemWithTitle:(NSString*)title action:(SEL)action andTarget:(id)target;
+
 + (void)addItem:(id)inBookmark toURLMap:(NSMutableDictionary*)urlMap usingURL:(NSString*)inURL;
 + (void)removeItem:(id)inBookmark fromURLMap:(NSMutableDictionary*)urlMap usingURL:(NSString*)inURL;  // url may be nil, in which case exhaustive search is used
 + (NSEnumerator*)enumeratorForBookmarksInMap:(NSMutableDictionary*)urlMap matchingURL:(NSString*)inURL;
@@ -734,15 +736,24 @@ static BookmarkManager* gBookmarkManager = nil;
 
   NSMenuItem *menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) keyEquivalent:@""] autorelease];
   [menuItem setTarget:target];
+  [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
   [contextMenu addItem:menuItem];
-  
+
+  NSMenuItem *shiftMenuItem = [self shiftAlternateMenuItemWithTitle:menuTitle action:@selector(openBookmarkInNewWindow:) andTarget:target];
+  [contextMenu addItem:shiftMenuItem];
+
   // open in new tabs in new window
   if (multipleItems)
   {
     menuTitle = NSLocalizedString(@"Open in Tabs in New Window", @"");
+
     menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarksInTabsInNewWindow:) keyEquivalent:@""] autorelease];
+    [menuItem setKeyEquivalentModifierMask:0];
     [menuItem setTarget:target];
     [contextMenu addItem:menuItem];
+
+    shiftMenuItem = [self shiftAlternateMenuItemWithTitle:menuTitle action:@selector(openBookmarksInTabsInNewWindow:) andTarget:target];
+    [contextMenu addItem:shiftMenuItem];
   }
 
   // open in new tab in current window
@@ -750,9 +761,14 @@ static BookmarkManager* gBookmarkManager = nil;
     menuTitle = NSLocalizedString(@"Open in New Tabs", @"");
   else
     menuTitle = NSLocalizedString(@"Open in New Tab", @"");
+
   menuItem = [[[NSMenuItem alloc] initWithTitle:menuTitle action:@selector(openBookmarkInNewTab:) keyEquivalent:@""] autorelease];
+  [menuItem setKeyEquivalentModifierMask:0];
   [menuItem setTarget:target];
   [contextMenu addItem:menuItem];
+  
+  shiftMenuItem = [self shiftAlternateMenuItemWithTitle:menuTitle action:@selector(openBookmarkInNewTab:) andTarget:target];
+  [contextMenu addItem:shiftMenuItem];
 
   if (!outlineView || ([items count] == 1)) {
     menuTitle = NSLocalizedString(@"Get Info", @"");
@@ -860,6 +876,15 @@ static BookmarkManager* gBookmarkManager = nil;
   return contextMenu;
 }
 
+- (NSMenuItem*)shiftAlternateMenuItemWithTitle:(NSString*)title action:(SEL)action andTarget:(id)target
+{
+  NSMenuItem* shiftMenuItem = [[[NSMenuItem alloc] initWithTitle:title action:action keyEquivalent:@""] autorelease];
+  [shiftMenuItem setTarget:target];
+  [shiftMenuItem setKeyEquivalentModifierMask:NSShiftKeyMask];
+  [shiftMenuItem setAlternate:YES];
+
+  return shiftMenuItem;
+}
 
 //
 // Copy a set of bookmarks URLs to the specified pasteboard.

camino/src/bookmarks/BookmarkMenu.mm

@@ -164,7 +164,7 @@ const long kOpenInTabsTag = 0xBEEF;
     {
       [self appendBookmarkItem:curItem buildingSubmenus:includeSubmenus];
     }
-    
+
     [self addLastItems];
     mDirty = NO;
   }
@@ -192,14 +192,37 @@ const long kOpenInTabsTag = 0xBEEF;
   {
     if (![(Bookmark *)inItem isSeparator])  // normal bookmark
     {
-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
+      menuItem                     = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
+      NSMenuItem *cmdMenuItem      = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent: @""];
+      NSMenuItem *cmdShiftMenuItem = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent: @""];
+
       [menuItem setTarget:[NSApp delegate]];
       [menuItem setAction:@selector(openMenuBookmark:)];
       [menuItem setImage:[inItem icon]];
+      [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
+
+      [cmdMenuItem setTarget:[NSApp delegate]];
+      [cmdMenuItem setImage:[inItem icon]];
+      [cmdMenuItem setKeyEquivalentModifierMask:NSCommandKeyMask];
+      [cmdMenuItem setRepresentedObject:inItem];
+      [cmdMenuItem setAlternate:YES];
+
+      [cmdShiftMenuItem setTarget:[NSApp delegate]];
+      [cmdShiftMenuItem setImage:[inItem icon]];
+      [cmdShiftMenuItem setKeyEquivalentModifierMask:NSCommandKeyMask | NSShiftKeyMask];
+      [cmdShiftMenuItem setRepresentedObject:inItem];
+      [cmdShiftMenuItem setAlternate:YES];
+
+      [self addItem:menuItem];
+      [self addItem:cmdMenuItem];
+      [cmdMenuItem release];
+      [self addItem:cmdShiftMenuItem];
+      [cmdShiftMenuItem release];
     }
     else    //separator
     {
       menuItem = [[NSMenuItem separatorItem] retain];
+      [self addItem:menuItem];
     }
   }
   else if ([inItem isKindOfClass:[BookmarkFolder class]])
@@ -225,10 +248,11 @@ const long kOpenInTabsTag = 0xBEEF;
       [menuItem setAction:@selector(openMenuBookmark:)];
       [menuItem setImage:[inItem icon]];      
     }
+
+    [self addItem:menuItem];
   }
 
   [menuItem setRepresentedObject:inItem];
-  [self addItem:menuItem];
   [menuItem release];
 }
 

camino/src/bookmarks/BookmarkViewController.mm

@@ -503,7 +503,13 @@ static const int kDisabledQuicksearchPopupItemTag = 9999;
     else
     {
       // otherwise follow the standard bookmark opening behavior
-      [[NSApp delegate] loadBookmark:curItem withWindowController:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_Preferred];
+      BOOL shiftKeyDown = ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0;
+      EBookmarkOpenBehavior behavior = eBookmarkOpenBehavior_Preferred;
+      // if command is down, open in new tab/window
+      if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
+        behavior = eBookmarkOpenBehavior_NewPreferred;
+
+      [[NSApp delegate] loadBookmark:curItem withBWC:mBrowserWindowController openBehavior:behavior reverseBgToggle:shiftKeyDown];
     }
   }
 }
@@ -529,7 +535,11 @@ static const int kDisabledQuicksearchPopupItemTag = 9999;
     else
     {
       // otherwise follow the standard bookmark opening behavior
-      [[NSApp delegate] loadBookmark:curItem withWindowController:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_NewTab];
+      BOOL reverseBackgroundPref = NO;
+      if ([aSender isAlternate])
+        reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
+
+      [[NSApp delegate] loadBookmark:curItem withBWC:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_NewTab reverseBgToggle:reverseBackgroundPref];
     }
   }
 }
@@ -595,7 +605,11 @@ static const int kDisabledQuicksearchPopupItemTag = 9999;
     else
     {
       // otherwise follow the standard bookmark opening behavior
-      [[NSApp delegate] loadBookmark:curItem withWindowController:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_NewWindow];
+      BOOL reverseBackgroundPref = NO;
+      if ([aSender isAlternate])
+        reverseBackgroundPref = ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0;
+
+      [[NSApp delegate] loadBookmark:curItem withBWC:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_NewWindow reverseBgToggle:reverseBackgroundPref];
     }
   }
 }

camino/src/browser/BrowserWindowController.mm

@@ -4084,9 +4084,9 @@ enum BWCOpenDest {
 //
 - (BOOL)loadBookmarkBarIndex:(unsigned short)inIndex openBehavior:(EBookmarkOpenBehavior)inBehavior
 {
-  NSArray* bookmarkBarChildren     = [[[BookmarkManager sharedBookmarkManager] toolbarFolder] childArray];
-  unsigned int loadableItemIndex   = 0; // holds the number of loadable items we've cycled through
-  NSEnumerator* enumerator         = [bookmarkBarChildren objectEnumerator];
+  NSArray* bookmarkBarChildren   = [[[BookmarkManager sharedBookmarkManager] toolbarFolder] childArray];
+  unsigned int loadableItemIndex = 0; // holds the number of loadable items we've cycled through
+  NSEnumerator* enumerator       = [bookmarkBarChildren objectEnumerator];
   id item;
 
   // We cycle through all the toolbar items.  When we've skipped enough loadable items
@@ -4099,7 +4099,7 @@ enum BWCOpenDest {
   }
 
   if (item)
-    [[NSApp delegate] loadBookmark:item withWindowController:self openBehavior:inBehavior];
+    [[NSApp delegate] loadBookmark:item withBWC:self openBehavior:inBehavior reverseBgToggle:NO];
   else // We ran out of toolbar items before finding the nth loadable one
     NSBeep();