Cmd-enter in the location bar has been beeping probably since cocoa. Bug 355817 fixed the broken cmd shortcuts, and cmd-enter does what it's supposed to do (add www.__.com), but it continues to beep. Cmd-shift-enter (.org) beeps, but Shift-enter (.net) doesn't. Assuming bug 355817 fixed the issue at the core side, perhaps there's something the browser doesn't like about the new widgets.

Josh, Colin, can you guys take a look here?

What is cmd-enter supposed to do?

> (add www.__.com) It lets you type "mozilla cmd+enter" to load www.mozilla.com.

Maybe the preventDefault hack in ctrlNumberTabSelection [1] should go away in favor of something in canonizeUrl [2]? [1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.886&mark=1425-1434#1423 [2] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.886#2090

Just to clarify on this one: The "Command + Return" shortcut is used to add http://www. and .com onto a word (in most cases). For example, if I type the word "mozilla" in the address bar and then press the COMMAND and RETURN keys on my keyboard, it will change "mozilla" to "http://www.mozilla.com/" It is a very handy feature. The only problem with the way it works in the latest build of Firefox is correct, except it beeps. It shouldn't beep. :) That is all. The beeping is not a problem in Firefox 2 at all. Thanks guys!

Just like we decided to never call the parent impl of keyDown: or pass events up the receiver chain for keyDown:, we should never let performKeyEquivalent continue on the event chain after sending events into gecko. The return value does not reliably tell us if the event was handled, so we have to assume it was handled. See bug 379199 comment #24 for more info.

Comment on attachment 291379 [details] [diff] [review] fix v1.0 It seems that this is adhoc, but I think that this is good way for now.

Comment on attachment 291379 [details] [diff] [review] fix v1.0 I think there is a lot of work to do in performKeyEquivalent (see bug 358379), but I don't think there is anything ad-hoc about this particular fix. If we ever send this type of Gecko event into Gecko we'll say that we handled the event, and that isn't going to change.

landed on trunk

verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre. I verified on Intel Mac as well.

I backed this out due to regressions.

I think fixing this in Firefox (like I suggested in comment 5) makes more sense than changing cocoa widgets.

Jesse (re: comment #13) - no matter what happens with this particular bug, returning YES or NO based on the return value for the gecko event dispatch is wrong. Until I learn otherwise, fixing the systemic problem is the right thing to do. See comment #7. We'll just keep getting bugs like this if we fix it in Firefox code.

I don't see any evidence that we'll "keep getting bugs like this". Windows has the same behavior of beeping on unhandled shortcuts, so Firefox code mostly gets this right. On the contrary, your solution is known to cause real problems such as bug 406970 and bug 407037.

The evidence is in the code, and the code isn't very complicated. As far as I know, we should not be trusting the return values for event dispatch to tell us if an event was handled or not. Thus, returning the result of the event dispatch as the result for performKeyEquivelent is wrong. No lack of other evidence is going to make that right, only a well-reasoned explanation of why I'm wrong will change my mind. My first attempt at fixing this didn't work out, but I still have no reason to believe my general approach is incorrect. I fixed the first bug you cited within minutes and I am well on my way to addressing the others. I should have a new patch pretty soon. I discovered that Apple changed the behavior of NSMenu's performKeyEquivalent: with the release of 10.5. On 10.4 if the menu had an item with the key equivalent, it would return YES. Even doing nothing for a disabled item was considered "handling" the key equiv. On 10.5 it only returns YES if the menu contains and invokes an enabled menu item corresponding to the key equiv. This is the cause of the rest of the regressions my patch had, I'll address it in the next revision.

This fixes the regressions caused by my first patch. Going to do some more testing before asking for review.

Comment on attachment 299826 [details] [diff] [review] fix v2.0.1 This looks fine to me. I did have questions at a couple of points (which I asked Josh via IRC): 1) Returning YES unconditionally at the end of [ChildView performKeyEquivalent:], and 2) the call to nsMenuItemX::Create() in LoadNativeMenus() in nsWebShellWindow.cpp. Josh told me that 1) is (basically) because the return value of nsChildView::DispatchWindowEvent() isn't reliable, and 2) that the call to nsMenuItemX::Create() should always have been there (and in any case is now needed to initialize the key-equivalent hash table). It might be a good idea to add comments at these two points (or at least at the first one).

+ CocoaKeyEquivContainer& operator=(CocoaKeyEquivContainer& other) + { + mModifiers = other.mModifiers; + mString = [other.mString retain]; + return *this; Shouldn't we release the old mString? Is it really necessary to keep all this data in the hash table? I.e., are you sure it would be unacceptably slow to just crawl the DOM to determine whether a key is associated with a disabled menu item? Because I'm not convinced.

Fixes leak roc pointed out. My choice to use a hash table is less about performance than it is clarity. It probably doesn't matter from a performance POV which one we pick, but a hash table is a better choice in terms of code simplicity and clarity.

landed on trunk

This appears to have broken Escape in the location bar, at least on Camino. See bug 415437.

This patch apparently caused bug 417466, because it hardcodes the keycode for the tilde key. It's different on 102-key keyboards and might also be remapped by the keyboard layout. In addition, window-switching might be bound to some other key combination in the first place.

The beeping on Cmd+Enter is back. See bug 427412.

https://litmus.mozilla.org/show_test.cgi?id=5022 has been modified to include making sure this behavior doesn't surface when invoking Cmd+Enter.