bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th from 16:00 until 20:00 UTC.

Context menu items everywhere should respect command-pref / shift toggle even if the key is pressed after the menu is shown



Camino Graveyard
Toolbars & Menus
11 years ago
6 years ago


(Reporter: froodian (Ian Leue), Assigned: froodian (Ian Leue))


(Depends on: 1 bug, Blocks: 1 bug)



(1 attachment)



11 years ago
(Bug 350806 comment 7)
> Part of the bitrot was the landing of BWC's |shouldLoadInBackground|, which
> uses current event modifier flags.  In order to avoid the "have to hold
> modifier before opening the menu bug," I made that method take a sender param. 
> For now, I made all other calls except the one concerned with this bug send in
> NULL to |shouldLoadInBackground|, essentially preserving the same behavior they
> have now.  A lot the other calls will have (already have) the same problem, and
> will need to get alternate menu items and send |shouldLoadInBackground| their
> sender as a param.  That can be covered in a followup bug.

Basically, any menu item that respects the shift toggle should have a shift alternate.  However, this is (and has been) an ongoing problem for our context menus, since IB won't give non-keyed menu items shift modifiers.  Our solution to date has been to give items needing alternates a certain tag, and fill in the rest later.  However, as more items need alternates, this is becoming more of a problem - See bug 359694.  This will bite us in the ass for some other items too ("Open in new window" and "Open in new tab" in the frame context menus).

What it really boils down to is that context menus are dynamic, and IB works with static or mostly static menus best.  I think we should seriously consider porting all context menus in BrowserWindow.nib to code (like we have for BVC context menus, for instance).  This has the (huge) downside of lots and lots of new code, and the (huge) upside of truly dynamic menus.

Comment 1

11 years ago
Created attachment 248702 [details] [diff] [review]
Patch for HistoryOutlineViewDelegate (checked in on trunk and MOZILLA_1_8_BRANCH)

This is a fix for the easy places (easy since HistoryOutlineViewDelegate creates its context menus programatically).  This:

- Sends sender to the 4 |shouldLoadInBackground| calls in this file.
- Creates alternates for the context menu items for 3 of the above calls.

The fourth one is for when we double-click.  Sender isn't a menu item in that case, so it's the same as sending in nil, but will be less fragile if we ever change the way the shift key's state is determined for non-menu items.

Obviously, this is a partial patch to this bug.  All other |shouldLoadInBackground| calls are either in BWC (and any action regarding them should pend a decision about comment 0), or are called from a non sender-initiated context (and should still be sent nil).
Attachment #248702 - Flags: review?(stuart.morgan)


11 years ago
Attachment #248702 - Flags: review?(stuart.morgan) → review?(bugzilla)

Comment 2

11 years ago
@@ -288,17 +288,17 @@
   id curItem;
   while ((curItem = [itemsEnum nextObject]))
     if ([curItem isKindOfClass:[HistorySiteItem class]])
       [urlArray addObject:[curItem url]];
   // make new window
-  BOOL loadNewTabsInBackgroundPref = [BrowserWindowController shouldLoadInBackground:nil];
+  BOOL loadNewTabsInBackgroundPref = [BrowserWindowController shouldLoadInBackground:aSender];

Can we call this something else now that it's no longer referring to the pref directly? It's not required by any means, but it would make more sense.

r=me with or without that change for the HistoryOutlineViewDelegate patch.

I'd be in favour of moving BrowserWindow's context menus into code for ease of alternates, unless Apple decides to allow IB to specify alternates on shortcut-less menu items in a soon-to-be-released future version.



11 years ago
Attachment #248702 - Flags: superreview?(stuart.morgan)
Attachment #248702 - Flags: review?(bugzilla)
Attachment #248702 - Flags: review+

Comment 3

11 years ago
Comment on attachment 248702 [details] [diff] [review]
Patch for HistoryOutlineViewDelegate (checked in on trunk and MOZILLA_1_8_BRANCH)

Attachment #248702 - Flags: superreview?(stuart.morgan) → superreview+

Comment 4

11 years ago
Comment on attachment 248702 [details] [diff] [review]
Patch for HistoryOutlineViewDelegate (checked in on trunk and MOZILLA_1_8_BRANCH)

Patch checked in on trunk and MOZILLA_1_8_BRANCH.  We should leave this bug open to discuss the remaining issues (whether the BrowserWindow.nib context menus should be made programatically instead)
Attachment #248702 - Attachment description: Patch for HistoryOutlineViewDelegate → Patch for HistoryOutlineViewDelegate (checked in on trunk and MOZILLA_1_8_BRANCH)
Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.
Target Milestone: Camino1.6 → ---


10 years ago
Duplicate of this bug: 408478
I'd like to see us fix this for 2.0, or at least the bug 359694 sub-case, which is probably the most common case of this we hear about.
Depends on: 359694

Comment 8

10 years ago
Putting this into the bug, since I keep remembering and forgetting again: the AppKit release notes for 10.5 suggest that the alternates hack should no longer be necessary:

"-[NSApplication currentEvent] set when menu tracking ends
For applications linked on or after Leopard, -[NSApplication currentEvent] will reflect the event that caused menu tracking to end during the action of the selected menu item."

I assume that means we can just read the modifiers at the moment the menu was seleceted, as $DIETY intended; we should verify that, and if so only do the alternates dance on 10.4.

As with bug 417963 comment 0, which comes from the same document (but doesn't mention linking at all), I don't see anything in that document that explains if "linked on or after Leopard" means SDK or build host, and/or if, once linked on or after Leopard, the behavior depends on the runtime OS or the SDK compiled against.

If someone wants to write a test app for this and/or bug 417963, I'll happily compile it here (to approximate our 10.5 buildhost), both with the 10.4u SDK and 10.5 SDK, and then we can test the behavior of both versions on 10.4 and 10.5 and see what actually happens.
From the current (10.7.x) AppKit release notes:

Backward Compatibility
One backward compatibility mechanism that is occasionally used in the frameworks is to check for the version of the system an application was built against, and if an older system, modify the behavior to be more compatible. This is done in cases where bad incompatibility problems are predicted or discovered; and most of these are listed below in these notes.

Typically we detect where an application was built by looking at the version of the System, Cocoa, AppKit, or Foundation frameworks the application was linked against. Thus, as a result of relinking your application on Lion, you might notice different behaviors, some of which might cause incompatibilities. In these cases because the application is being rebuilt, we expect you to address these issues at the same time as well. For this reason, if you are doing a small incremental update of your application to address a few bugs, it's usually best to continue building on the same build environment and libraries used originally.



smokey% uname -srv
Darwin 9.8.0 Darwin Kernel Version 9.8.0: Wed Jul 15 16:55:01 PDT 2009; root:xnu-1228.15.4~1/RELEASE_I386

smokey% otool -L /Users/smokey/Camino/dev/192branch/Camino192Branch/camino/build/Debug/Camino.app/Contents/MacOS/Camino | grep AppKit
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 824.47.0)

smokey% defaults read /System/Library/Frameworks/AppKit.framework/Versions/C/Resources/Info CFBundleVersion

I can't figure out where the version number is in the SDK’s copy of the framework is (since it's all headers), but:

cb-xserve01 bash$ uname -srv 
Darwin 8.11.1 Darwin Kernel Version 8.11.1: Wed Oct 10 18:23:28 PDT 2007; root:xnu-792.25.20~1/RELEASE_I386

cb-xserve01 bash$ defaults read /System/Library/Frameworks/AppKit.framework/Versions/C/Resources/Info CFBundleVersion

So it seems like the version number we're linked against is recorded as the SDK’s version number, not the build host’s.
You need to log in before you can comment on or make changes to this bug.