Closed Bug 428760 Opened 16 years ago Closed 16 years ago

cmd+key shortcuts no longer work after bug 398514 landed (apple key shortcuts broken).

Categories

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

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: jag+mozilla, Assigned: jaas)

References

Details

(Keywords: regression) 

Most shortcut keys (see bug 398514 comment 36) no longer work in Minefield and SeaMonkey nightly trunk builds, nor in my own builds.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre)
Gecko/2008041104 Minefield/3.0pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041112 SeaMonkey/2.0a1pre

I use a regular 104-key US keyboard and the standard US layout, so this isn't bug 422972 or bug 427797 or bug 427995.

I've verified that backing out attachment 314215 [details] [diff] [review] fixes most of the shortcuts (the ones associated with menu items, other ones like cmd+l or cmd+shift+i in SM were already broken (but not that long ago) before that landed), and applying attachment 314624 [details] [diff] [review] from bug 359638 doesn't fix things for me.
Flags: blocking1.9?
Oh, two interesting details I forgot to copy from bug 398514:

cmd+option+t works, as do cmd+left, cmd+right, cmd+h, cmd+q, cmd+,.

Also, hitting e.g. cmd+w or cmd+t highlights the File menu item, hitting cmd+a or cmd+c highlights the Edit menu item, so they shortcut is recognized by the native menu system, but the command isn't executed.
So yeah, I never hit the line in nsMenuBarX.mm where DoCommand() is called (in |menuItemHit|).

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsMenuBarX.mm&rev=1.66&mark=1004,1040#994

For every key that method gets called twice. The first time gMenuEffectsOnly is 1 so it skips the first bit, then bails out at line 1040 because gPerformKeyEquivOnStack is non-null, the second time gMenuEffectsOnly is 0, it's none of the few items we explicitly test for, and then we bail out again at line 1040 because gPerformKeyEquivOnStack is non-null. Maybe there's another entry into |menuItemHit| but it looks like |gPerformKeyEquivOnStack| is always non-null:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsMenuBarX.mm&rev=1.66&mark=965-969#954

Is there code elsewhere that's supposed to execute |DoCommand()| in response to a shortcut?
With all windows closed cmd+n does open a new browser window, probably courtesy of [NSApp mainWindow] returning a null value.

So my naive view is that the test on line 1040 isn't correct, or |gPerformKeyEquivOnStack| shouldn't always be set in |performKeyEquivalent|, or c) there's some other code that's supposed to execute |DoCommand()|, or d) ???, where c and d seem the most likely since this stuff seems to be working just fine for most people.

Any hints on where I might find c) or d)?
I'm using build 2008041204 for MacOS on a MacBook, standard US Qwerty layout. All of the key combos I've tried work except for two: Cmd-LeftArrow and Cmd-RightArrow for history navigation. When I press these, I do see the History menu flash, but the page does not go back or forth. Selecting the menus (or the buttons) works fine.
-'ing as this is WFM for the four of us in the room (10.4 and 10.5).  Marcia, can you take a look and renom if there's an issue?
Flags: blocking1.9? → blocking1.9-
Let's hope that it's just me :-)
Jag has a tendency to be right about things, I'll follow up with him.

Jag, maybe find on IRC so we can debug this together?
I'm actually going to put this on the wanted1.9.0.x list because I think I just hit this issue.  I still can't repro all the time.  Can't see us blocking the release until we can get a solid repro test case.
Flags: wanted1.9.0.x+
Bug 428047 has the symptom that key commands don't work, people who think they are experiencing this should make sure they aren't experiencing that.
Is bug 428976 a dupe of this ?
Josh, I'm in a brand new window with about:blank. I don't think plugins are related, so that rules out bug 428047.

Phillipe, bug 428976 is indeed a dupe (by the sound of it).

Josh, perhaps you can explain how shortcut keys are supposed to work (or I guess I could go through your patch when I have some more time). I'm starting to suspect cmd+l in SeaMonkey (focus URL bar, not on a menu item, just a <key>) might be the key (no pun intended). If the idea is that we fake the menu high-lighting but all the actual |DoCommand()| work is done by the <key>s and their |command| / |oncommand| attributes, then I should start looking in that direction.

On the other hand, cmd+l could be a red herring and just independently broken (like cmd+shift+i, which suggests that all <key>s that aren't associated with menu items are broken).
Summary: cmd+key shortcuts no longer work after bug 398514 landed. → cmd+key shortcuts no longer work after bug 398514 landed (apple key shortcuts broken).
By the way, if cmd+l is a red herring (read: a separate bug) then we should file a separate bug on it. I duped bug 428976 since it mentioned "or any other shortcut".

Whoa. My shortcuts just started working again. In an instance in which they previously didn't. Let me hook up gdb and see what's going on.
And not just in this instance. Also in Minefield.

The only thing I've really done differently is that I launched and quit "Vine Server" (a vnc server). That might have reset something. Or it could've been something else.

Alright, off to gdb.
Ok, so previously (see comment 2) I was hitting menuItemHit twice, once with gMenuEffectsOnly true, once with it false. Now I hit it just once, with gMenuEffectsOnly true.

Uninitialized (return) value in a higher level method?
Or something else that would cause me to previously not go through the normal path (still not quite sure what that is) and fall back to calling |menuItemHit| (through |performKeyEquivalent|) a second time around?

Cmd+l now works too, which suggests that we're counting on <key>s to be handled properly (their command executed) and iff they happen to be associated with a menu item we'll try to do the high-light effect, as opposed to having the menu item execute the command.

I'd mark this bug WFM, except someone else has seen this, and anyway with my luck I'd just see the problem again tomorrow so I'd have to re-open this bug.

Something's definitely not working quite right. I'll see if I can figure out the toggle, though I'm kinda hoping maybe someone familiar with this code (looking at you, Josh) can give it another once over to see if maybe there is some uninitialized variable kinda thing going on here.
I see a lot of discussion about Cmd+l and would just like to reiterate that I haven't had had any trouble with any other key commands, including Cmd+l, *other* than Cmd+Left and Cmd+Right. If I'm experiencing a different problem, should a new bug be filed for it?
A Gaudio: it sounds like you are seeing a different problem, and a new bug would be useful.
Not sure why, but it's working for me again.  I'm using this version (it wasn't always working in this version):

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041404 Minefield/3.0pre

It seems to be random, but consistent over a long period of time.  I haven't changed anything about my current build, but it just started working correctly.

All these tested + working:

Apple+L, Apple+K, Apple+Left/Right, Apple+T
Well, the good news is that this stopped working for me again this morning.

What's interesting is that I haven't quit out of this process yet. So, it started with cmd+foo not working, then magically it started working, and just as magically it stopped working, all without a quit/crash -> restart.
You know what, this might actually be a really, really old bug. Ever since the switch from toolkit=mac to toolkit=cocoa I've seen cmd+l and cmd+shift+i stop and start working just as magically. Since all other shortcuts previously went through the native menu code I guess they weren't affected, but now that they use the same code[0], they are, and it's merely become a lot more visible.

[0] They do, right? Still haven't seen a yes or no on my guesses here ;-) [1]
[1] I know, I know, I should really just go read the patches and code.
Severity: blocker → major
Priority: -- → P2
OK, it started again for me.  It seems to start by the Javascript Debugger failing to recognize Apple-F, then spreads to the rest of the browser. 

After thinking about it, I realized that two out-of-the-ordinary things happened today:

1.  I updated Adium
2.  I told my laptop to sleep, then woke it up to work on it on battery, then slept it again and started it back on AC, all while Firefox was running.  Normally I work 100% on AC.

I'll fiddle around with sleep a bit and see if I can't get it to start failing.
This hasn't been a problem for a long time now. I know this got fixed at some point, but the details escape me -> WFM.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
FWIW, confirming that I no longer see this.
You need to log in before you can comment on or make changes to this bug.