Josh Aas Assignee
	Description • 17 years ago

I'm rewriting the way we handle key events in Cocoa widgets, what we have now is broken in quite a few ways.

	Josh Aas Assignee
	Comment 1 • 17 years ago

This is a little cocoa app that I use to test the path of cocoa key events. As it is in this simple form you can learn a lot, but with little tweaks and more log statements it is easy to learn a lot more. Look at console output when running it. Try different types of key input like "a" vs. "cmd-a" vs. "ctrl-a" vs. "F1" vs. "ctrl-opt-a", stuff like that.

Anyway, it might be that nobody else is interested in this but I want it backed up here for me if nothing else.

	Josh Aas Assignee
	Comment 2 • 17 years ago

Putting this up as a backup. I'm beyond this now but this is a good place for me to fall back to if things go wrong, only pKE changes.

	Josh Aas Assignee
	Comment 3 • 17 years ago

This fixes bugs 358379 and 374076. It also fixes a huge number of other problems, way too much for me to list specifically here. Here is a general list:

1. For whole classes of key events, we only send key up or key down events into gecko, not both. Fixed in my patch.
2. Sometimes we only send key down events into gecko when we should potentially also be sending a key press. Fixed in my patch.
3. Often we send incorrectly constructed events into gecko because we're constructing a gecko event out of a different type of native event and the conversion routines do the wrong thing. Fixed in my patch.
4. Same as above, but we send incorrectly constructed carbon events to plugins. Fixed in my patch.
5. Cocoa just doesn't give us some key events, fixed or worked around in my patch.

I still have more cleanup and testing to do, this is just another backup.

	Josh Aas Assignee
	Comment 4 • 17 years ago

This patch makes our menu system appear to handle key equivs but not actually do anything. This might be part of my final solution, posting it here as a backup.

	Josh Aas Assignee
	Comment 7 • 16 years ago

Updated to current trunk.

	Josh Aas Assignee
	Comment 8 • 16 years ago

This is a fix for bug 382138 and bug 418334, the last of the major work I planned to do in my key rewrite that hasn't taken place in other bugs. More work to do on this patch:
- application menu items don't behave correctly, often don't work
- hidden window keyboard commands don't work
- need to remove key command hashing system, unused now

	Josh Aas Assignee
	Comment 9 • 16 years ago

- application menu items don't behave correctly, often don't work
- hidden window keyboard commands don't work

	Josh Aas Assignee
	Comment 10 • 16 years ago

Remaining issues:
- application menu items don't behave correctly, often don't work

	Josh Aas Assignee
	Comment 11 • 16 years ago

Note that "final fix v0.5" has a nasty crash caused by an exception being thrown. Can reproduce by very quickly hitting "cmd-w" and "cmd-n" repeatedly to close and open a window.

	Josh Aas Assignee
	Comment 12 • 16 years ago

This moves the nsAutoRetainView class to nsCocoaUtils.h and makes it generic, can be used for any Cocoa object. Not actually used in this patch in any ways it wasn't before, but should be done anyway.

Remaining issues:
- crash described in comment #11
- application menu items don't behave correctly, often don't work

	Josh Aas Assignee
	Comment 13 • 16 years ago

In order to make my patches marked "final fix" work with the application menu consistently, we need to not use carbon events to handle menu commands. This patch gets rid of our carbon-based command handling in favor of pure Cocoa.

	Stuart Morgan
	Comment 14 • 16 years ago

Comment on attachment 313685 [details] [diff] [review]
no carbon command handling v1.0

>+      // Can't do anything if we don't know the last painted menu bar.
>+      if (!menuBar)
>+        return;
>+      // given the commandID, look it up in our hashtable and dispatch to
>+      // that content node. Recall that we store weak pointers to the content
>+      // nodes in the hash table.
>+      nsIMenuItem* content = menuBar->GetMenuItemForCommandID(static_cast<PRUint32>(tag));
>+      if (content)
>+        content->DoCommand();
>+      break;

Since none of the other cases early-return, I'd do this as
  if (menuBar) {
   ...
  }
  break;

but r=me either way.

	Nick Kreeger
	Comment 15 • 16 years ago

Comment on attachment 313685 [details] [diff] [review]
no carbon command handling v1.0

looks good to me, I like smorgans comment about the early return....

r=me

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 16 • 16 years ago

Comment on attachment 313685 [details] [diff] [review]
no carbon command handling v1.0

I'm scared :(

	Damon Sicore (:damons)
	Comment 17 • 16 years ago

Marking this as blocking.  It's required for blocker bug 418334.

	Josh Aas Assignee
	Comment 18 • 16 years ago

Updated to current trunk. Remaining issues:
- Crash I noted above still exists.
- Some app-global menu items have their commands invoked twice.

	Josh Aas Assignee
	Comment 19 • 16 years ago

This fixes bugs 382138, 413681, and 418334. This has only one problem, which is the crash mentioned above. It doesn't happen often under normal usage, but it is easy to reproduce by quickly opening and closing the only browser window in Firefox via cmd-o and cmd-w.

	Josh Aas Assignee
	Comment 20 • 16 years ago

Fixes a compiler warning on 10.5, still has the crash bug.

I should also note that this patch comes with some sacrifices in menu system feedback. Menu flashing will continue to work, but disabled command beeping does not always work, even for some common cases like cmd-c in a text field with no selection. Because we're going to allow commands to work even though they have a disabled native menu item and we have no way of knowing whether or not the command did anything, we can't beep in many cases where a command didn't do anything (to be clear, we used to beep because the menu item was disabled). The version of WebKit in Safari 3.1 appears to have the same issue, though they don't suffer from it when the user is interacting with native Cocoa chrome. Over time we might be able to do better but for now I think we have no choice but to accept this.

	Josh Aas Assignee
	Comment 21 • 16 years ago

This wraps the call that throws the fatal exception with a non-fatal exception handler. Not ideal, and at this point I'm not suggesting we go for it, but it could turn out to be a good enough option if we don't find another fix reasonably quickly.

I didn't see any problems with the app after ignoring the exception, and the fact that my debugging indicates it comes from a cache is somewhat encouraging. It is possible that the exception is a valid response to an incorrect cache query. This is guesswork though because I don't have Apple's source, I'd like to have more evidence of safety before resorting to this solution.

	Steven Michaud [:smichaud] (Retired)
	Comment 22 • 16 years ago

Josh, there's no need to paper over the NSException crash.  I've found
a fix.

The first part of the fix is to change some code in [ChildView
performKeyEquivalent:] (not part of your patch) from this:

  // if we aren't the first responder, pass the event on
  if ([[self window] firstResponder] != self)
    return [super performKeyEquivalent:theEvent];

to this:

  // if we aren't the first responder, pass the event on
  id firstResponder = [[self window] firstResponder];
  if (firstResponder != self) {
    if ([firstResponder isKindOfClass:[ChildView class]]) {
      return [(ChildView *)firstResponder performKeyEquivalent:theEvent];
    } else {
      return [super performKeyEquivalent:theEvent];
    }
  }

This call to super is where all the Objective-C exceptions that I saw
took place (an out-of-bounds error reading an NSCFArray object).
Clearly the second block of code is more correct than the first, which
doesn't account for the possibility that firstResponder is also a
ChildView object.  I don't know precisely the steps that led up to the
exception.  But I suspect the crucial factor is that the superclass's
performKeyEquivalent: method doesn't have a way to bail out when
firstResponder is being destroyed (when it's mGeckoChild object is
nil).

The specific exception was an out-of-bounds error trying to access an
NSCFArray object's item at index 1, when that array had only one item
in it (a ChildView object).  Shortly prior to this call the array had
two objects (both ChildView objects).  But the first one got deleted
and the calling code didn't check again before trying to access the
"second" one.  (I found all this out by using method swizzling to hook
the NSCFArray class's objectAtIndex: and removeObjectAtIndex:
methods.)

Once I'd fixed this problem, though, I started seeing a _second_
Objective-C exception (probably unrelated) :-)

This one I fixed by adding the following code to your patch's
[GeckoNSMenu performKeyEquivalent:] method (just below the assertion):

  // Yes, we have no menu items.
  if ([self numberOfItems] <= 0)
    return NO;

It seems that the OS can call this method even when the GeckoNSMenu
object has no more menu items (presumably as it is being destroyed).
It's not being called _after_ the GeckoNSMenu object's destruction,
though (I checked).

If you want, Josh, I can post a tryserver build made from your v0.9.1
patch plus my fix.

	Josh Aas Assignee
	Comment 24 • 16 years ago

"final fix v1.0" integrates Steven's fixes, the work for me. Thanks Steven!

	Steven Michaud [:smichaud] (Retired)
	Comment 25 • 16 years ago

Comment on attachment 314215 [details] [diff] [review]
final fix v1.0

Looks good to me.  I did some testing and had no problems.

I'm _amazed_ how many big nasty bugs this patch fixes :-)

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 26 • 16 years ago

Comment on attachment 314215 [details] [diff] [review]
final fix v1.0

Looks ok here, at least as far as I understand things.

	Josh Aas Assignee
	Comment 27 • 16 years ago

landed on trunk, closing this out, track further issues in their own bugs

	jag (Peter Annema)
	Comment 28 • 16 years ago

I'll try to do a binary search tonight to find the exact patch that broke cmd+2, cmd+n, cmd+a, cmd+c, cmt+t, cmd+w, etc. etc. (everywhere, not just in Mail/News), but since this seems the most likely one I figured I'd give you a heads up.

Mac OS X 10.4.11 with SeaMonkey trunk.

cmd+~ and cmd+q work, as do cmd+left/right, ctrl+page-up/down. When I issue any of the cmd+key combos from the menu the relevant top-level menu item hi-lights as expected, and no beep. For cmd+l (select url-bar contents, not associated with a menu item) I get a beep.

	jag (Peter Annema)
	Comment 29 • 16 years ago

Alright, so cmd+a, cmd+c, cmd+t, cmd+w, etc. not working is definitely this patch.

Cmd+l is having issues even with this patch backed out.

	Josh Aas Assignee
	Comment 30 • 16 years ago

This patch exposed a bug which will be fixed by the patch in bug 359638, that should fix keyboard commands like cmd+c on certain keyboard layouts.

	jag (Peter Annema)
	Comment 31 • 16 years ago

Josh: I have a pretty standard qwerty 104-key keyboard, so I don't think that's it.

	jag (Peter Annema)
	Comment 32 • 16 years ago

Josh: do you have a 10.4 system you can test on, or some pointers to strategically placed printfs that'll help you figure out why the command doesn't get executed? Like I said, the proper menu bar menu items high-light, so the shortcut is hooked up to the Mac menu system at least. And clicking on the menu item works.

	jag (Peter Annema)
	Comment 33 • 16 years ago

Hmm, interesting detail: cmd+option+t (Edit -> Special Characters) does work.

	jag (Peter Annema)
	Comment 34 • 16 years ago

And just to be clear, this isn't a SeaMonkey only bug. Firefox trunk (Minefield) on 10.4 is affected as well.

I'll try some nightly builds next, maybe it's something in my build environment (NB: cvs diff -u comes up empty), since no one else seems to be complaining.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 35 • 16 years ago

Peter, can you give a list of shortcuts which don't work with a trunk build of Firefox? Everything you mentioned above is WFM with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041104 Minefield/3.0pre ID:2008041104

	jag (Peter Annema)
	Comment 36 • 16 years ago

Henrik: pretty much all of them: cmd+n, cmd+t, cmd+l, cmd+o, cmd+shift+w, cmd+w, cmd+p, cmd+z, cmd+shift+z, cmd+x, cmd+c, cmd+v, cmd+a, cmd+f, cmd+g, cmd+b, cmd+shift+h, cmd+., cmd+r, cmd+-, cmd++ (cmd+shift+=), cmd+0, cmd+u, cmd+[, cmd+], cmd+d, cmd+shift+d, cmd+shift+b, cmd+k, cmd+j, cmd+shift+j, cmd+shift+i, cmd+i, cmd+m, cmd+? (cmd+shift+/).

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

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

checkout start: Fri Apr 11 11:11:58 PDT 2008

	jag (Peter Annema)
	Comment 37 • 16 years ago

With a nightly build of Minefield I see the same, so it's not my build env.

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

I tried creating a new profile, no luck.

I suspected it was a 10.4 vs 10.5 thing, but since it works for Henrik I guess I'll have to look onward some more.

My keyboard has a P/S2 plug hooked up through a P/S2-to-USB converter to my Intel Mac Mini, but is a run of the mill keyboard with a standard keyboard layout, _and_ it worked just fine until "final fix v1.0" landed.

	jag (Peter Annema)
	Comment 38 • 16 years ago

Oh, and I'm using a standard U.S. layout.

	jag (Peter Annema)
	Comment 39 • 16 years ago

I probably sound a bit grumpy, so let me clarify that I think this rewrite was the right thing to do and I wanna thank Josh for all the hard work he's put into it. I'm currently doing a build with the patch from bug 359638, just in case it does fix my problem, and if that doesn't I'm confident we'll figure out what's going on and get it fixed.

	jag (Peter Annema)
	Comment 40 • 16 years ago

Well, the patch from bug 359638 doesn't solve my problem.

	jag (Peter Annema)
	Comment 41 • 16 years ago

I filed bug 428760 on this issue.