Josh's cocoa widget key handling rewrite

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

(Depends on 1 bug)

Trunk
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 14 obsolete attachments)

41.30 KB, application/octet-stream
Details
15.93 KB, patch
stuart.morgan+bugzilla
: review+
nick.kreeger
: review+
Details | Diff | Splinter Review
38.40 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Assignee

Description

12 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.
Assignee

Updated

12 years ago
Blocks: 358379
Assignee

Comment 1

12 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.
Assignee

Comment 2

12 years ago
Posted patch fix v0.4 (obsolete) — Splinter Review
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.
Assignee

Updated

12 years ago
Blocks: 374076
Assignee

Updated

12 years ago
Blocks: 379199
Assignee

Comment 3

12 years ago
Posted patch fix v0.7 (obsolete) — Splinter Review
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.
Attachment #283521 - Attachment is obsolete: true
Assignee

Comment 4

12 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.
Assignee

Comment 5

12 years ago
Attachment #284497 - Attachment is obsolete: true
Assignee

Comment 6

12 years ago
Attachment #284662 - Attachment is obsolete: true
Blocks: 382138
Hardware: PC → All
Assignee

Updated

11 years ago
Blocks: 418334
Assignee

Comment 7

11 years ago
Updated to current trunk.
Attachment #292541 - Attachment is obsolete: true
Assignee

Comment 8

11 years ago
Posted patch final fix v0.1 (obsolete) — Splinter Review
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
Attachment #283680 - Attachment is obsolete: true
Assignee

Comment 9

11 years ago
Posted patch fix v0.2 (obsolete) — Splinter Review
- application menu items don't behave correctly, often don't work
- hidden window keyboard commands don't work
Attachment #313366 - Attachment is obsolete: true
Assignee

Comment 10

11 years ago
Posted patch final fix v0.5 (obsolete) — Splinter Review
Remaining issues:
- application menu items don't behave correctly, often don't work
Attachment #313367 - Attachment is obsolete: true
Assignee

Comment 11

11 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.
Assignee

Comment 12

11 years ago
Posted patch final fix v0.6 (obsolete) — Splinter Review
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
Attachment #313373 - Attachment is obsolete: true
Assignee

Comment 13

11 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.
Attachment #313685 - Flags: review?(smichaud)

Comment 14

11 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.
Attachment #313685 - Flags: review?(smichaud) → review+
Assignee

Updated

11 years ago
Attachment #313685 - Flags: superreview?(roc)

Comment 15

11 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
Attachment #313685 - Flags: review+
Assignee

Updated

11 years ago
Attachment #313685 - Flags: superreview?(roc) → superreview?(vladimir)
Comment on attachment 313685 [details] [diff] [review]
no carbon command handling v1.0

I'm scared :(
Attachment #313685 - Flags: superreview?(vladimir) → superreview+
Assignee

Updated

11 years ago
Attachment #313685 - Flags: approval1.9?
Marking this as blocking.  It's required for blocker bug 418334.
Flags: blocking1.9+
Priority: -- → P2
Assignee

Updated

11 years ago
Attachment #313685 - Flags: approval1.9?
Assignee

Updated

11 years ago
Blocks: 413681
Assignee

Comment 18

11 years ago
Posted patch final fix v0.7 (obsolete) — Splinter Review
Updated to current trunk. Remaining issues:
- Crash I noted above still exists.
- Some app-global menu items have their commands invoked twice.
Attachment #313397 - Attachment is obsolete: true
Assignee

Updated

11 years ago
No longer blocks: 379199
Assignee

Comment 19

11 years ago
Posted patch final fix v0.9 (obsolete) — Splinter Review
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.
Attachment #313357 - Attachment is obsolete: true
Attachment #314001 - Attachment is obsolete: true
Assignee

Comment 20

11 years ago
Posted patch final fix v0.9.1 (obsolete) — Splinter Review
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.
Attachment #314014 - Attachment is obsolete: true
Assignee

Comment 21

11 years ago
Posted patch final fix v0.9.2 (obsolete) — Splinter Review
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.
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.
Assignee

Comment 23

11 years ago
Attachment #314086 - Attachment is obsolete: true
Attachment #314149 - Attachment is obsolete: true
Attachment #314215 - Flags: review?(smichaud)
Assignee

Comment 24

11 years ago
"final fix v1.0" integrates Steven's fixes, the work for me. Thanks Steven!
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 :-)
Attachment #314215 - Flags: review?(smichaud) → review+
Assignee

Updated

11 years ago
Attachment #314215 - Flags: superreview?(vladimir)
Comment on attachment 314215 [details] [diff] [review]
final fix v1.0

Looks ok here, at least as far as I understand things.
Attachment #314215 - Flags: superreview?(vladimir) → superreview+
Assignee

Comment 27

11 years ago
landed on trunk, closing this out, track further issues in their own bugs
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 427932

Updated

11 years ago
Blocks: 427995
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.

Updated

11 years ago
Depends on: 427995
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.
Assignee

Comment 30

11 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.
Josh: I have a pretty standard qwerty 104-key keyboard, so I don't think that's it.
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.
Hmm, interesting detail: cmd+option+t (Edit -> Special Characters) does work.
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.
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
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
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.
Oh, and I'm using a standard U.S. layout.
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.
Well, the patch from bug 359638 doesn't solve my problem.
Depends on: 428062
I filed bug 428760 on this issue.

Updated

11 years ago
Depends on: 429824
No longer depends on: 428062
You need to log in before you can comment on or make changes to this bug.