Closed Bug 376077 Opened 14 years ago Closed 13 years ago

Cmd-enter in Location Bar beeps but does correct action

Categories

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

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Mardak, Assigned: jaas)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 alpha6
Josh, Colin, can you guys take a look here?
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Assignee: nobody → joshmoz
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Assignee: joshmoz → nobody
Assignee: nobody → joshmoz
Component: Location Bar and Autocomplete → Widget: Cocoa
Flags: blocking-firefox3+
Product: Firefox → Core
QA Contact: location.bar → cocoa
Target Milestone: Firefox 3 M10 → mozilla1.9 M10
Flags: blocking1.9+
What is cmd-enter supposed to do?
> (add www.__.com)

It lets you type "mozilla cmd+enter" to load www.mozilla.com.
Priority: -- → P3
Target Milestone: mozilla1.9 M10 → ---
Duplicate of this bug: 402953
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!
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #291379 - Flags: review?
Attachment #291379 - Flags: review? → review?(masayuki)
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.
Attachment #291379 - Flags: review?(masayuki) → review+
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.
Attachment #291379 - Flags: superreview?(roc)
Attachment #291379 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Depends on: 407037
I backed this out due to regressions.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
No longer depends on: 355817
No longer depends on: 407037
I think fixing this in Firefox (like I suggested in comment 5) makes more sense than changing cocoa widgets.
Duplicate of this bug: 409239
Duplicate of this bug: 409339
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.
Attached patch fix v2.0 (obsolete) — Splinter Review
This fixes the regressions caused by my first patch. Going to do some more testing before asking for review.
Attachment #291379 - Attachment is obsolete: true
Attached patch fix v2.0.1 (obsolete) — Splinter Review
Attachment #299665 - Attachment is obsolete: true
Attachment #299826 - Flags: review?(smichaud)
Priority: P3 → P2
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).
Attachment #299826 - Flags: review?(smichaud) → review+
Attachment #299826 - Flags: superreview?(roc)
+  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.
Attached patch fix v2.1Splinter Review
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.
Attachment #299826 - Attachment is obsolete: true
Attachment #299873 - Flags: superreview?(roc)
Attachment #299826 - Flags: superreview?(roc)
Attachment #299873 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 415437
This appears to have broken Escape in the location bar, at least on Camino. See bug 415437.
Depends on: 417108
Target Milestone: --- → mozilla1.9beta3
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.
Depends on: 417466
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.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.