398514 - Josh's cocoa widget key handling rewrite

Status: RESOLVED FIXED

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

Description

[Josh Aas]

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

Comment 1

[Josh Aas]

Attachment: cocoa key event tracing test app, v1.0

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.

Comment 2

[Josh Aas]

Attachment: fix v0.4

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.

Comment 3

[Josh Aas]

Attachment: fix v0.7

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.

Comment 4

[Josh Aas]

Attachment: menus only visually handle key events v1.0

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.

Comment 5

[Josh Aas]

Attachment: menus only visually handle key events v1.1

Comment 6

[Josh Aas]

Attachment: menus only visually handle key events v1.2

Comment 7

[Josh Aas]

Attachment: menus only visually handle key events v1.3

Updated to current trunk.

Comment 8

[Josh Aas]

Attachment: final fix v0.1

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

Comment 9

[Josh Aas]

Attachment: fix v0.2

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

Comment 10

[Josh Aas]

Attachment: final fix v0.5

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

Comment 11

[Josh Aas]

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.

Comment 12

[Josh Aas]

Attachment: final fix v0.6

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

Comment 13

[Josh Aas]

Attachment: no carbon command handling v1.0

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.

Comment 14

[Stuart Morgan]

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.

Comment 15

[Nick Kreeger]

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

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

I'm scared :(

Comment 17

[Damon Sicore (:damons)]

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

Comment 18

[Josh Aas]

Attachment: final fix v0.7

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

Comment 19

[Josh Aas]

Attachment: final fix v0.9

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.

Comment 20

[Josh Aas]

Attachment: final fix v0.9.1

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.

Comment 21

[Josh Aas]

Attachment: final fix v0.9.2

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.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 23

[Josh Aas]

Attachment: final fix v1.0

Comment 24

[Josh Aas]

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

Comment 25

[Steven Michaud [:smichaud] (Retired)]

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 :-)

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

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

Comment 27

[Josh Aas]

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

Comment 28

[jag (Peter Annema)]

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.

Comment 29

[jag (Peter Annema)]

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.

Comment 30

[Josh Aas]

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.

Comment 31

[jag (Peter Annema)]

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

Comment 32

[jag (Peter Annema)]

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.

Comment 33

[jag (Peter Annema)]

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

Comment 34

[jag (Peter Annema)]

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.

Comment 35

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

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

Comment 36

[jag (Peter Annema)]

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

Comment 37

[jag (Peter Annema)]

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.

Comment 38

[jag (Peter Annema)]

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

Comment 39

[jag (Peter Annema)]

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.

Comment 40

[jag (Peter Annema)]

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

Comment 41

[jag (Peter Annema)]

I filed bug 428760 on this issue.