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

VERIFIED FIXED

Status

()

defect
P1
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: jruderman, Assigned: smichaud)

Tracking

({platform-parity, verified1.9.1})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

11 years ago
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.
Flags: blocking-firefox3.1?
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.)
Maybe using keypress instead of keydown would fix this. Not sure.
Severity: minor → normal
Depends on: 445573
Keywords: pp
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.
Version: unspecified → Trunk
Is this still an issue?
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.
Reporter

Updated

11 years ago
Summary: Esc key should cancel out of Ctrl+Tab panel → [10.4] Esc key should cancel out of Ctrl+Tab panel
Assignee: nobody → joshmoz
Component: Tabbed Browser → Widget: Cocoa
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: tabbed.browser → cocoa
Summary: [10.4] Esc key should cancel out of Ctrl+Tab panel → [10.4] Esc key doesn't cancel out of Ctrl+Tab panel
Summary: [10.4] Esc key doesn't cancel out of Ctrl+Tab panel → [10.4] Esc key doesn't cancel out Ctrl+Tab panel
Flags: blocking1.9.1?

Updated

11 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Assignee: joshmoz → smichaud

Updated

11 years ago
Priority: -- → P1
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.
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.
Attachment #339500 - Flags: review?(joshmoz)
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
(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.
I can confirm that the tryserver build posted in Comment 11 works properly running on 10.4.11.

Comment 13

11 years ago
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.
Attachment #339500 - Flags: superreview?(roc)
Attachment #339500 - Flags: review?(joshmoz)
Attachment #339500 - Flags: review+
> +      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.
Attachment #339500 - Flags: superreview?(roc) → superreview+
Landed on mozilla-central with Josh's change.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED

Updated

11 years ago
Keywords: verified1.9.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.
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.