358379 - Cmd+Return no longer opens focused link in new tab (Cmd+Enter, which requires holding Fn, works)

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

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?

Comment 1

[Stefan [:stefanh]]

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

Comment 2

[Stefan [:stefanh]]

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?

Comment 3

[Josh Aas]

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?

Comment 4

[Josh Aas]

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?

Comment 5

[Asaf Romano (gone)]

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

Comment 6

[Stefan [:stefanh]]

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

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 8

[Josh Aas]

Attachment: init mIgnoreDoCommand, v1.0

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

Comment 9

[Josh Aas]

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

Comment 10

[Colin Barrett [:cbarrett]]

Comment on attachment 291376 [details] [diff] [review]
init mIgnoreDoCommand, v1.0

Yea we should be doing this.

Comment 11

[Josh Aas]

"init mIgnoreDoCommand, v1.0" landed on trunk

Comment 12

[Håkan Waara]

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.

Comment 13

[Jesse Ruderman]

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

Comment 14

[Josh Aas]

Attachment: fix v1.0

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

Comment 15

[Josh Aas]

landed on trunk

Comment 16

[Josh Aas]

backed out fix v1.0 because it caused bug 415923

Comment 17

[Stuart Morgan]

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.

Comment 18

[Josh Aas]

Attachment: fix v2.0

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 19

[Stuart Morgan]

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 20

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

Comment on attachment 305932 [details] [diff] [review]
fix v2.0

looks ok, and there are no damage for IME.

Comment 21

[Josh Aas]

Attachment: fix v2.1

Comment 22

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

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 23

[Josh Aas]

Attachment: fix v2.2

Comment 24

[Josh Aas]

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 25

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

Comment on attachment 306356 [details] [diff] [review]
fix v2.2

You haven't documented what mKeyPressSent means. Please do that.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306356 [details] [diff] [review]
fix v2.2

a1.9b4=beltzner w/roc's comments addressed

Comment 27

[Josh Aas]

landed on trunk

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Possible fix for topcrasher

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:[e-mail]-bugzilla358379-crash/[e-mail]

If you've seen these crashes (and can reproduce them), please try this
build and report your results here.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

> Mike Connor tells me

Just to be clear, Mike sent email to Josh, me, and
[e-mail].

Comment 32

[Mike Connor [:mconnor]]

Comment on attachment 306760 [details] [diff] [review]
Possible fix for topcrasher

test build looks good, please land and clobber ASAP per 1.9 drivers

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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