Closed Bug 358379 Opened 15 years ago Closed 13 years ago

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

Categories

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

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: jruderman, Assigned: jaas)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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?
Flags: blocking1.9?
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?
Blocks: cocoa
Assignee: nobody → joshmoz
Component: Keyboard: Navigation → Widget: Cocoa
QA Contact: keyboard.navigation → cocoa
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.
Summary: Cmd+Return no longer opens web page in new tab (Cmd+Enter, which requires holding Fn, works) → Cmd+Return no longer opens focused link in new tab (Cmd+Enter, which requires holding Fn, works)
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.
Flags: blocking1.9? → blocking1.9+
Blocks: 372987
Target Milestone: --- → mozilla1.9alpha6
Blocks: 164863
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Depends on: 398514
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Priority: -- → P2
Target Milestone: mozilla1.9 M10 → ---
Depends on: 404632
While working on this I noticed we don't init mIgnoreDoCommand.
Attachment #291376 - Flags: review?(cbarrett)
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.
Attachment #291376 - Flags: review?(cbarrett) → review+
Attachment #291376 - Flags: superreview?(roc)
Attachment #291376 - Flags: superreview?(roc) → superreview+
"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.
Attached patch fix v1.0 (obsolete) — Splinter Review
Keyboard events coming in through NSView's performKeyEquivalent are getting sent to the wrong widgets.
Attachment #300703 - Flags: review?(masayuki)
Attachment #300703 - Flags: superreview?(roc)
Attachment #300703 - Flags: superreview?(roc) → superreview+
Blocks: 415437
Priority: P2 → P1
landed on trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 415923
backed out fix v1.0 because it caused bug 415923
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch fix v2.0 (obsolete) — Splinter Review
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.
Attachment #300703 - Attachment is obsolete: true
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.
Attachment #305932 - Flags: review?(masayuki)
Comment on attachment 305932 [details] [diff] [review]
fix v2.0

looks ok, and there are no damage for IME.
Attachment #305932 - Flags: review?(masayuki) → review+
Attached patch fix v2.1 (obsolete) — Splinter Review
Attachment #305932 - Attachment is obsolete: true
Attachment #306181 - Flags: superreview?(roc)
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...)
Target Milestone: --- → mozilla1.9beta4
Flags: blocking1.9+
Attached patch fix v2.2Splinter Review
Attachment #306356 - Flags: superreview?
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.
Attachment #306356 - Flags: superreview? → superreview?(roc)
Comment on attachment 306356 [details] [diff] [review]
fix v2.2

You haven't documented what mKeyPressSent means. Please do that.
Attachment #306356 - Flags: superreview?(roc) → superreview+
Attachment #306356 - Flags: approval1.9b4?
Comment on attachment 306356 [details] [diff] [review]
fix v2.2

a1.9b4=beltzner w/roc's comments addressed
Attachment #306356 - Flags: approval1.9b4? → approval1.9b4+
Attachment #306181 - Attachment is obsolete: true
landed on trunk
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Hardware: Macintosh → All
> 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
Attachment #306760 - Flags: superreview+
Attachment #306760 - Flags: review+
Attachment #306760 - Flags: approval1.9b4+
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.
Blocks: 420403
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.
Duplicate of this bug: 430688
You need to log in before you can comment on or make changes to this bug.