584965 - improve key event handling for Cocoa widgets, follow Cocoa event propagation model properly

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

The way we handle key events in Cocoa widgets right now is pretty seriously flawed in terms of the event propagation model. We completely break the standard native key event propagation model, to the point that for many things we make (bad) guesses about how the event will propagate within our own system. We haven't had a better strategy and we were able to get things working well enough to release before, but that might not be the case any more.

I spent some time figuring out how things should work, this bug will track my work on a key event patch that makes our key event handling work with the standard event propagation model.

Comment 1

[Josh Aas]

Attachment: fix v0.8

This has some issues that still need to be resolved but it basically works and the strategy is pretty clear in the code.

Comment 2

[Josh Aas]

Attachment: fix v0.9

Most things work at this point, the only major problem is that key equivalents always make a beeping noise (they work though).

Comment 3

[Josh Aas]

Attachment: fix v0.95

Comment 4

[Josh Aas]

Attachment: fix v1.0

This fixes the bugs I know about. It entirely removes the ChildView override of "performKeyEquivalent:" and all of its nasty associated code. Events now propagate properly.

Comment 5

[Josh Aas]

Attachment: fix v1.0 (updated to current trunk)

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 463991 [details] [diff] [review]
fix v1.0 (updated to current trunk)

This patch (basically) looks good to me.  But (in its current state)
it breaks Ctrl+Tab (used to change tabs) and probably also Ctrl+Esc
(currently used for nothing, as far as I can tell).  I don't think we
can land a patch that breaks commonly used functionality, so I'm r-ing
it (in its current state).

The Ctrl+Tab and Ctrl+Esc breakage is explained by the fact that, even
with the current patch, [ChildView keyDown:] is never called when
pressing either of these key combinations (only [ChildView keyUp:]).
This is probably an OS bug (especially considering that it only
happens on OS X 10.5 and up).  We used to work around it in [ChildView
performKeyEquivalent:] (which this patch removes).  I don't know
whether or not we'll have to (partially) restore [ChildView
performKeyEquivalent:] to continue working around it.

By the way (as I assume you're aware), this patch doesn't fix bug
429824.  Do you have any ideas about how we might fix it?

(From comment #0)

> We completely break the standard native key event propagation model,
> to the point that for many things we make (bad) guesses about how
> the event will propagate within our own system.

It'd be nice to have a more detailed explanation of what the native
key event propagation model is, and how we broke it.

Comment 7

[Josh Aas]

Attachment: fix v1.1

Nice catch Steven. Turns out the problem of getting ctrl-tab events is not unique to us and the solution is an SPI on NSView called "-(BOOL)_wantsKeyDownForEvent:(NSEvent*)event". This patch implements that and it solves the problem.

Comment 8

[Josh Aas]

I don't have any new ideas about fixing bug 429824, I don't have time to look into that right now. I'm suspect whatever the solution is this patch will make it easier to figure out.

As for how our existing implementation breaks the normal event propagation model... We basically have duplicate event processing code in ChildView's "performKeyEquivalent:" and "keyDown:". This shouldn't be necessary and the "performKeyEquivalent" version is buggy. We return inconsistent values from ChildView's "performKeyEquivalent:" because we're not sure what is going to be returned from the menu system's "performKeyEquivalent:", and we should never have to do that guessing. Here is the crux of that bad guessing, from ChildView's "performKeyEquivalent:":

  // With Cmd key or Ctrl+Tab or Ctrl+Esc, keyDown will be never called.
  // Therefore, we need to call processKeyDownEvent from performKeyEquivalent.
  UInt32 keyCode = [theEvent keyCode];
  PRBool keyDownNeverFiredEvent = (modifierFlags & NSCommandKeyMask) ||
           ((modifierFlags & NSControlKeyMask) &&
            (keyCode == kEscapeKeyCode || keyCode == kTabKeyCode));

This is bogus, especially the "Cmd key" part. If the menu system returns "NO" for "performKeyEquivalent" then command key events go to ChildView's "keyDown:" just fine.

Another problem with our approach is bug 379199 - not allowing events to propagate properly past ChildView's "performKeyEquivalent:" messes up embedding.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 464272 [details] [diff] [review]
fix v1.1

This looks fine to me.  Ctrl+Tab and Ctrl+Shift+Tab now work.  And
Ctrl+Esc now triggers calls to both [ChildView keyDown:] and
[ChildView keyUp:].

> I don't have any new ideas about fixing bug 429824, ...  I suspect
> whatever the solution is this patch will make it easier to figure
> out.

Thanks for the info.  I suspect you're right.

> As for how our existing implementation breaks the normal event
> propagation model... We basically have duplicate event processing
> code in ChildView's "performKeyEquivalent:" and "keyDown:". This
> shouldn't be necessary and the "performKeyEquivalent" version is
> buggy. We return inconsistent values from ChildView's
> "performKeyEquivalent:" because we're not sure what is going to be
> returned from the menu system's "performKeyEquivalent:", and we
> should never have to do that guessing.

Once again, thanks for the info.

> Here is the crux of that bad guessing, from ChildView's
> "performKeyEquivalent:":
>
>  // With Cmd key or Ctrl+Tab or Ctrl+Esc, keyDown will be never called.
>  // Therefore, we need to call processKeyDownEvent from performKeyEquivalent.
>  UInt32 keyCode = [theEvent keyCode];
>  PRBool keyDownNeverFiredEvent = (modifierFlags & NSCommandKeyMask) ||
>           ((modifierFlags & NSControlKeyMask) &&
>            (keyCode == kEscapeKeyCode || keyCode == kTabKeyCode));
>
> This is bogus, especially the "Cmd key" part. If the menu system
> returns "NO" for "performKeyEquivalent" then command key events go
> to ChildView's "keyDown:" just fine.

Your last statement is false -- making the menu system return "NO" for
performKeyEquivalent: isn't enough to make Cmd, Ctrl+Tab or Ctrl+Esc
go through [ChildView keyDown:]

But Cmd key events (like Shift, Ctrl and Alt key events) all go
through [ChildView flagsChanged:], which in turn calls
fireKeyEventForFlagsChanged:.  And your override of
_wantsKeyDownForEvent: makes Ctrl+Tab and Ctrl+Esc go through
keyDown:.  So, as of your current patch, there no longer appears to be
any need to override performKeyEquivalent: (to have a [ChildView
performKeyEquivalent:] method).

Comment 10

[Josh Aas]

Marking as blocking beta 4 because this is important, it affects the Jetpack SDK's Widget API and I'd prefer this go in earlier rather than later for testing purposes.

We already have tests in the tree for this code, more are in the works in bug 582052 (and those cases have been manually tested already).

Comment 11

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/33f8c2bb77ca

Comment 12

[Josh Aas]

backed out because of regressions

http://hg.mozilla.org/mozilla-central/rev/2c69cdb5a98c
http://hg.mozilla.org/mozilla-central/rev/0ee09dea0911

Comment 13

[Daniel Holbert [:dholbert]]

Odd as it is, the backout appears to have caused an orange on OS X64 Bd:

The log from comment 12's push is:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281729093.1281734779.8933.gz
and the next cycle is:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281730617.1281736240.15695.gz
(same failure)

The error is:
{
>###!!! ASSERTION: Fastload file should have been closed via xpcom-shutdown: '!mFastLoadTimer', file /builds/slave/mozilla-central-macosx64-debug/build/js/src/xpconnect/loader/mozJSComponentLoader.cpp, line 570
mozJSComponentLoader::~mozJSComponentLoader()+0x000000B0 [/builds/slave/mozilla-central-macosx64-debug/build/obj-firefox/dist/bin/XUL +0x00F0EE10]
>mozJSComponentLoader::Release()+0x000000FB [/builds/slave/mozilla-central-macosx64-debug/build/obj-firefox/dist/bin/XUL +0x00F0A5E5]
>nsCOMPtr<mozilla::ModuleLoader>::~nsCOMPtr()+0x00000054 [/builds/slave/mozilla-central-macosx64-debug/build/obj-firefox/dist/bin/XUL +0x01558902]
>nsComponentManagerImpl::KnownModule::~KnownModule()+0x00000042 [/builds/slave/mozilla-central-macosx64-debug/build/obj-firefox/dist/bin/XUL +0x01558946]
>nsAutoPtr<nsComponentManagerImpl::KnownModule>::~nsAutoPtr()+0x00000027 [/builds/slave/mozilla-central-macosx64-debug/build/obj-firefox/dist/bin/XUL +0x0155ACAD]
}

Comment 14

[Daniel Holbert [:dholbert]]

Not to worry -- the next cycle after the logs from comment 13 was green, so it looks like we just got really (un)lucky and had two instances of an as-yet-unreported randomorange in a row.  Filed bug 587263 on that issue.

Comment 15

[Josh Aas]

Attachment: fix v1.2

This fixes a number of problems with the previous patch and tests are now integrated with the patch. The only remaining bug to fix before this is ready to go again is bug 587058.

Comment 16

[Josh Aas]

Masayuki - bug 587058 is a problem with my patch here that is caused by your code from bug 422888. I don't understand that logic and I don't have a way to test it. Can you take a look?

Specifically, "Cmd+Opt+F" does not work properly because "processKeyDown:" doesn't send a key press for it. It doesn't send the key press because of the code from bug 422888.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> 4059 static PRBool IsNormalCharInputtingEvent(const nsKeyEvent& aEvent)
> 4060 {
> 4061   // this is not character inputting event, simply.
> 4062   if (!aEvent.isChar || !aEvent.charCode)
> 4063     return PR_FALSE;
> 4064   // if this is unicode char inputting event, we don't need to check
> 4065   // ctrl/alt/command keys
> 4066   if (aEvent.charCode > 0x7F)
> 4067     return PR_TRUE;
> 4068   // ASCII chars should be inputted without ctrl/alt/command keys
> 4069   return !aEvent.isControl && !aEvent.isAlt && !aEvent.isMeta;
> 4070 }

I think that you can return PR_FALSE at first |if| statement if aEvent.isMeta is TRUE. Because if Cmd key is pressed insertText should have been never sent.

Then, you can remove |&& !aEvent.isMeta| from the last return statement.

This doesn't break the bug 422888's issue.

Note that the fix of bug 422888 is still hacky, I wonder why interpretKeyEvents doesn't have return value...

Comment 18

[Josh Aas]

Attachment: fix v1.3

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 466348 [details] [diff] [review]
fix v1.3

This looks fine to me.

I compiled it an did some testing, and didn't see any problems.
Ctrl+Tab and Ctrl+Shift+Tab work fine.  So do Cmd+, (Preferences) and
Cmd+Alt+F (Web Search).

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we need a tryserver run on this before landing it?

Comment 21

[Josh Aas]

Try server run is done, just waiting to land.

Comment 22

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3a8430b73a91

Comment 23

[juan becerra [:juanb]]

I've tried several of the keyboard shortcuts here and I don't see any problems. I specifically tried to reproduce the problem from bug 580789, and I was not able to.