Closed Bug 50590 Opened 20 years ago Closed 15 years ago

[Mac] Using a key shortcut should 'flash' the menu title

Categories

(Core Graveyard :: Widget: Mac, defect, P3)

PowerPC
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: sfraser_bugs, Assigned: jaas)

References

Details

(Keywords: access, fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

Convention in Mac apps is to flash the menu title when you use the key shortcut 
for an item on that menu. This provides valuable user feedback that the shortcut 
has been recognized and acted upon.

Mozilla's menus don't do that, and I think they should.
A good time to flash the menu title for is 6 ticks.
->future
Target Milestone: --- → Future
I'd prefer 8 ticks, for consistency with Mac OS guidelines for other controls 
(bug 49008).

But whatever, that's a minimum interval; if the operation itself takes longer 
than 8/60 of a second (e.g. pasting a large quantity of text), the menu should 
stay lit for the whole operation. We already do this for opening dialogs, so 
presumably it's possible for other operations.
Would be nice to fix for Mozilla 1.0
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0
nominating for dogfood (from sdagley's list of bugs that are good candidates for 
our next release) 
Keywords: nsdogfood
Keywords: nsCatFood
Keywords: nsdogfood
Keywords: access
Summary: Using a key shortcut should 'flash' the menu title → [Mac] Using a key shortcut should 'flash' the menu title
*** Bug 101447 has been marked as a duplicate of this bug. ***
*** Bug 107142 has been marked as a duplicate of this bug. ***
Are shortcut keys traceable back to the actual menu?  They must be, I guess.  If
so, this should be fairly easy to do once I get bug 66120 working how I want it to.
Dean, do you want to take this bug then? The accelerator is referenced by the
menu XUL command usually.
well menus know what keys they're attached to, so the xul objects could/probably
should maintain backlinks.

alternatively, keys and menus both talk to listeners which control enabling,
that listener has a way to talk to both keys and menus, using this might be
easier and cleaner (although it enforces a requirement...)
Someone else needs to take this if they want it fixed in the moz 1.0 timeframe.
Sorry.
Target Milestone: mozilla1.0 → mozilla1.1
See bug 66120.  If I can figure out my SetAttribute() problem there, I can take
a run at both of these for 1.0.

mpt, please file a different bug on this:

------- Additional Comments From Matthew Thomas (mpt) 2000-09-12 17:21 -------
But whatever, that's a minimum interval; if the operation itself takes longer 
than 8/60 of a second (e.g. pasting a large quantity of text), the menu should 
stay lit for the whole operation. We already do this for opening dialogs, so 
presumably it's possible for other operations.

Someone filed that additional behavior as bug 109844.
*** Bug 109842 has been marked as a duplicate of this bug. ***
Someone filed that additional behavior mpt mentioned as bug 109844.
Since we don't do development for Mac OS 9 or earlier anymore, and this
behaviour is still valid in Mac OS X (for instance, the Finder still does it),
shouldn't we move this to Mac OSX ?
This is related to bug 195979.
OS: Mac System 8.5 → MacOS X
Just an observation [1.4a/OS X], it seems that the menus are getting flashed
when certain things align, I just haven't seen the pattern yet. For example,
right now I have 4  browser windows open and can cycle through them with CMD-1
and the Window menu always flashes on the same one of the 4... then I killed one
of them and it flashes on 2 of the remaining 3.
*** Bug 343880 has been marked as a duplicate of this bug. ***
Attached patch fix v1.0 (obsolete) — Splinter Review
Assignee: saari → joshmoz
Attachment #228443 - Flags: review?(mark)
Component: XP Toolkit/Widgets: Menus → Widget: Mac
Flags: blocking1.8.1?
Target Milestone: mozilla1.1alpha → mozilla1.8.1beta2
Comment on attachment 228443 [details] [diff] [review]
fix v1.0

ChangeNativeEnabledStatusForMenuItem should be defined (no-op) for Cocoa widgets.

The newEnabledStatus argument should be renamed to begin with an "a".  "aEnabled" is fine.

There's some more HelpMenu stuff you can kill in nsMenuBarX and nsMenuX.  If you want to leave the method declared in nsIMenu, you can leave the nsMenu::IsHelpMenu definition, but get rid of all callers and unused member variables.  But I don't see why it needs to remain in nsIMenu.  (Remember to account for cocoa.)

Bump the nsIMenu iid.  It's not in an idl, I assume that means it's exempt from the branch freeze rules.

The Carbon menu-item model is weird, but ChangeNativeEnabledStatusForMenuItem is the best we can do with it.  I thought about having that method get the enabled status by looking at the menu item itself, but newEnabledStatus (aEnabled?) is a good optimization.  I thought about not passing it a menu item and letting it update all menu items, but that seems like overkill and even less optimized.  I thought about adding GetItemNativeData to nsIMenu, but that's not really any better than this.
Attachment #228443 - Flags: review?(mark) → review+
Attached patch fix v1.0.1 (obsolete) — Splinter Review
This addresses Mark's comments, includes diffs to Cocoa menu code.
Attachment #228443 - Attachment is obsolete: true
Attachment #228465 - Flags: review?(mark)
Problem I noticed with v1.0.1 when I was porting it to the trunk - we also need to get rid of "PRPackedBool mIsHelpMenu;" in nsMenuX.h for Carbon and Cocoa.
Attached patch fix v1.0.1 trunkSplinter Review
This is the trunk version of v1.0.1.
Attachment #228465 - Flags: review?(mark) → review+
Attachment #228497 - Flags: review+
Attachment #228497 - Flags: superreview?(mikepinkerton)
Comment on attachment 228497 [details] [diff] [review]
fix v1.0.1 trunk

sr=pink
Attachment #228497 - Flags: superreview?(mikepinkerton) → superreview+
landed on trunk
Attachment #228465 - Flags: approval1.8.1?
Josh - you nominated for 1.8 branch - can you comment on risk/necessity for that branch?
This is a big win in terms of making Firefox feel more like a normal Mac OS X application and it also has good accessibility implications. The fix is not very risky, and we've had it on the trunk for a while in the form of a fix for Cocoa widgets.

There is one problem, which we are working to resolve right now. This fix exacerbates an existing bug (344249) so that it occurs much more frequently. This patch does not actually cause that bug, but that bug occurs when java is loaded and native menus are set up correctly. With this patch native menus are always set up correctly. Fixing the original problem will probably require an update to JEP, which would land at the same time as this patch. One way or another we should have this resolved soon.
(In reply to comment #29)
> https://bugzilla.mozilla.org/show_bug.cgi?id=227489

please disregard this comment. It was an accidental pasting.
Comment on attachment 228465 [details] [diff] [review]
fix v1.0.1

We'd love this patch for the 1.8 branch if it can be done w/o interface changes.
Attachment #228465 - Flags: approval1.8.1? → approval1.8.1-
The only consumer of any of the interfaces changed in this patch is the mac widget code itself - absolutely nobody else, not even in third-party code.
Right now it looks like webshell uses nsIMenu, but it actually doesn't (someone just left the #includes there). I posted a patch to clean that up in bug 344568.
Doesn't seem like a blocker, given how long it's been around, but we're happy to take a patch.

Regarding the patch -- how can you be sure no third-party code can use it?  Is there actually no way for other code to get ahold of the objects?
Flags: blocking1.8.1? → blocking1.8.1-
I don't think it is possible for people to get ahold of nsIMenu objects. Nothing I know of can qi to nsIMenu, and nsIMenu is a Mac OS X only API. You might be able to create an nsIMenu object of your own, but that would be totally pointless.
(In reply to comment #35)
> Nothing I know of can qi to nsIMenu, and nsIMenu is a Mac OS X only API. You
> might be able to create an nsIMenu object of your own, but that would be
> totally pointless.

If nothing can ever use an nsIMenu object, then we should remove the unused file, but I don't think it is since you're changing it in this patch.
Attached patch fix v1.1, 1.8.1 branch (obsolete) — Splinter Review
This version does not change nsIMenu, so no public-facing interfaces are modified. This is accomplished by assuming that anything implementing nsIMenu is of the type nsMenuX.
Attachment #228465 - Attachment is obsolete: true
Attachment #229713 - Flags: review?(mark)
This is the same as Josh's 1.1 patch but moves the method into nsIMenu_MOZILLA_1_8_BRANCH and uses qi instead of a cast.
Attachment #229713 - Attachment is obsolete: true
Attachment #229829 - Flags: review?(joshmoz)
Attachment #229713 - Flags: review?(mark)
Attachment #229829 - Flags: review?(joshmoz) → review+
Attachment #229829 - Flags: superreview?(mikepinkerton)
Attachment #229829 - Flags: superreview?(mikepinkerton) → approval1.8.1?
Comment on attachment 229829 [details] [diff] [review]
Use nsIMenu_MOZILLA_1_8_BRANCH

a=drivers, please go ahead and land this on the 181branch
Attachment #229829 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [checkin needed (1.8 branch)]
checked in on 1.8 branch
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed (1.8 branch)]
This appears to have fixed bug 273208 on both trunk 1.9 and branch 1.8.1.
Depends on: 346878
Blocks: 347026
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.