317214 - Bookmarks menu or bar checks for Cmd-key state after click is registered

Status: RESOLVED DUPLICATE of bug 333765

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[froodian (Ian Leue)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051118 Camino/1.0b1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051118 Camino/1.0b1+

When opening a bookmark from a folder in the bookmarks bar or from the bookmarks menu, the click is registered before testing whether or not Command is being held down.  The result is that when opening bookmarks quickly in both the same tab and new tabs, the behavior can be confusing

Reproducible: Always

Steps to Reproduce:
1. Select a bookmark, enclosed in a folder in the bookmarks bar
2. IMMEDIATELY after releasing mouse button, depress and hold the command key

Actual Results:  
Bookmark loads in new tab

Expected Results:  
Bookmark should load in current tab

The same is possible in reverse:

1. Cmd-click on a bookmark, enclosed in a folder in the bookmarks bar
2. IMMEDIATELY after releasing mouse button, release Command key.

Actual Results: Bookmark loads in current tab
Expected Results: Bookmark should load in new tab

Comment 1

[froodian (Ian Leue)]

For reference, I'm on a iBook G4 1.33Ghz, 1.2GB RAM, 60GB HD running 10.4.3

Comment 2

[Torben]

I see this too (2005111504 (v1.0b1+), 10.2.8), confirming.

Comment 3

[Nick Forge]

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

Comment 4

[Nick Forge]

This bug happens for me too, using 10.4.3, and 1.0b2.

My Logitech mouse is setup to do a Command-Click when I use the middle mouse button, and because of the way that the software emulates the Command-Click (doing so instantaneously, as described in this bug) this bug happens everytime I try to middle-click a bookmark in a folder.

Comment 5

[Simon Fraser [no longer active]]

OK, that's a good reason to fix this.

Comment 6

[Mark Mentovai]

Is this going to happen for 1.0?

Comment 7

[Markus Haenchen]

Cannot reproduce this either way.
Version 2006020309 (1.0rc1), Powerbook G4 1.67 Hi-Res, build-in trackpad button

Comment 8

[Adrian von Stechow]

Confirmed on a PBG4 1.5 Ghz, Version 2006021400 (1.0), built-in trackpad too.
I think I remember a similar bug in early releases of firefox for mac, can't find the bug though.

Comment 9

[Samuel Sidler (old account; do not CC)]

1.0 has shipped. Requesting approval for 1.0.1.

Comment 10

[Håkan Waara]

Attachment: Proposed fix

What this does is move the cmd-key logic from the loadBookmark:-method to the callers which today are two:

* BookmarkButton
  - Here we simply handle the logic in the mouseDown:-method.
* BookmarkViewController
  - For the bookmark manager, I don't think cmd-click ever did the right thing, so I fixed this as well.

Comment 11

[Håkan Waara]

The only gotcha is that cmd-clicking and then selecting a bookmark in the main bookmarks menu won't react to the command key.  

But that seems quite natural, since it's a main menu after all.  

(Also, we'd have to jump through hoops to make the menu know about the key modifiers, when it opens, just to do the right thing if the bookmarks menu is selected. Another indication that it wouldn't be very normal behavior.)

Comment 12

[Mark Mentovai]

GetCurrentKeyModifiers sucks.

It's not synchronized with any event state.  What we REALLY want is the modifier state at the time the event occurred, we don't want the "current" modifiers, because event processing may have been delayed and the modifier state may have changed.  Your change in BookmarkButton mouseDown: is the right thing to do, because it pulls the modifiers from the NSEvent.  Where you can't use the NSEvent, there's the possibility of GetCurrentEventKeyModifiers, but I'm not convinced that's a good idea in Cocoa (see bug 320746).

I'd also not like to break the menu, but that's trickier.

Comment 13

[Mark Mentovai]

Actually, if this is where we drop 10.2, we can add extra bookmark menu items and call NSMenu setAlternate:YES.  It will still do the right thing for us if we don't set key equivalents but do set a modifier for the alternate.

Comment 14

[Torben]

> The only gotcha is that cmd-clicking and then selecting a bookmark in the 
> main bookmarks menu won't react to the command key.  
> 
> But that seems quite natural, since it's a main menu after all.  

FWIW, Safari opens new tabs if the Cmd-key is held down when you _open_ the menu, the state when you actually select the bookmark you want to open doesn't matter. This is the opposite of how the Opt-key works in all other menus...

Comment 15

[Mark Mentovai]

Comment on attachment 214429 [details] [diff] [review]
Proposed fix

r- per discussion.  Håkan and I came up with good ways to cover all the bases.

Comment 16

[Håkan Waara]

Attachment: Proposed fix with alternate menuitems

There are two possible solutions to this bug:

#1:  Either move the command-click logic outside of loadBookmark: and let the mouseDown-methods handle that, and in the case of clicking a item in the bookmark menu, we'd use alternate menuitems for that.

#2:  (Not tested to make sure it'd work.)  Change the current line GetCurrentModifierKeys() to [NSApp currentEvent]. I think this should work, and it's less of a change, but IMHO also feels more unsafe since we're never 100% guaranteed to get the right event.

This patch is approach #1.

Comment 17

[Håkan Waara]

Comment on attachment 214550 [details] [diff] [review]
Proposed fix with alternate menuitems

FYI, this is missing the change to add a constant "eBookmarkOpenBehaviorNewDefault" in MainController.h

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 214550 [details] [diff] [review]
Proposed fix with alternate menuitems

If this approach gets all the requisite approvals, then this bug needs to be minused for 1.0.1.

Comment 19

[Mark Mentovai]

It needs to be minused anyway.

Comment 20

[Håkan Waara]

Attachment: Rename and remove some constants (checked in)

This is a simple cleanup (per Mark's request) to make the EOpenBookmarkBehavior constants human readable, and also remove the foreground/background constants that were unused. This make this logic less prone to bugs and more readable.

Comment 21

[Mark Mentovai]

Comment on attachment 214612 [details] [diff] [review]
Rename and remove some constants (checked in)

The enum names should still be eBookmarkOpenBehavior*, to match the enum type.  Please use:

eBookmarkOpenBehavior_Preferred
eBookmarkOpenBehavior_NewWindowOrTab
eBookmarkOpenBehavior_NewWindow
eBookmarkOpenBehavior_NewTab

The idea of doing this separately from the other patch made more sense when we thought it'd be more cleanup - since the foreground/background stuff is unused, it's trivial.

eBookmarkOpenBehavior_NewWindowOrTab will be wired up in the other patch.

r=me with all that renaming, and sr=me too but only because this is cleanup (ordinarily, the more eyes, the better.)

Comment 22

[Håkan Waara]

Comment on attachment 214612 [details] [diff] [review]
Rename and remove some constants (checked in)

Checked in with comments addressed.

Comment 23

[Håkan Waara]

Comment on attachment 214550 [details] [diff] [review]
Proposed fix with alternate menuitems

Any other review comments on this patch? 
It remains pretty much the same, even after the cleanup.

Comment 24

[Håkan Waara]

Attachment: Updated patch

Comment 25

[Mark Mentovai]

Comment on attachment 214680 [details] [diff] [review]
Updated patch

Index: src/application/MainController.mm
===================================================================
+  BOOL openInNewTabPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];

This is only used in one case, eBookmarkOpenBehavior_NewWindowOrTab.  Please move it down there, and avoid an unnecessary conditional like this:

openInNewTab = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
openInNewWindow = !openInNewTab;

+  // if this is a alternate, it means the cmd-key is down, and the user wants to open in a new tab/window.

That's only true because the only alternate that's being added to the menu is a command-key alternate.  Your comment should be explicit, and mention BookmarkMenu.mm by name as the file responsible for setting the alternate.

Index: src/bookmarks/BookmarkButton.mm
===================================================================


+  if (cmdKeyDown) {
+    if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
+      currentAction = @selector(openBookmarkInNewTab:);
+    else 
+      currentAction = @selector(openBookmarkInNewWindow:);
+  
+    [self setAction:currentAction];
+  }


Hold the phone!  You're completely obliterating [self action] even if it's something other than openBookmark: - not a normal bookmark item, for example.

Index: src/bookmarks/BookmarkMenu.mm
===================================================================

+      menuItem = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent: @""];

You can get rid of the space between keyEquivalent: and @"".

+      [menuItem setKeyEquivalentModifierMask:0];

Not necessary.

-      [menuItem setAction:@selector(openMenuBookmark:)];


Good call moving this up into the init.

+      // make a copy that is an "alternate"; which means it will be used whenever the user presses
+      // cmd and show the menu.
+      menuItemAlternate = [menuItem copy];

[...blah blah blah...]
+  // if we have alternate items, we need to add and release them as well.
+  if (menuItemAlternate)


The copy is sucky and easily avoided.  Since we need to do a conditional anyway at NSMenu addItem: time to release the alternate, let's change things around a bit.  After you do [self addItem:menuItem]; but before [menuItem release];, reuse menuItem.  Change the two fields and then add it into the NSMenu.  You can add a BOOL isNormalBookmark to control this behavior.

Index: src/bookmarks/BookmarkViewController.mm
===================================================================

+      
^^^^^^^

(Note 6 characters of whitespace)

I see this a lot in your patches - is this something your editor is doing for you?  Can you make it stop?  Not a tremendous deal, but blank lines shouldn't have bogus whitespace.

Comment 26

[Håkan Waara]

(In reply to comment #25)
> +      [menuItem setKeyEquivalentModifierMask:0];
> 
> Not necessary.

This is necessary because the default modifier mask includes command-click, so we need to clear that.

> 
> +      
> ^^^^^^^
> 
> (Note 6 characters of whitespace)
> 
> I see this a lot in your patches - is this something your editor is doing for
> you?  Can you make it stop?  Not a tremendous deal, but blank lines shouldn't
> have bogus whitespace.
> 

You need to see it in context:

+      
+      if (commandKeyDown)

I have xcode setup to indent automatically to the same level, as the code I am editing, is this not normal?

Other than that, all your comments are fixed. Thanks

Comment 27

[Håkan Waara]

Attachment: Patch

Two more fixes:

* I was using the old carbon event cmdKey accidently, oops.
* You can't just reuse an already-added NSMenuItem - NSMenu will scream at you and refuse to even add the first one, so we have to do the copy. However, I've changed this code slightly according to that comment anyway.

Comment 28

[Mark Mentovai]

Comment on attachment 215007 [details] [diff] [review]
Patch

> I have xcode setup to indent automatically to the same level, as the code I am
> editing, is this not normal?

It's normal to edit like that if it's your preference.  I don't consider it normal to check that stuff in.  We hardly have any of that extra whitespace in our code.  Get rid of it.

Comment 29

[Simon Fraser [no longer active]]

Comment on attachment 215007 [details] [diff] [review]
Patch

>Index: src/application/MainController.mm
>===================================================================

>   BOOL loadNewTabsInBackgroundPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
>   
>   NSWindow* behindWindow = nil;
> 
>   switch (behavior)
>   {
>     case eBookmarkOpenBehavior_Preferred:
>-      {
>-        BOOL cmdKeyDown = (GetCurrentKeyModifiers() & cmdKey) != 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];
>-          }
>-      }
>+      break;

Maybe add a comment to say why this clause is now empty.


> -(IBAction) openMenuBookmark:(id)aSender
> {
>   BookmarkItem*  item = [aSender representedObject];
>-  [self loadBookmark:item withWindowController:[self getMainWindowBrowserController] openBehavior:eBookmarkOpenBehavior_Preferred];
>+  EBookmarkOpenBehavior openBehavior = eBookmarkOpenBehavior_Preferred;
>+  
>+  // in BookmarkMenu.mm we add command-key alternate menuitems. if the
>+  // cmd-key is down, we open a new tab or window, depending on the pref.
>+  if ([aSender isAlternate])
>+    openBehavior = eBookmarkOpenBehavior_NewWindowOrTab;

You're assuming that sender is a menu item here, which is probably OK, but you might want to check that sender responds to |isAlternate| just in case.

>Index: src/bookmarks/BookmarkButton.mm
>===================================================================

> -(void)mouseDown:(NSEvent*)aEvent
> {
>   mLastEventWasMenu = NO;
>+  
>+  SEL previousAction = [self action];
>+  SEL tempAction = nil;
>+  
>+  BOOL cmdKeyDown = (([aEvent modifierFlags] & NSCommandKeyMask) != 0);
>+  
>+  // if this is a cmd-click on a normal bookmark, let's open in a new
>+  // tab/window, depending on the pref.
>+  if (cmdKeyDown && previousAction == @selector(openBookmark:)) {
>+    if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
>+      tempAction = @selector(openBookmarkInNewTab:);
>+    else 
>+      tempAction = @selector(openBookmarkInNewWindow:);
>+  
>+    [self setAction:tempAction];
>+  }
>+  
>   [super mouseDown:aEvent];
>+  
>+  if (tempAction)
>+    // since we temporarily changed our action (the cmd key was down), let's revert it
>+    [self setAction:previousAction];
>+  
>   if ([[self cell] lastClickHoldTimedOut])
>     [self showFolderPopup:aEvent];
> }

This is a bit ugly. I wonder if it could be made cleaner with a NSCell subclass that implements something like -setAlternateAction:forKeyModifier: and then overrides -action to return the correct one.

>Index: src/bookmarks/BookmarkMenu.mm
>===================================================================

> - (void)appendBookmarkItem:(BookmarkItem *)inItem buildingSubmenus:(BOOL)buildSubmenus
> {
>   NSString *title = [[inItem title] stringByTruncatingTo:MENU_TRUNCATION_CHARS at:kTruncateAtMiddle];
>-
>-  NSMenuItem *menuItem = nil;
>+  BOOL hasAlternateBookmark = NO;
>+  
>+  NSMenuItem *menuItem;
>+  
>   if ([inItem isKindOfClass:[Bookmark class]])
>   {
>     if (![(Bookmark *)inItem isSeparator])  // normal bookmark
>     {
>-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      menuItem = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent:@""];
>+      [menuItem setKeyEquivalentModifierMask:0];
>       [menuItem setTarget:[NSApp delegate]];
>-      [menuItem setAction:@selector(openMenuBookmark:)];
>       [menuItem setImage:[inItem icon]];
>+      
>+      // normal bookmark items also have an alternate item, that will be used
>+      // for cmd-clicks.
>+      hasAlternateBookmark = YES;
>     }
>     else    //separator
>     {
>       menuItem = [[NSMenuItem separatorItem] retain];
>     }
>   }
>   else if ([inItem isKindOfClass:[BookmarkFolder class]])
>   {
>     BookmarkFolder* curFolder = (BookmarkFolder*)inItem;
>     if (![curFolder isGroup])     // normal folder
>     {
>-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent:@""];
>       [menuItem setImage:[inItem icon]];
> 
>       BookmarkMenu* subMenu = [[BookmarkMenu alloc] initWithTitle:title bookmarkFolder:curFolder];
>       // if building "deep", build submenu; otherwise it will get built lazily on display
>       if (buildSubmenus)
>         [subMenu rebuildMenuIncludingSubmenus:buildSubmenus];
> 
>       [menuItem setSubmenu:subMenu];
>       [subMenu release];
>     }
>     else // group
>     { 
>-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>+      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent:@""];
>       [menuItem setTarget:[NSApp delegate]];
>       [menuItem setAction:@selector(openMenuBookmark:)];
>       [menuItem setImage:[inItem icon]];      
>     }
>   }
> 
>   [menuItem setRepresentedObject:inItem];
>   [self addItem:menuItem];
>+  
>+  if (hasAlternateBookmark)
>+  {
>+    // if we need to add an alternative item (for example to handle cmd-click),
>+    // let's reuse the info from the current menuitem, and just tag it as an alternate.
>+    NSMenuItem *alternate = [menuItem copy];
>+    [alternate setKeyEquivalentModifierMask:NSCommandKeyMask];
>+    [alternate setAlternate:YES];
>+    
>+    [self addItem:alternate];
>+    [alternate release];
>+  }
>+  
>   [menuItem release];
> }

Whoa, you've just doubled the size of the bookmarks menu. I can't let this in without some hard numbers on how it affects bookmark loading time with a large (3Mb) bookmarks file. I worked my ass off to optimize that :)


>Index: src/bookmarks/BookmarkViewController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkViewController.mm,v
>retrieving revision 1.66
>diff -u -8 -p -r1.66 BookmarkViewController.mm
>--- src/bookmarks/BookmarkViewController.mm	10 Mar 2006 10:37:23 -0000	1.66
>+++ src/bookmarks/BookmarkViewController.mm	14 Mar 2006 13:42:50 -0000
>@@ -474,38 +474,55 @@ static const int kDisabledQuicksearchPop
> 
> -(IBAction)openBookmark: (id)aSender
> {
>   NSArray* items = nil;
>   if ([aSender isKindOfClass:[BookmarkItem class]])
>     items = [NSArray arrayWithObject:aSender];
>   else
>     items = [mBookmarksOutlineView selectedItems];
>+  
>+  // if the command key is held down, we open the bookmark in a new tab or window, depending on the preference.
>+  BOOL commandKeyDown = (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0);
>+  BOOL openInNewTabPref = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
> 
>   NSEnumerator* itemsEnum = [items objectEnumerator];
>   id curItem;
>   while ((curItem = [itemsEnum nextObject]))
>   {
>     // see if it's a rendezvous item
>     if ([curItem isKindOfClass:[RendezvousBookmark class]] && ![curItem resolved])
>     {
>       [[NetworkServices sharedNetworkServices] attemptResolveService:[(RendezvousBookmark*)curItem serviceID] forSender:curItem];
>-      mOpenActionFlag = kOpenBookmarkAction;
>+      
>+      if (commandKeyDown)
>+        // open in new tab or new window, depending on the preference.
>+        mOpenActionFlag = openInNewTabPref ? kOpenInNewTabAction : kOpenInNewWindowAction;
>+      else
>+        mOpenActionFlag = kOpenBookmarkAction;
>     }    
>     else if ([curItem isKindOfClass:[BookmarkFolder class]] && ![curItem isGroup])
>     {
>       if ([mBookmarksOutlineView isItemExpanded:curItem])
>         [mBookmarksOutlineView collapseItem:curItem];
>       else
>         [mBookmarksOutlineView expandItem:curItem];
>     }
>     else
>     {
>+      EBookmarkOpenBehavior openBehavior;
>+  
>+      if (commandKeyDown)
>+        // open in new tab or new window, depending on the preference.
>+        openBehavior = eBookmarkOpenBehavior_NewWindowOrTab;
>+      else
>+        openBehavior = eBookmarkOpenBehavior_Preferred;
>+      
>       // otherwise follow the standard bookmark opening behavior
>-      [[NSApp delegate] loadBookmark:curItem withWindowController:mBrowserWindowController openBehavior:eBookmarkOpenBehavior_Preferred];
>+      [[NSApp delegate] loadBookmark:curItem withWindowController:mBrowserWindowController openBehavior:openBehavior];
>     }
>   }
> }

Maybe we should figure out if we can bottleneck all the bookmark opening through one routine that does the cmdKey check. It sucks to have so many places we have to do this.

Comment 30

[Håkan Waara]

(In reply to comment #29)
> (From update of attachment 215007 [details] [diff] [review] [edit])
>
> <snip lots of good comments>
>
> >+    [alternate setAlternate:YES];
> >+    
> >+    [self addItem:alternate];
> >+    [alternate release];
> >+  }
> >+  
> >   [menuItem release];
> > }
> 
> Whoa, you've just doubled the size of the bookmarks menu. I can't let this in
> without some hard numbers on how it affects bookmark loading time with a large
> (3Mb) bookmarks file. I worked my ass off to optimize that :)
> 

:)

rebuildMenuIncludingSubMenus: is only ever called with NO currently. This means that that 3MB bookmarks file needs to be completely flat ;)

So I think your optimization is still valid (making the submenus load lazyily).

How monstrous a bookmarks file (i.e., how many top-level bookmarks) do you want me to test with? 

> 
> >Index: src/bookmarks/BookmarkViewController.mm
> >===================================================================
> >RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkViewController.mm,v
> >retrieving revision 1.66
> >diff -u -8 -p -r1.66 BookmarkViewController.mm
> >--- src/bookmarks/BookmarkViewController.mm	10 Mar 2006 10:37:23 -0000	
> > <snip lots of code>s
> > }
> 
> Maybe we should figure out if we can bottleneck all the bookmark opening
> through one routine that does the cmdKey check. It sucks to have so many places
> we have to do this.
> 

Right now, we only have one function. So the patch is basically the opposite of what you suggest here...   I guess we could use [NSApp currentEvent], and that would "probably" work, but mento has suggested it might not work with accessibility software.

So I kinda agree, it's a little messy, but we need to start tracking the commandkey where the click happens, and not after the fact if we want to fix this bug...

I'm open to ideas though.

Comment 31

[Håkan Waara]

Anyone interested in helping out? We need a big (~ 3MB) bookmarks file to test with.

I think the patch is doing the right thing, we just need to make sure there's no perf hit.

Comment 32

[Simon Fraser [no longer active]]

See bug 236373 for a large bookmarks file.

BTW, I sr-'d the patch. That means that I don't like it as-is.

Comment 33

[Håkan Waara]

(In reply to comment #32)
> See bug 236373 for a large bookmarks file.
> 
> BTW, I sr-'d the patch. That means that I don't like it as-is.
> 

Yes, I meant the general approach.  I will make a new one with your comments though. :)

Comment 34

[Håkan Waara]

Duping against bug 333765 now that it seems that its patch will be used, and the patch here is basically a duplicate.

*** This bug has been marked as a duplicate of 333765 ***