445531 - [10.4] Esc key doesn't cancel out Ctrl+Tab panel

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1pre) Gecko/2008071609 Minefield/3.1a1pre

Steps to reproduce:
1. Make sure there are multiple tabs open.
2. While holding Ctrl, press Tab and then Esc.

Result: Esc is ignored, and if I want to not switch to a different tab, I have to keep pressing Tab to find the tab I was on initially.

Comment 1

[Dão Gottwald [:dao]]

Seems like an OS X only bug. On Linux this works:

  onKeyDown: function (event) {
...
    switch (event.keyCode) {
...
      case event.DOM_VK_ESCAPE:
        if (isOpen)
          this.panel.hidePopup();

On Windows this code isn't even needed because Ctrl+Esc opens the Start menu, which closes the panel. (The user doesn't want the Start menu in this case, though, which is a bug of its own.)

Comment 2

[Dão Gottwald [:dao]]

Maybe using keypress instead of keydown would fix this. Not sure.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Esc while holding ctrl shows also the Windows start menu, at least on this XP SP3 laptop, and this menu remains on the screen after releasing ctrl.

Comment 4

[Dão Gottwald [:dao]]

Is this still an issue?

Comment 5

[Marcia Knous [:marcia]]

Using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080812022114 Minefield/3.1a2pre, if I follow Jesse's steps when I press ESC it closes the Ctrl-Tab panel. However, using 10.4 I still see what Jesse sees using the equivalent build.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

This bug is caused by a Tiger-specific Apple bug.  But I've found a
way to work around it.

When you press ctrl-ESC (as you do in step 2 of comment #0's STR), the
OS treats this as a keyboard shortcut potentially corresponding to a
particular command.  As a result, the OS invokes performKeyEquivalent:
on various objects (including NSMenu, NSWindow, NSView and ChildView
objects), passing the ctrl-ESC key event as a parameter.

This works fine on Leopard (OS X 10.5.X).  But on Tiger, the OS messes
up the event passed as performKeyEquivalent:'s parameter -- it zeroes
out most of its fields, including keyCode and modifierFlags.

[ChildView performKeyEquivalent:] contains code to special-case the
ctrl-ESC key combination -- passing it to Gecko as if it were an
ordinary NSKeyDown event.  This code works fine on Leopard.  But on
Tiger (thanks to the problems described above) it doesn't recognize
the ctrl-ESC event for what it is, and therefore doesn't process it
correctly.

I'll attach my patch in the next comment.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix (work around Tiger-specific Apple bug)

My patch works around the Apple bug (described in comment #6) by wrapping all
calls to [NSEvent keyCode] and [NSEvent modifierFlags] in new nsCocoaUtils
methods -- GetCocoaEventKeyCode() and GetCocoaEventModifierFlags().  These
methods know what to do with a defective ctrl-ESC event, but otherwise just
pass their calls through to the underlying methods.

I needn't have wrapped every call to either [NSEvent keyCode] or
(particularly) [NSEvent modifierFlags].  But I figure doing so will make the
code more readable and easier to maintain.

nsChildView.mm contains one call to the Carbon GetCurrentEventKeyModifiers()
method.  I'm quite sure this method isn't effected by the Apple bug, so I
haven't modified it.  What I say in my next comment should make clear that the
Apple bug is specifically a Cocoa bug.

With luck, a tryserver build should follow in an hour or two.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

The (Tiger-specific) Apple bug that I've referred to in the previous comments
happens the (undocumented) [NSWindow _cancelKey:], when it's called
(indirectly) from [NSWindow sendEvent:] on a ctrl-ESC NSKeyDown event.

This method calls [NSEvent keyEventWithType:...] to synthesize another
NSKeyDown event, and then calls various objects' performKeyEquivalent: methods
(including [ChildView performKeyEquivalent:]) with that synthesized NSKeyDown
event as their 'theEvent' parameter.  But this event was synthesized
incorrectly -- most of its fields are 0, including keyCode and modifierFlags.

Here's a gdb stack trace of [NSWindow _cancelKey:]'s call to [NSEvent
keyEventWithType:...]:

#0  0x9360e5b3 in +[NSEvent keyEventWithType:location:modifierFlags:
                  timestamp:windowNumber:context:characters:
                  charactersIgnoringModifiers:isARepeat:keyCode:] ()
#1  0x9372aee1 in -[NSWindow _cancelKey:] ()
#2  0x934fa1f9 in -[NSWindow _processKeyboardUIKey:] ()
#3  0x9334b34b in -[NSWindow sendEvent:] ()
#4  0x017df024 in -[NSWindow(MethodSwizzling)
                  nsCocoaWindow_NSWindow_sendEvent:] ()
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsCocoaWindow.mm:2198
#5  0x017d9700 in -[ToolbarWindow sendEvent:]
                  (self=0x34070930, _cmd=0x90ac4484, anEvent=0x35cdc550)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsCocoaWindow.mm:1857
#6  0x9333cd8c in -[NSApplication sendEvent:] ()
#7  0x932678e7 in -[NSApplication run] ()
#8  0x017d71a9 in nsAppShell::Run (this=0x1b620d00)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsAppShell.mm:591
#9  0x0169de01 in nsAppStartup::Run (this=0x1b63ff80)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/toolkit/components/
       startup/src/nsAppStartup.cpp:182
#10 0x0100fde2 in XRE_main (argc=1, argv=0xbffff9e0, aAppData=0x1b60c670)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/toolkit/xre/
       nsAppRunner.cpp:3220
#11 0x00002ce1 in main (argc=1, argv=0xbffff9e0)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/browser/app/
       nsBrowserApp.cpp:156

Here's a stack trace of the subsequent call to [ChildView
performKeyEquivalent:] with a "bad" theEvent parameter:

#0  -[ChildView performKeyEquivalent:]
    (self=0x340b5280, _cmd=0x90abc8b0, theEvent=0x2cefbb60)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsChildView.mm:5467
#1  0x9335c15b in -[NSView performKeyEquivalent:] ()
#2  0x017daf49 in -[ToolbarWindow performKeyEquivalent:]
                  (self=0x34070930, _cmd=0x90abc8b0, theEvent=0x2cefbb60)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsCocoaWindow.mm:1814
#3  0x9372aefa in -[NSWindow _cancelKey:] ()
#4  0x934fa1f9 in -[NSWindow _processKeyboardUIKey:] ()
#5  0x9334b34b in -[NSWindow sendEvent:] ()
#6  0x017df024 in -[NSWindow(MethodSwizzling)
                  nsCocoaWindow_NSWindow_sendEvent:] ()
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsCocoaWindow.mm:2198
#7  0x017d9700 in -[ToolbarWindow sendEvent:]
                  (self=0x34070930, _cmd=0x90ac4484, anEvent=0x35cdc550)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsCocoaWindow.mm:1857
#8  0x9333cd8c in -[NSApplication sendEvent:] ()
#9  0x932678e7 in -[NSApplication run] ()
#10 0x017d71a9 in nsAppShell::Run (this=0x1b620d00)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/widget/src/
       cocoa/nsAppShell.mm:591
#11 0x0169de01 in nsAppStartup::Run (this=0x1b63ff80)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/toolkit/components/
       startup/src/nsAppStartup.cpp:182
#12 0x0100fde2 in XRE_main (argc=1, argv=0xbffff9e0, aAppData=0x1b60c670)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/toolkit/xre/
       nsAppRunner.cpp:3220
#13 0x00002ce1 in main (argc=1, argv=0xbffff9e0)
    at /usr/local/src/Mozilla/hg/comm-central/mozilla/browser/app/
       nsBrowserApp.cpp:156

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with this patch (attachment 339500 [details] [diff] [review]):

https://build.mozilla.org/tryserver-builds/2008-09-19_12:[e-mail]-try-8f56b751a8e/[e-mail]

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #9)

Forget that tryserver build -- it doesn't work right (and probably is just trunk code without my patch).

Sigh :-(

I'll try another way to do an hg tryserver build.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

_This_ tryserver build works properly, and does contain my patch (attachment 339500 [details] [diff] [review]):

https://build.mozilla.org/tryserver-builds/2008-09-19_15:[e-mail]-bugzilla445531/[e-mail]

Comment 12

[Marcia Knous [:marcia]]

I can confirm that the tryserver build posted in Comment 11 works properly running on 10.4.11.

Comment 13

[Josh Aas]

Comment on attachment 339500 [details] [diff] [review]
Fix (work around Tiger-specific Apple bug)

+      nsCocoaUtils::GetCocoaEventModifierFlags(theEvent) & NSControlKeyMask
+      && nsCocoaUtils::GetCocoaEventKeyCode(theEvent) == kTabKeyCode) {

Our style is to put the && at the end of the first line, not at the beginning of the second line.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

> +      nsCocoaUtils::GetCocoaEventModifierFlags(theEvent) & NSControlKeyMask
> +      && nsCocoaUtils::GetCocoaEventKeyCode(theEvent) == kTabKeyCode) {
>
> Our style is to put the && at the end of the first line, not at the
> beginning of the second line.

I'll change this in my next revision, or when I land the patch.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central with Josh's change.

Comment 16

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b1pre) Gecko/20080926021000 Minefield/3.1b1pre.

Comment 17

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

Would be nice to have in Litmus for the over-next Firefox release. Even when we probably won't support 10.4 anymore. Adding litmus flag, so it's on our radar.