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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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.
Posted patch fix v0.8 (obsolete) — Splinter Review
This has some issues that still need to be resolved but it basically works and the strategy is pretty clear in the code.
Assignee: nobody → joshmoz
Blocks: 582052, 379199
Posted patch fix v0.9 (obsolete) — Splinter Review
Most things work at this point, the only major problem is that key equivalents always make a beeping noise (they work though).
Attachment #463461 - Attachment is obsolete: true
Posted patch fix v0.95 (obsolete) — Splinter Review
Attachment #463470 - Attachment is obsolete: true
Posted patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #463656 - Attachment is obsolete: true
Attachment #463689 - Flags: review?(smichaud)
Attachment #463689 - Flags: review?(b56girard)
Attachment #463689 - Attachment is obsolete: true
Attachment #463991 - Flags: review?(b56girard)
Attachment #463689 - Flags: review?(smichaud)
Attachment #463689 - Flags: review?(b56girard)
Attachment #463991 - Flags: review?(smichaud)
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.
Attachment #463991 - Flags: review?(smichaud) → review-
Attachment #463991 - Attachment is obsolete: true
Attachment #463991 - Flags: review?(b56girard)
Posted patch fix v1.1 (obsolete) — Splinter Review
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.
Attachment #464272 - Flags: review?(smichaud)
Attachment #464272 - Flags: review?(b56girard)
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.
Status: NEW → ASSIGNED
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).
Attachment #464272 - Flags: review?(smichaud) → review+
Blocks: 580789
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).
blocking2.0: --- → beta4+
Attachment #464272 - Flags: review?(b56girard)
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/33f8c2bb77ca
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 586966
backed out because of regressions

http://hg.mozilla.org/mozilla-central/rev/2c69cdb5a98c
http://hg.mozilla.org/mozilla-central/rev/0ee09dea0911
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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]
}
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.
Blocks: 581608
Posted patch fix v1.2 (obsolete) — Splinter Review
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.
Attachment #464272 - Attachment is obsolete: true
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.
> 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...
Posted patch fix v1.3Splinter Review
Attachment #466181 - Attachment is obsolete: true
Attachment #466348 - Flags: review?(smichaud)
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).
Attachment #466348 - Flags: review?(smichaud) → review+
Do we need a tryserver run on this before landing it?
Try server run is done, just waiting to land.
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3a8430b73a91
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Depends on: 613710
You need to log in before you can comment on or make changes to this bug.