376077 - Cmd-enter in Location Bar beeps but does correct action

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Mike Connor [:mconnor]]

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

Comment 2

[Josh Aas]

What is cmd-enter supposed to do?

Comment 3

[Phil Ringnalda (:philor)]

> (add www.__.com)

It lets you type "mozilla cmd+enter" to load www.mozilla.com.

Comment 5

[Jesse Ruderman]

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

Comment 6

[kerplunk]

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!

Comment 7

[Josh Aas]

Attachment: fix v1.0

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 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 9

[Josh Aas]

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.

Comment 10

[Josh Aas]

landed on trunk

Comment 11

[Marcia Knous [:marcia]]

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.

Comment 12

[Josh Aas]

I backed this out due to regressions.

Comment 13

[Jesse Ruderman]

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

Comment 16

[Josh Aas]

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.

Comment 17

[Jesse Ruderman]

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.

Comment 18

[Josh Aas]

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.

Comment 19

[Josh Aas]

Attachment: fix v2.0

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

Comment 20

[Josh Aas]

Attachment: fix v2.0.1

Comment 21

[Steven Michaud [:smichaud] (Retired)]

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).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 23

[Josh Aas]

Attachment: fix v2.1

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.

Comment 24

[Josh Aas]

landed on trunk

Comment 25

[Chris Lawson (gone)]

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

Comment 26

[[e-mail]]

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.

Comment 27

[Jesse Ruderman]

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

Comment 28

[Marcia Knous [:marcia]]

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.