Steps to reproduce: 1. Focus a link. 2. Cmd+Return. Result: nothing happens. Expected: open the link in a new tab. I'm using a 10/27 Firefox trunk build. When did this regress?

It doesn't work in 10/25 SeaMonkey either (10.3.9). Hmm, and it's not a cocoa regression (like bug 355817)?

I just checked: Firefox 2006-09-28-08 --> Cmd+Return works Firefox 2006-09-29-06 --> Cmd+Return does not work I wonder what all those shortcuts that broke due to the cocoa-switch have in common?

What the shortcuts that broke with the cocoa switch have in common is that they don't have menu items that match up with them. In comment #1, what makes you think this is not a cocoa regression?

Jesse - this doesn't work in Carbon Firefox either. I get the impression that the only key combo that is supposed to open a URL in a new tab is option-enter. Are you sure command-enter is supposed to do what you think it is supposed to?

Josh: this bug is about pressing modifiers+enter on a focused link, that's different than pressing the same combo in the location bar.

(In reply to comment #3) > What the shortcuts that broke with the cocoa switch have in common is that they > don't have menu items that match up with them. > > In comment #1, what makes you think this is not a cocoa regression? > Nothing, that was supposed to suggest that it *was* a cocoa regression. My bad grammar, probably.

We've had this bug forever in Camino; see bug 164863. I'm guessing since this broke with Cocoa, it's the same bug, but given the different cmd-handling issues with Fx, it may not be.

While working on this I noticed we don't init mIgnoreDoCommand.

Initializing mIgnoreDoCommand doesn't fix this bug, we just need to do it and it isn't worth its own bug.

Comment on attachment 291376 [details] [diff] [review] init mIgnoreDoCommand, v1.0 Yea we should be doing this.

"init mIgnoreDoCommand, v1.0" landed on trunk

Cmd+Enter on a focused link opens the link in a new tab for me. This is a trunk build from a few days ago.

Cmd+Return is still broken for me with a debug build from last night.

Keyboard events coming in through NSView's performKeyEquivalent are getting sent to the wrong widgets.

landed on trunk

backed out fix v1.0 because it caused bug 415923

Comment on attachment 300703 [details] [diff] [review] fix v1.0 > // don't bother if we don't have a gecko widget > if (!mGeckoChild) > return YES; > >- // return 'NO' if we are in a transaction of IME. >- if (nsTSMManager::IsComposing()) >+ // Return 'NO' if we are in a transaction of IME or we are not the first responder. >+ if (nsTSMManager::IsComposing() || [[self window] firstResponder] != self) > return NO; Assuming you still end up with something like this, the order of these checks should be reversed or it will still be possible for ChildViews to incorrectly black-hole events.

This fixes bugs 358379, 418226, and 417108. It probably fixes bug 411304 also, but I need to check with Masayuki and also make sure we're not breaking IME in some other way. Not requesting review yet because I want to do more work on it, more testing and cleanup.

Comment on attachment 305932 [details] [diff] [review] fix v2.0 >+ // consume the event but don't do anything if we don't have a gecko widget > if (!mGeckoChild) > return YES; Actually, wouldn't the right behavior be to return NO in this case? Clearly if there's no mGeckoChild, then Gecko isn't consuming the event.

Comment on attachment 305932 [details] [diff] [review] fix v2.0 looks ok, and there are no damage for IME.

Can you document exactly what mKeyPressSent means? These window members describing events scare me. If one of the events spawns a nested event loop, we can reenter our event handling code and the member gets all confused. This is one of the things that got us into trouble with Cocoa focus handling. That's why you need to explain precisely the invariants that govern mKeyPressSent and ensure they hold even if one of the event handlers spawns a nested event loop (so e.g. the keyup event might be processed *during* the keydown event handler...)

Comment on attachment 306356 [details] [diff] [review] fix v2.2 The variable I added and 3 that are already in the tree do have potential problems with nested event loops. If an event loop is spawned by an event after one of the values is set, the value will could get wiped out by the time the nested event loop ends, causing problems. This is unlikely but possible. We definitely need to fix that but for now I think we should have it documented (as it is in this patch revision) and land the patch as is. I'll file a followup bug on the issue. This patch really needs to make it into beta4, and I want more time to fix and test the fix for the nested event loop issues.

Comment on attachment 306356 [details] [diff] [review] fix v2.2 You haven't documented what mKeyPressSent means. Please do that.

Comment on attachment 306356 [details] [diff] [review] fix v2.2 a1.9b4=beltzner w/roc's comments addressed

landed on trunk

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko Minefield/3.0b4pre ID:2008022907. Pressing Cmd+Enter opens the underlaying link in a new background tab now.

Mike Connor tells me that the patch for this bug (attachment 306356 [details] [diff] [review]) has triggered a topcrasher: http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0b4pre&range_value=2&signature=libobjc.A.dylib%400x146e4 http://crash-stats.mozilla.com/?do_query=1&product=Firefox&version=Firefox%3A3.0b4pre&query_search=signature&query_type=contains&query=&date=&range_value=1&range_unit=days These crashes seem to happen on accessing a deleted object. Here's a potential fix, and a tryserver build made with it: https://build.mozilla.org/tryserver-builds/2008-03-01_09:22-smichaud@pobox.com-bugzilla358379-crash/smichaud@pobox.com-bugzilla358379-crash-firefox-try-mac.dmg If you've seen these crashes (and can reproduce them), please try this build and report your results here.

> Mike Connor tells me Just to be clear, Mike sent email to Josh, me, and mozilla-1.9-drivers@mozilla.org.

Comment on attachment 306760 [details] [diff] [review] Possible fix for topcrasher test build looks good, please land and clobber ASAP per 1.9 drivers

Steven, for the future it would be nice if you could file a new bug. It makes QA more easier. There is still bug 420403 which handles this topcrash.

Landed on trunk and clobbered (at least I think the Mac builds have been clobbered). > Steven, for the future it would be nice if you could file a new > bug. It makes QA more easier. There is still bug 420403 which > handles this topcrash. Sorry ... but I was in a big rush and didn't know about bug 420403.

(In reply to comment #34) > Landed on trunk and clobbered (at least I think the Mac builds have > been clobbered). Crash doesn't occur anymore with my updated selfmade debug build Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko Minefield/3.0b4pre ID:2008030120.