235966 - nsMenuBarListener.cpp opens the menu on Ctrl+Alt+T

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

User-Agent:       
Build Identifier: 

The problem can be tested with Linux and it will occur on Windows as well - as
soon as bug 197474 is fixed (see comment 42 there). The Tools menu opens on the
key combination Alt+T, but it opens on Ctrl+Alt+T as well.

The reason is in the method KeyPress:
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuBarListener.cpp#234

232       // If charCode == 0, then it is not a printable character.
233       // Don't attempt to handle accesskey for non-printable characters
234       if (IsAccessKeyPressed(keyEvent) && charCode)
235       {

IsAccessKeyPressed() doesn't check whether other modifier keys are down or not.
The easiest solution is probably making IsAccessKeyPressed() check other keys,
something like this (mostly copy&paste from KeyDown):

PRBool
nsMenuBarListener::IsAccessKeyPressed(nsIDOMKeyEvent* aKeyEvent)
{
  PRBool access = PR_FALSE;

  // No other modifiers can be down (SHIFT is ok here).
  // Especially CTRL.  CTRL+ALT == AltGR, and
  // we'll fuck up on non-US enhanced 102-key
  // keyboards if we don't check this.
  PRBool ctrl = PR_FALSE;
  if (mAccessKey == nsIDOMKeyEvent::DOM_VK_CONTROL)
    keyEvent->GetCtrlKey(&access);
  else
    keyEvent->GetCtrlKey(&ctrl);
  PRBool alt=PR_FALSE;
  if (mAccessKey == nsIDOMKeyEvent::DOM_VK_ALT)
    keyEvent->GetAltKey(&access);
  else
    keyEvent->GetAltKey(&alt);
  PRBool meta=PR_FALSE;
  if (mAccessKey == nsIDOMKeyEvent::DOM_VK_META)
    keyEvent->GetMetaKey(&access);
  else
    keyEvent->GetMetaKey(&meta);
  if (ctrl || alt || meta)) {
    return PR_FALSE;
  } else {
    return access;
  }
}


Reproducible: Always
Steps to Reproduce:
1. Open Mozilla under Linux and press Ctrl+Alt+T
Actual Results:  
The Tools menu opens.

Expected Results:  
The menu shouldn't open.

Comment 1

[Wladimir Palant]

Attachment: Proposed patch

Added a function GetModifiers() to get all modifiers of the event in a single
value - makes modifier checking much easier. Fixed the modifier check for menu
shortcuts, only Shift is allowed to be down now.

Comment 2

[Jan Varga [:janv]]

Comment on attachment 142997 [details] [diff] [review]
Proposed patch

Looks good
r=varga

Comment 3

[Wladimir Palant]

Attachment: Updated patch

The old patch bitrottened, here the new version of the same thing.

Comment 4

[Wladimir Palant]

Since bug 197474 has been fixed this matter is urgent now. The menus are
reacting to shortcuts they shouldn't get and this is a major annoyance. I don't
know why nobody complained before as this existed on Linux for years but Windows
users won't be that silent. I think this bug should be fixed as soon as possible
to get some testing even before the beta version comes out. Marking it
blocking1.8a3?

Comment 5

[Asa Dotzler [:asa]]

We're not going to hold the release for this but if it gets the proper reviews
in time for inclusion in 1.8a3, please request landing approval. 

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 153308 [details] [diff] [review]
Updated patch

unsetting 1.8a3 approval request. we've shipped already.

Comment 7

[Wladimir Palant]

Attachment: Additional patch to resolve conflict with bug 253798

Comment 8

[Wladimir Palant]

Attachment: Additional patch to resolve conflict with bug 253798

Forgot one file...

Comment 9

[timeless]

Comment on attachment 153308 [details] [diff] [review]
Updated patch

mozilla/layout/xul/base/src/nsMenuBarListener.h 	1.20
mozilla/layout/xul/base/src/nsMenuBarListener.cpp	1.71

Comment 10

[Wladimir Palant]

Attachment: Updated patch (the version checked in)

This is the patch that timeless checked in (slightly changed because of a
conflict). Then the patch in attachment 156492 [details] [diff] [review] had to be applied additionally
to fix the bustage.

Comment 11

[Wladimir Palant]

The bug doesn't occur in nightly build 2004081908 anymore, marking FIXED.