86770 - ALT,SPACEBAR is not activating control menu

Status: NEW

(Core :: XUL, defect)

Description

[Tim Powell]

ALT,spacebar does not activate the Windows control menu like ALT+spacebar does
(see bug 19328). The Windows control menu can also be triggered by clicking the
application icon at the left end of the title bar.

Steps to Reproduce:
1. Launch any Mozilla App.
2. Type ALT,SPACEBAR (press ALT, release ALT, hit spacebar)
(A variation on 2 that should also work is to press ALT, release ALT, arrow
right and left through the menus, and then press spacebar.)

Actual Result:
Focus goes to the menubar; other than that, nothing at all.

Expected Result:
Focus goes to the menubar, then the control menu activates so that a selection
can be made from it. This should behave similarly to how you can activate the
Edit menu by pressing Alt, Releasing Alt, and then pressing E.

Fails on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.1+) Gecko/2001061

This almost certainly occurs on all MS-Windows OS.

The same sort of widget may be triggered the same way with some X Window
Managers - don't know.

This bug an almost exact duplicate of bug 23445. I don't think it makes sense to
reopen that one since it has been somewhat confusingly marked as a duplicate of
bug 19328 and has comments about multiple spacebar presses or using
ALT+Spacebar. Bug 19328 (ALT+Spacebar) is fixed. This one (or bug 23445 if you
prefer) is not.

Comment 1

[Tim Powell]

Adding appropriate keywords. This is 4xp (worked in Nav 4.x), access (multiple
key combos may be problematic for disabled users, this may be the only way to
move/resize windows if the user has no mouse, and this is a Windows OS
standard). Because of access concerns, proposing for 0.9.3.

Comment 2

[Tim Powell]

Drat! I meant to include this from bug 23445 (although it seems pretty obvious): 

Fixing this probably means making [spacebar] an accelerator key for the main
menubar, which then selects the same action that ALT+spacebar is triggering
directly.

Comment 3

[Mike Pinkerton (not reading bugmail)]

i think this is a dupe

Comment 4

[John Morrison]

Just not note the heritage: bug 23445 was the original version of this bug 
report. It was duped to bug 19328 (which was for ALT+SPACE) as 'close enough'.
bug 19328 was fixed for ALT+SPACE, but ALT,SPACE was apparently dropped.

Not sure if there is an otherwise open bug for this, but I couldn't find one.

Comment 5

[:deanis74]

This should be pretty easy to add to nsMenuBarListener.cpp, at least I think it 
should...

Comment 6

[Joshua Prowse]

I think that this is an important bug for usability and accessibility.  I think 
that it should be moved from "Future" to Mozilla 1.01

Comment 7

[:deanis74]

taking

Comment 8

[:deanis74]

Attachment: This should do it. Anyone care to review?

Comment 9

[:deanis74]

Attachment: removed commented-out code. reviews?

Comment 10

[:deanis74]

Adding Aaron, since it's got the access keyword.

Comment 11

[:deanis74]

*** Bug 23445 has been marked as a duplicate of this bug. ***

Comment 12

[:deanis74]

Second call for reviews.  It's quick and painless... anyone??

Comment 13

[:deanis74]

Adding Rod and Bill, known Windows dabblers, to check this out.

Comment 14

[Jesse Ruderman]

*** Bug 145562 has been marked as a duplicate of this bug. ***

Comment 15

[Ere Maijala (slow)]

I tried it, but the patch doesn't apply cleanly. Dean, could you create a diff
that works for the current cvs?

Comment 16

[Ere Maijala (slow)]

Attachment: Updated patch to apply cleanly to current tree

I updated the old patch to apply cleanly to current tree. It looks good to me,
but I had to make some changes to make it compile. Therefore it's maybe not
appropriate for me to r= it..

Comment 17

[:deanis74]

Was the only change from using aLetter to aKeyEvent->GetCharCode?  The change in
ShortcutNavigation from having aLetter as a parameter to aKeyEvent was in the
fix for bug 92491.

How about you review my code and I'll review your change?

Comment 18

[Ere Maijala (slow)]

Comment on attachment 92136 [details] [diff] [review]
Updated patch to apply cleanly to current tree

Yes, that's the only real change.

r=[e-mail] for everything but my lines of the patch.

Comment 19

[:deanis74]

and r=me for that change

Comment 20

[Alec Flett]

I want to see someone familiar with keys review this, such as akkana - not sure
it makes sense to hardcode this all into C++? I mean, aren't there similar
keystrokes to pull down the menu on the mac.. not sure about unix.

Comment 21

[:deanis74]

Well, on Mac we use the native menu so we don't have to worry about handling 
keypresses there.  I can't speak for *nix, does it have keyboard access to the 
system menu?  Does it have a concept of a system menu?

Akkana, can you take a look at this re: Alec's comment?

Comment 22

[:deanis74]

From IRC:

<bz> dean: well.. the unix thing is like this
<bz> dean: you hit a key
<bz> dean: the first thing that gets a crack at it is the X server itself
<bz> dean: it usually does nothing with it
<bz> dean: then the windowmanager gets a shot at it
<bz> dean: if _that_ does nothing with it, the app gets it
<dean> bz: so you already have keyboard access to the system menu?
<bz> dean: so the point is, the WM will do whatever it should do
<bz> dean: yes
<bz> dean: that's the upshot
<bz> dean: you would have to work very hard to prevent it

Alec, since this is only Windows-only, should we worry about the key being hard-
coded in the .cpp?  We could do something like the menu access key in 
nsMenuBarListener 
(http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuBarListener.cpp
#106), but that seems like overkill right now.

Comment 23

[Akkana Peck]

I'm not familiar with what the "control menu" is, but I have two questions after
looking at the patch:

1. If ALT+space was already working correctly, and it was just ALT,space that
needed work, why do we need this whole new #ifdef XP_WIN routine?  Can't
ALT,space do whatever ALT+space was already doing?

2. Are we absolutely sure that Windows is the only platform which will ever want
this functionality, and that no windows user would ever want to turn off the
functionality?  Generally we try to stay away from platform ifdefs in mozilla
code, and use dynamic libraries, LookAndFeel entries, prefs and other
runtime-changeable methods to control behavior.  What if OS2 or Be or some other
minor platform turned out to want the same behavior?  But if this is something
that can only have meaning on a Windows system, the ifdef may be warranted.  I
don't have a clear enough understanding of what this menu is to know.

Comment 24

[timeless]

the reason alt,space is special...
  alt-space is handled by the os
  alt,space should be handled by the os -- Except that we don't have an os
managed menubar, and we handle alt and then space on our own (or try to, this
bug is about the fact that we don't properly handle it) so the os doesn't.

So instead alt,space does nothing useful.  The only os's w/ system menus as such
are Windows, OS/2 PM and maybe QNX Photon (it's been a while, and i didn't
really fiddle with alt-space/alt,space while I was keying around Photon). I
think technically OS/2 is supposed to behave the same way as Windows (i can
check the next time I visit OS/2).

I can speak for BeOS, it doesn't have a system menu. BeOS behaves like macos
classic wrt widgets if you ignore the menubar's position.

If OS/2 and QNX Photon need this define, then we can make the ifdef generic or
we could just provide a stub that returns notimplemented/notavailable if you prefer.

Comment 25

[:deanis74]

> 1. If ALT+space was already working correctly, and it was just ALT,space that
> needed work, why do we need this whole new #ifdef XP_WIN routine?  Can't
> ALT,space do whatever ALT+space was already doing?

When Alt+Space is pressed, we ignore it so it is handled by Windows so the
system menu pops up.  For Alt,Space though, pressing and releasign Alt activates
the menu bar and we handle all key strokes after that.  As timeless said, we
don't have an os-managed menu bar so we have to manually recreate this specific
functionality.

Comment 26

[Ere Maijala (slow)]

You can just think that pressing space while focus is on the menu bar (and
there's no menu open) should open system menu.

Comment 27

[:deanis74]

akkana: Did you get your questions answered to your satisfaction?

mkaply: Does OS/2 need this as well?  Does it work on OS/2 by simply expanding
this #ifdefs to include OS/2?

Comment 28

[:deanis74]

Akkana, Alec: we hard-code F10 in C++ in nsMenuBarListener.cpp.

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuBarListener.cpp#248

In talking to users of other platforms this is only for Windows, but might be
for OS/2 because I haven't heard from anyone using that platform.

Comment 29

[Vaclav Dvorak]

I believe this applies to OS/2 too, although I admit it's been a while since I
used OS/2.

Comment 30

[timeless]

*** Bug 197333 has been marked as a duplicate of this bug. ***

Comment 31

[Matthew Fowles]

this bug also applies to mail.

Comment 32

[Mart Rootamm]

I've got Moz 1.2.1. I haven't checked it recently, but it may assumingly be
possible that it might be with every window that in Windows has to be closed
with Alt+F4.

Comment 33

[Henrik Gemal]

what's the status here? Still looking for review from sspitzer. Can we check it
in after that?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  PRUint32 charCode;
+  aKeyEvent->GetCharCode(&charCode);
+  if (charCode == NS_VK_SPACE)
+    ShowSystemMenu();
+  else

brendanize this by turning it into 

if (charCode == NS_VK_SPACE) {
  ShowSystemMenu();
  return NS_OK;
}

I think you should really move the Windows part of ShowSystemMenu to nsIWidget
and widget/src/windows/nsWindow.{h,cpp}. The rest of ShowSystemMenu in
nsMenuBarFrame should just call nsFrame::GetWindow(aPresContext, &window) and
then window->ShowSystemMenu(); that could even be inlined into the if() statement.

Comment 35

[:deanis74]

I can move ShowSystemMenu into nsIWidget, sure, and just make it a no-op for
other platforms.  I'll still need an #ifdef XP_WIN in nsMenuBarFrame.h, if
you're OK with that, unless I add a second function to nsIWidget like
SupportsKeyboardSystemMenuAccess (exaggeration), but that seems like overkill to me.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I'll still need an #ifdef XP_WIN in nsMenuBarFrame.h, if you're OK with that

Yeah, I'm OK with that. It's entire Windows-specific functions that bum me out.

Comment 37

[:deanis74]

I finally got back to thinking about this.  roc, is it really necessary to move
ShowSystemMenu into nsIWidget if its only caller is wrapped in #ifdef XP_WIN?

Comment 38

[timeless]

ok, the system menu is in the menu loop for os/2 and needs to be triggered by
mozilla. so the api should probably be stubbed for non winish os's.

i'll check on photon some other time.

Comment 39

[Adam Guthrie]

*** Bug 343869 has been marked as a duplicate of this bug. ***

Comment 40

[Daniel Dumanis]

sorry about that, I didn't realize that alt+space is different from alt, space.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:enndeakin, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 44

[Mike Kaply [:mkaply]]

Looking at other windows apps, Alt,Spacebar should not show the system menu, but it should activate the first menu, which we don't - space makes the focus leave the menubar (even if the menubar is showing).

So the bug here is that space should act like it's invoking the file menu.

Comment 45

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.